Skip to content

perf: release JS parse trees between symbol-fact collection files - #3353

Open
VasuBansal7576 wants to merge 3 commits into
Graphify-Labs:v8from
VasuBansal7576:codex/stream-js-symbol-facts
Open

perf: release JS parse trees between symbol-fact collection files#3353
VasuBansal7576 wants to merge 3 commits into
Graphify-Labs:v8from
VasuBansal7576:codex/stream-js-symbol-facts

Conversation

@VasuBansal7576

@VasuBansal7576 VasuBansal7576 commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

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 937e59a versus fixed head ac3449f, 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.

Repository Files Workload Time, before → after Peak RSS, before → after
Next.js 23,909 Facts 50.73s → 35.21s 3,886 → 380 MiB
Next.js 23,909 Graph 164.61s → 133.36s 3,032 → 761 MiB
webpack 15,273 Facts 9.60s → 5.33s 1,088 → 153 MiB
webpack 15,273 Graph 32.97s → 30.38s 1,160 → 267 MiB

A separate three-pair comparison against previous PR head c2a8667 isolates 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 4fed8ea and webpack 92c4650.
Next.js includes five duplicate-path groups, such as hmr-symlink/app/symlink-chain/page.tsxsymlink-link/page.tsxsymlink-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.json

Use --mode facts for collection or change --source to 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.

@graphify-labs graphify-labs Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 batchgraphify/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).

Comment thread graphify/extractors/resolution.py Outdated

@graphify-labs graphify-labs Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Comment thread graphify/extractors/resolution.py Outdated
Comment thread tests/test_js_fact_collection.py Outdated

@graphify-labs graphify-labs Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 occurrencesgraphify/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 treegraphify/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_importsgraphify/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 inconsistentlygraphify/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 datagraphify/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).

Comment thread graphify/extractors/resolution.py
Comment thread tests/test_js_fact_collection.py
Comment thread tests/test_js_fact_collection.py
@VasuBansal7576

Copy link
Copy Markdown
Contributor Author

Checked all five semantic advisories in the latest bot review against the original collector at 937e59a and the tests on ac3449f:

  1. Path attribution: each occurrence has a distinct result container and is emitted using its own path. Original occurrence indices restore category order across [A, B, alias-of-A]. test_interleaved_groups_preserve_every_fact_category verifies paths and ordering across two interleaved groups in both grammar orders.
  2. Last successful tree for exports/calls/class uses: this is the pre-existing contract. The original collector stored trees by path.resolve() and reused the last successful parse in its later passes. The new helper preserves that behavior within one resolved-file group.
  3. module_imports: JS collection never produces this Python-specific category. The existing facts.module_imports list is untouched; the explicit prefix test seeds it and verifies preservation. No generated JS facts are discarded.
  4. Failed occurrences: a failed parse contributes no first-pass facts, but later passes can still use the group's last successful tree, exactly as before. The explicit first-failed, last-failed, and all-failed cases match the original collector. Per-occurrence slots remain present, so zip does not shift later facts.
  5. First-pass versus second-pass export behavior: declarations/imports/aliases use each occurrence's parse, while exports use the last successful group parse. The mixed-grammar fixtures freeze this deliberate distinction from the original implementation.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant