perf(detect): index ignore patterns by anchor; lru_cache fix - #2845
perf(detect): index ignore patterns by anchor; lru_cache fix#2845stephanGarland wants to merge 1 commit into
Conversation
…che (Graphify-Labs#1958) detect() accumulates ignore patterns into one flat [(anchor_dir, pattern)] list and _is_ignored() rescanned all of it per path. Two costs fell out of that on a 356k-file corpus with 1,697 .gitignore files (57,286 patterns): Anchor selection. The scan tested every anchor with target.relative_to(anchor). On CPython 3.12+ that scans target.parents linearly (_PathParents is a Sequence with no __contains__ override), minting a Path per element whose __eq__ raises and catches an AttributeError off an unset __slots__ member, then throws ValueError away for the ~97% of anchors that are not ancestors. Measured 813ms per path. _AnchorIndex buckets the list by anchor so a lookup walks the target's own ancestor chain instead — the same candidate set relative_to accepted, in list order so last-match-wins and ! negations are unaffected. The list is append-only during a walk, so the index refreshes over the new tail only. Cyclic garbage. _match_anchored_ignore_pattern wrapped a recursive closure in its own @lru_cache, so every call built a reference cycle only the GC could reclaim (14 cyclic objects per call), and the cache was discarded before it could serve a second call. It is now a module-level function taking an explicit dict memo, with patterns lacking ** skipping the recursion entirely (54 of 57,283 patterns contain **). Behavior is unchanged: differentially fuzzed against the previous implementations — 200k random plus exhaustive small-space pattern/path pairs for the matcher, and 3,000 random pattern sets x 40 files for _is_ignored, covering sibling anchors, negations and directory-only rules — 0 mismatches. On a synthetic 1,500-anchor / 30k-pattern corpus, 807ms -> 0.089ms per path, and 500 globstar matches now leave 0 cyclic objects. Tests: 18 new in test_detect.py. Suite: 4,621 passed, 48 skipped, no regressions (test_labeling.py::test_label_communities_batches_when_over_batch_size and the four in test_ollama_retry_cap.py fail identically before this change). Co-Authored-By: Claude Opus 5 (1M context) <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 5 advisory finding(s) below merit a look before merge.
Formal verification. 1 change(s) tested, no difference found (not proven).
Graphify review — findings
Replaces _is_ignored's full-list pattern scan with an anchor-bucketed _AnchorIndex (memoized by list identity via _anchor_index) that yields only ancestor-anchored patterns in original order, avoiding the per-anchor relative_to/ValueError cost on large monorepos. Rewrites the nested lru_cache globstar matcher as a module-level _match_globstar_parts with a passed-in memo and adds a **-free fast path in _match_anchored_ignore_pattern. Adds test coverage asserting the index matches the old relative_to survivor set and preserves last-match-wins ordering.
Worth a look
- AnchorIndex._refresh may not detect in-place mutation when list length is unchanged —
graphify/detect.py· 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.
- id()-keyed memo can return a stale/foreign index after list is freed and address recycled —
graphify/detect.py· 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.
- Memo can return stale index if a mutated list keeps same id() —
graphify/detect.py· 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.
- Anchor index not refreshed when pattern anchors are replaced in-place —
graphify/detect.py:1336· 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.
- Cached anchor index ignores in-place replacement of existing pattern entries —
graphify/detect.py:1358· 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 — 1577 functions depend on the 507 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()— 107 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 — 1577 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: 825 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
No difference found (not proven): No behavior difference found in \_match\_anchored\_ignore\_pattern (not a proof).
The verifier ran both versions of \_match\_anchored\_ignore\_pattern on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
· 33 more finding(s) on lines outside this diff (see the check run).
|
Ran this against our own test suite (test_detect.py + full pytest tests/) and a real ~84k-file corpus with 304 .gitignore — clean, identical baseline failures, no regressions. We'd independently implemented our own fix for the #1958 half (module-level function + bounded lru_cache instead of a per-call memo dict) before seeing this PR, but since this covers both #2834 and #1958 together, we won't submit a duplicate — this looks like the way to go. |
Fixes #1958 and #2834
detect() accumulates ignore patterns into one flat [(anchor_dir, pattern)] list and _is_ignored() rescanned all of it per path. Two costs fell out of that on a 356k-file corpus with 1,697 .gitignore files (57,286 patterns):
Anchor selection. The scan tested every anchor with target.relative_to(anchor). On CPython 3.12+ that scans target.parents linearly (_PathParents is a Sequence with no contains override), minting a Path per element whose eq raises and catches an AttributeError off an unset slots member, then throws ValueError away for the ~97% of anchors that are not ancestors. Measured 813ms per path. _AnchorIndex buckets the list by anchor so a lookup walks the target's own ancestor chain instead — the same candidate set relative_to accepted, in list order so last-match-wins and ! negations are unaffected. The list is append-only during a walk, so the index refreshes over the new tail only.
Cyclic garbage. _match_anchored_ignore_pattern wrapped a recursive closure in its own @lru_cache, so every call built a reference cycle only the GC could reclaim (14 cyclic objects per call), and the cache was discarded before it could serve a second call. It is now a module-level function taking an explicit dict memo, with patterns lacking ** skipping the recursion entirely (54 of 57,283 patterns contain **).
Behavior is unchanged: differentially fuzzed against the previous implementations — 200k random plus exhaustive small-space pattern/path pairs for the matcher, and 3,000 random pattern sets x 40 files for _is_ignored, covering sibling anchors, negations and directory-only rules — 0 mismatches. On a synthetic 1,500-anchor / 30k-pattern corpus, 807ms -> 0.089ms per path, and 500 globstar matches now leave 0 cyclic objects.
Tests: 18 new in test_detect.py. Suite: 4,621 passed, 48 skipped, no regressions (test_labeling.py::test_label_communities_batches_when_over_batch_size and the four in test_ollama_retry_cap.py fail identically before this change).