perf(detect): hoist per-pattern work out of the ignore-matching loop - #2869
perf(detect): hoist per-pattern work out of the ignore-matching loop#2869Alond wants to merge 2 commits into
Conversation
`_is_ignored` re-did the same work for every (target, pattern) pair, so the
cost scaled with patterns × targets even though most of it depends on neither
the pattern nor the target:
- pattern parsing (`startswith("!")`, `endswith("/")`, `strip("/")`) does not
depend on the target at all, yet ran once per target per pattern;
- `target.relative_to(anchor)` + `_nfc()` depend only on the anchor, and
anchors are few while patterns are many;
- `target.relative_to(root)` + `_nfc()` depend on neither;
- `target.is_dir()` (an lstat) ran for every directory-only pattern instead of
once per target.
Now the parse is done once per scan and kept in the shared cache, the
relative paths are memoised per target, and `is_dir()` is lazy.
Measured on an iOS repo (3614 detected files, ~55 ignore patterns from four
.gitignore files, 74k files on disk):
detect() 5.34s -> 2.24s (2.4x)
graphify update . 13.05s -> 9.89s
`detect()` output is byte-identical before and after — compared the full
result dict (files, ignored, unclassified), not just counts.
The parse cache invalidates on pattern count, because detect() appends
patterns from nested .gitignore files *while the walk is running*. Caching
the parse without that guard silently stops the later patterns from being
applied; the added test covers it and fails without the guard.
Full suite: same 25 pre-existing failures before and after, no new ones.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 3 advisory finding(s) below merit a look before merge.
Formal verification. No changes could be formally verified in this run.
Graphify review — findings
Optimizes _is_ignored in graphify/detect.py by hoisting the per-target pattern parsing into a per-scan cache (keyed on pattern count so nested .gitignore patterns appended mid-walk still take effect) and memoizing relative_to/_nfc results and the is_dir check within a single call. Adds test_nested_gitignore_applies_when_patterns_grow_mid_walk to guard the cache-invalidation behavior when the pattern list grows during the walk.
Worth a look
- Prepared ignore-pattern cache can be reused for different same-length pattern lists —
graphify/detect.py:1342· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review Execution auto-disposal is off for this run; enable it (with sandbox isolation) to have Graphify try to confirm or refute this automatically.
- Prepared-pattern cache keyed only on pattern count silently drops later patterns when count is unchanged but contents differ —
graphify/detect.py:1348· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review Execution auto-disposal is off for this run; enable it (with sandbox isolation) to have Graphify try to confirm or refute this automatically.
- Prepared-pattern cache invalidation keyed only on pattern count can serve stale parse —
graphify/detect.py:1360· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review Execution auto-disposal is off for this run; enable it (with sandbox isolation) to have Graphify try to confirm or refute this automatically.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 1554 functions depend on the 484 functions this change touches.
Health — this change adds coupling hotspots:
- new:
extract()— 468 callers, 41 callees - new:
_rebuild_code()— 98 callers, 51 callees - new:
detect()— 108 callers, 15 callees - new:
save_manifest()— 34 callers, 11 callees - new:
extract_files_direct()— 17 callers, 20 callees - new:
extract_xaml()— 19 callers, 17 callees - new:
extract_corpus_parallel()— 26 callers, 10 callees - new:
dispatch_command()— 2 callers, 117 callees - …and 25 more — each is listed as a finding
Verification — 1554 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 802 function(s) in the blast radius were not formally verified this run
Formal verification
Could not verify: Could not verify \_is\_ignored.
The verifier did not have enough to check \_is\_ignored, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: parameter `path` is annotated `Path` — outside the synthesizable primitive/collection set
· 33 more finding(s) on lines outside this diff (see the check run).
… its length Review flagged the parse cache three times over: keyed on len(patterns) alone, a shared _cache can serve one pattern list's parse to a different list of the same length. Not reachable from detect() — it builds ignore_patterns once and only ever appends to it, so the length is effectively a monotonic version counter. But _is_ignored takes `patterns` and `_cache` as separate arguments, so the invariant lives in the caller, not in the function, and nothing enforces it. Now the entry is (patterns, len(patterns), prepared) and the hit requires `cached[0] is patterns`. Keeping the list referenced in the cache also keeps it alive, so the `is` check cannot be fooled by a new list allocated at a freed one's address — which is why this pins the object rather than id(). The length stays in the key: it is what makes nested .gitignore patterns appended mid-walk take effect, and the existing test covers that. Added test fails on the previous commit and passes here. tests/test_detect.py: 250 passed. detect() on the same iOS repo: 2.36s (was 5.34s before this branch), so the extra identity check costs nothing measurable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.
Formal verification. No changes could be formally verified in this run.
Graphify review — findings
Optimizes _is_ignored in graphify/detect.py by parsing the ignore-pattern list once per scan and memoizing the per-target relative_to/_nfc and is_dir() work, instead of redoing them for every pattern. The parse cache is keyed on both the pattern list's identity and length so it stays valid as detect() appends nested .gitignore patterns mid-walk and never serves one list's parse to another. Adds two test_detect cases covering the grow-mid-walk invalidation and the cross-list cache isolation.
Worth a look
- Prepared-pattern cache keyed by list identity fails when detect() extends patterns in place —
graphify/detect.py:1353· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review Execution auto-disposal is off for this run; enable it (with sandbox isolation) to have Graphify try to confirm or refute this automatically.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 1556 functions depend on the 486 functions this change touches.
Health — this change adds coupling hotspots:
- new:
extract()— 468 callers, 41 callees - new:
_rebuild_code()— 98 callers, 51 callees - new:
detect()— 108 callers, 15 callees - new:
save_manifest()— 34 callers, 11 callees - new:
extract_files_direct()— 17 callers, 20 callees - new:
extract_xaml()— 19 callers, 17 callees - new:
extract_corpus_parallel()— 26 callers, 10 callees - new:
dispatch_command()— 2 callers, 117 callees - …and 25 more — each is listed as a finding
Verification — 1556 functions in the blast radius were not formally verified this run (proofs are advisory here).
Health delta baseline: last indexed commit b14b52e (diverged from this PR's base — delta is approximate).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 804 function(s) in the blast radius were not formally verified this run
Formal verification
Could not verify: Could not verify \_is\_ignored.
The verifier did not have enough to check \_is\_ignored, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: parameter `path` is annotated `Path` — outside the synthesizable primitive/collection set
· 33 more finding(s) on lines outside this diff (see the check run).
1 participant
What
_is_ignoredre-did the same work for every(target, pattern)pair, so the cost scaled withpatterns × targetseven though most of that work depends on neither the pattern nor the target:startswith("!"),endswith("/"),strip("/")) does not depend on the target at all, yet ran once per target per pattern;target.relative_to(anchor)+_nfc()depend only on the anchor — anchors are few, patterns are many;target.relative_to(root)+_nfc()depend on neither;target.is_dir()(anlstat) ran for every directory-only pattern instead of once per target.The parse is now done once per scan and kept in the shared cache, the relative paths are memoised per target, and
is_dir()is lazy. No behaviour change intended.Numbers
Measured on an iOS repo: 3614 detected files, ~55 ignore patterns across four
.gitignorefiles, 74k files on disk (60k of them under a git-ignored.build/).detect()graphify update .Profiling the original pointed at exactly this loop: 762k
Path.relative_tocalls, 4.6Mfnmatch, 1.36Mlstat, withdetect.py:_matchesat 5.8s cumulative.Correctness
detect()output is byte-identical before and after — I compared the full result dict (files,ignored,unclassified, …), not just counts. Counts alone would not have caught the bug below.Full suite: the same 25 pre-existing failures before and after, no new ones.
tests/test_detect.pyandtests/test_ignore_file_encoding.py: 261 passed.The trap worth reviewing carefully
Caching the parse needs invalidation, because
detect()appends patterns from nested.gitignorefiles while the walk is already running (ignore_patterns.extend(...)inside the walk loop). My first version cached the parse unconditionally and silently stopped applying the nested rules — and the detected file count stayed the same, so it looked fine.The cache key is therefore
len(patterns), andtests/test_detect.py::test_nested_gitignore_applies_when_patterns_grow_mid_walkcovers it. That test needs a root-level pattern in the fixture: without one,_is_ignoredreturns before touching the cache and the bug cannot reproduce. I verified the test fails with the guard removed and passes with it — an earlier version of the test passed in both directions and was worthless.Notes
_PREPARED_KEY,_UNSET) are module-level; the string cache key cannot collide with thePathkeys the ancestor memo uses.