perf: release JS parse trees between symbol-fact collection files - #3353
perf: release JS parse trees between symbol-fact collection files#3353VasuBansal7576 wants to merge 3 commits into
Conversation
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
Refactors JS/TS symbol-resolution collection to process files one at a time via a new _collect_js_symbol_resolution_facts_batch, so each source's parse tree is released before the next is parsed instead of holding every tree in memory at once. Parsing now indexes only the relevant node kinds (imports/exports, lexical declarations, classes) per file rather than re-walking the whole tree for each pass, and the streaming path preserves the original fact ordering (all calls uses before class type uses). Falls back to the batch collector when the input contains duplicate resolved paths (e.g. symlinks), keeping the "last parsed tree wins" behavior for mixed grammars.
Worth a look
- Only 'calls' vs non-'calls' partition assumed, but 'references'/'inherits' ordering not guaranteed to match batch —
graphify/extractors/resolution.py:1511· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 1731 functions depend on the 132 functions this change touches.
Health — this change adds coupling hotspots:
- new:
extract()— 537 callers, 43 callees - new:
_rebuild_code()— 113 callers, 50 callees - new:
_extract_generic()— 18 callers, 25 callees - new:
extract_js()— 85 callers, 4 callees - new:
extract_xaml()— 19 callers, 17 callees - new:
dispatch_command()— 2 callers, 124 callees - new:
extract_objc()— 27 callers, 9 callees - new:
_resolve_js_module_path()— 27 callers, 6 callees - …and 34 more — each is listed as a finding
Verification — 1731 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: 738 function(s) in the blast radius were not formally verified this run
Formal verification
Could not verify: Could not verify \_collect\_js\_symbol\_resolution\_facts.
The verifier did not have enough to check \_collect\_js\_symbol\_resolution\_facts, 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 `facts` is annotated `_SymbolResolutionFacts` — outside the synthesizable primitive/collection set
· 1 grounded finding(s) anchored inline below; 41 more finding(s) on lines outside this diff (see the check run).
There was a problem hiding this comment.
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Formal verification. No changes could be formally verified in this run.
Graphify review — findings
Splits JS symbol-resolution collection so that when a path batch has no duplicate resolved paths, each file is parsed in its own _SymbolResolutionFacts scope via _collect_js_symbol_resolution_facts_batch and its native tree is released before the next parse; batches containing duplicate/symlinked paths fall back to the original multi-pass batch collector to keep the last-tree-wins and mixed-grammar behavior. Preserves the existing fact ordering by emitting all calls uses before class-type uses across files, and speeds the batch pass by pre-indexing only import/export, lexical-declaration, and class nodes per tree instead of re-walking the whole tree for each phase. Adds scripts/benchmark_js_fact_collection.py, which samples tracked JS/TS files from a source checkout and compares two git checkouts for elapsed time, peak RSS, and byte-identical ordered output across fresh alternating worker processes.
No blocking issues surfaced. 9 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 1739 functions depend on the 140 functions this change touches.
Health — this change adds coupling hotspots:
- new:
extract()— 539 callers, 43 callees - new:
_rebuild_code()— 113 callers, 50 callees - new:
_extract_generic()— 18 callers, 25 callees - new:
extract_js()— 85 callers, 4 callees - new:
extract_xaml()— 19 callers, 17 callees - new:
dispatch_command()— 2 callers, 124 callees - new:
extract_objc()— 27 callers, 9 callees - new:
_resolve_js_module_path()— 27 callers, 6 callees - …and 35 more — each is listed as a finding
Verification — 1739 functions in the blast radius were not formally verified this run (proofs are advisory here).
Health delta baseline: last indexed commit c9f9901 (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: 746 function(s) in the blast radius were not formally verified this run
Formal verification
Could not verify: Could not verify \_collect\_js\_symbol\_resolution\_facts.
The verifier did not have enough to check \_collect\_js\_symbol\_resolution\_facts, 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 `facts` is annotated `_SymbolResolutionFacts` — outside the synthesizable primitive/collection set
· 2 grounded finding(s) anchored inline below; 41 more finding(s) on lines outside this diff (see the check run).
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. No changes could be formally verified in this run.
Graphify review — findings
Reworks JS symbol-resolution fact collection to process each resolved file's occurrences in its own group via _collect_js_file_group_facts, so it no longer retains every parsed tree in memory at once while preserving the emit order (per-occurrence declarations/imports/aliases, then function calls, then class type uses) and the multi-pass contract where exports and class uses reuse the last successful parse. Adds scripts/benchmark_js_fact_collection.py, which measures fact-collection or full-graph extraction time, peak RSS, and output hash across two git checkouts using a seeded file sample in alternating fresh processes with isolated caches. Adds tests confirming native trees are released before parsing the next file and before releasing unrelated file groups.
Worth a look
- Exports/calls/class-uses attributed to wrong file path when a resolved file has multiple occurrences —
graphify/extractors/resolution.py:1594· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Exports/calls/classes for all occurrences use only the last successfully-parsed occurrence's tree —
graphify/extractors/resolution.py:1596· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- JS fact merge drops module_imports —
graphify/extractors/resolution.py:1514· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Occurrence whose parse fails still receives facts via zip, but occurrences after a failed one get empty facts inconsistently —
graphify/extractors/resolution.py:1520· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- First-pass import/export/alias facts now use each occurrence's own tree but export second-pass reuses only last tree, dropping earlier occurrences' export data —
graphify/extractors/resolution.py:1544· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 1743 functions depend on the 144 functions this change touches.
Health — this change adds coupling hotspots:
- new:
extract()— 539 callers, 43 callees - new:
_rebuild_code()— 113 callers, 50 callees - new:
_extract_generic()— 18 callers, 25 callees - new:
extract_js()— 85 callers, 4 callees - new:
extract_xaml()— 19 callers, 17 callees - new:
dispatch_command()— 2 callers, 124 callees - new:
extract_objc()— 27 callers, 9 callees - new:
_resolve_js_module_path()— 27 callers, 6 callees - …and 36 more — each is listed as a finding
Verification — 1743 functions in the blast radius were not formally verified this run (proofs are advisory here).
Health delta baseline: last indexed commit 67f99bd (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: 750 function(s) in the blast radius were not formally verified this run
Formal verification
Could not verify: Could not verify \_collect\_js\_symbol\_resolution\_facts.
The verifier did not have enough to check \_collect\_js\_symbol\_resolution\_facts, 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 `facts` is annotated `_SymbolResolutionFacts` — outside the synthesizable primitive/collection set
· 3 grounded finding(s) anchored inline below; 41 more finding(s) on lines outside this diff (see the check run).
|
Checked all five semantic advisories in the latest bot review against the original collector at
All 11 independent expected-output cases pass against the original collector. The revised implementation also matches it in 200 seeded cases with interleaving, mixed grammars, repeated/missing paths, and parse failures. Both full Next.js fact comparisons completed with exact ordered output in all six pairs. These are executed compatibility checks, not a formal-proof claim. Tests on the reviewed commit and passing CI provide the evidence. No behavior change is warranted for these advisories. |
What changes
JS/TS symbol-fact collection retains syntax trees for an entire repository and repeatedly walks them.
This PR indexes relevant syntax nodes once and releases native trees after collecting each resolved-file group.
The previous PR's whole-corpus fallback for duplicate paths is removed: the full Next.js corpus now benefits too.
Graph and fact storage still scale with output size; this is not a constant-total-memory claim.
Each occurrence keeps its original input position, so
[A.ts, B.ts, alias-of-A.js]retains exact fact order.Declarations, imports, and aliases use each occurrence's grammar; exports, calls, and class uses retain the original last-successful-parse behavior, including failed aliases.
Calls precede class-type uses, and existing facts remain prefixes.
The follow-up also removes redundant type checks and repeated source-path resolution, replaces string-keyed indexes with local lists, and renames the group helper.
Repeated full-corpus measurements
Original PR base
937e59aversus fixed headac3449f, using the same Python 3.13.11 environment and parser versions.Each row is the median of three alternating before/after pairs in fresh processes, covering all tracked eligible JS/TS-family files, including fixtures, bundles, and symlinks.
A separate three-pair comparison against previous PR head
c2a8667isolates this follow-up: Next.js collector median RSS falls 2,504 → 380 MiB, with time 51.11s → 31.46s.All 15 comparison pairs preserve exact ordered output; every graph run reports zero failed sources.
Immutable receipts, ranges, hashes, commands, and validation metadata.
Pinned sources: Next.js
4fed8eaand webpack92c4650.Next.js includes five duplicate-path groups, such as
hmr-symlink/app/symlink-chain/page.tsx→symlink-link/page.tsx→symlink-target/page.tsx; these remain in the benchmark.From the fixed checkout, with the pinned original checkout and source beside it:
python scripts/benchmark_js_fact_collection.py \ --baseline ../graphify-original --candidate . --source ../next.js \ --files 0 --runs 3 --mode graph --output /tmp/next-graph.jsonUse
--mode factsfor collection or change--sourceto the pinned webpack checkout.Earlier VS Code, OpenClaw, and other measurements describe the previous PR revision, not this fix.
Validation and limits
Linux CI passes: 5,490 tests on Python 3.10; 5,489 on Python 3.12.
CI tested this head merged into upstream
67f99bd; the benchmark comparisons use the original base separately.The security job retains its existing non-blocking findings in unchanged code and build dependencies.
13 focused tests cover independent expected facts, prefixes, interleaved groups, grammar order, failed parses, and native lifetime.
The original collector passes all 11 expected-output cases and fails the new duplicate-group lifetime check.
200 seeded differential cases match the original; retained-root and retained-descendant negative controls fail as intended.
The native lifetime assertions intentionally depend on CPython/tree-sitter ownership and skip non-CPython runtimes.
Ruff, all five generated-artifact checks, and
graphify update .pass locally.These are library fact/graph extraction timings, not full CLI or all-language measurements.
Graph RSS excludes workers and is measured before result serialization.
Filesystem caches are not flushed and desktop load is uncontrolled; timing ranges vary substantially and do not establish universal speedups.