perf: annotate carbonah E002/E003 false positives (efficiency round 2b) - #185
Closed
tina4stack wants to merge 3 commits into
Closed
tina4stack wants to merge 3 commits into
tina4stack wants to merge 3 commits into
Conversation
Pin the behaviour carbonah's N+1 heuristic misreads, with a real result AND a real query count on SQLite (no mocks): execute_many collapses 200 rows into one statement (anti-N+1), the RETURNING fallback still writes every row, and the migration apply+rollback loop reaches the right end state. Mutation-proven. Co-Authored-By: Claude Opus 4.8 <[email protected]> Co-Authored-By: Tina4 <[email protected]> Signed-off-by: Andre van Zuydam <[email protected]>
carbonah 0.3.3 flagged 9 E002 (N+1) + 1 E003 (unbounded). Investigation found 0 genuine, safely-fixable findings: all are heuristic false positives or behaviour-locked intentional patterns — the execute_many batch primitive (the anti-N+1 itself), its row-at-a-time fallback, sequential migration DDL, a per-migration rollback DELETE inside its own txn, a spatial-index DDL per PointField, a race-safe CREATE TABLE retry loop, the admin SQL console running the developer's own multi-statement batch, and a single-row PK lookup via fetch_one. Each is annotated '# carbonah:ignore <CODE>' with a one-line reason, so the reported count drops with zero runtime change (production diff is 100% comments). The migration v2->v3 backfill keeps its per-row commit deliberately: a single batch would abort the whole PostgreSQL transaction on one bad row and break the log-and-continue contract. carbonah lint: E002 9->0, E003 1->0. E004 12->11 as a same-loop side effect — the session retry-loop annotation also clears carbonah's coupled 'polling loop' finding for that same bounded 5-attempt backoff (also a false positive). Co-Authored-By: Claude Opus 4.8 <[email protected]> Co-Authored-By: Tina4 <[email protected]> Signed-off-by: Andre van Zuydam <[email protected]>
Co-Authored-By: Claude Opus 4.8 <[email protected]> Co-Authored-By: Tina4 <[email protected]> Signed-off-by: Andre van Zuydam <[email protected]>
Owner
Author
|
Superseded by the Round 3 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Efficiency round 2b — carbonah E002 (N+1) + E003 (unbounded)
carbonah 0.3.3 flagged 9 E002 (N+1) and 1 E003 (unbounded) in
tina4_python.I traced each against the source. Every one is a heuristic false positive or a
behaviour-locked intentional pattern — zero genuine, safely-fixable findings.
Each is annotated
# carbonah:ignore <CODE>with a one-line reason, so the reportedcount drops with zero runtime change: the production diff is 100% comments.
carbonah lint: before → after
E004 12→11 is a same-loop side effect: annotating the E002 in session's
_ensure_tableretry loop also clears carbonah's coupled "polling loop" E004 forthat same bounded 5-attempt backoff — itself a false positive. No behaviour changed.
Findings — all left (documented), none needed a code fix
build_batch_insertscollapses N single-row INSERTs into ~1 multi-row statement — one round-trip per chunk, not per rowwhileis a bounded retry, executes once on successfetch_one(WHERE id = ?→ at most one row)The migration backfill (275) is the one genuinely inefficient loop, but its per-row
commit is deliberate — a single batch would abort the whole PostgreSQL transaction on
one bad row and break the log-and-continue contract. Fixing it would regress behaviour
on a one-time cold path for no hot-path win, so it is left with a reason.
Characterization tests (real SQLite, no mocks — mutation-proven)
execute_manycollapsible:build_batch_insertsreturns 1 statement for 200 rows (query-count) and all 200 land with the rightaffected_rows/data (result).[]), rows still written via the fallback loop.carbonah measure (batch workload, 3 runs each)
Branch median SCI
0.00022239vs baseline0.00021779— within noise; grade APlus both.Verification
[needs:firebird], no live Firebird provisioned).tina4 metrics: unaffected — no file became a new offender; comments add no complexity.Do not merge — for review.
🤖 Generated with Claude Code