fix(configlink): order candidates canonically before the collector cap - #1470
Open
Studnicky wants to merge 1 commit into
Open
fix(configlink): order candidates canonically before the collector cap#1470Studnicky wants to merge 1 commit into
Studnicky wants to merge 1 commit into
Conversation
collect_config_entries and collect_code_entries fill fixed-capacity arrays and stop at max_out. They walk gbuf label indexes in insertion order, which under parallel extraction is worker-merge order and varies run to run, so on any repository with more candidates than the cap the surviving set — and therefore the emitted CONFIGURES edges — depended on thread scheduling. Sort candidates by a pure content key (qualified_name, file_path, start_line, name) before the cap applies, so the surviving set is a function of the inputs alone. Tie-breaks are content-only: node ids are handed out in merge order and belong in no canonical comparator. Also log configlinker.truncated when a collector fills to capacity, so the cap reports itself instead of dropping candidates silently. Measured on a 4191-file TypeScript corpus (12940 candidates against the 8192 code cap): before, 3-61 CONFIGURES edges differed per run against a single-threaded reference and no two multi-threaded runs agreed; after, the single-threaded reference and four multi-threaded runs are byte-identical. Corpora below the cap were already stable, and all divergence was CONFIGURES. Signed-off-by: Andrew Studnicky <a.j.studnicky@gmail.com>
|
Thanks for opening this — it has been seen, and it is queued. This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence. Current review status: working through a backlog. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
If this fixes a bug, a reproduction we can run is worth more than a description of the symptom. Thanks for contributing, and sorry in advance for the wait. |
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.
Fixes #1468
What does this PR do?
collect_config_entriesandcollect_code_entriesfill fixed-capacity arrays and stop atmax_out, walking gbuf label indexes in insertion order — which under parallel extraction is worker-merge order and varies run to run. On a repository with more candidates than the cap, the surviving set, and therefore the emittedCONFIGURESedges, depended on thread scheduling.This sorts candidates by a pure content key (
qualified_name,file_path,start_line,name) before the cap applies. Tie-breaks are content-only — node ids are handed out in merge order and belong in no canonical comparator.It also logs
configlinker.truncatedwhen a collector fills to capacity, so the cap reports itself rather than dropping candidates silently.Why
Indexing the same tree twice produced a different
CONFIGURESedge set each time, and a different set from the single-threaded reference — which breaks the deterministic-output property established in #921, makes A/B comparisons noisy, and churns incremental artifacts.This is the recipe #998 called for ("sort candidates by (score, qn) instead of arrival order"), applied to the pass that did not get it.
Testing
4191-file TypeScript corpus,
--mode moderate, freshCBM_CACHE_DIRper run, comparing sorted(source_qn, type, target_qn)triples:CONFIGURES— 86 edges across 3 runs, of 414 total.CONFIGURESedges are byte-identical before and after (70393), so the blast radius is contained to this pass.New test
configlink_candidate_truncation_is_order_independentbuilds a corpus exceeding the code-entry cap and runs the pass over it twice, inserting candidates forwards and then backwards; the emittedCONFIGURESset must match. It fails on an unpatched tree (7 passed, 1 failed) and passes with the change (8 passed). Theconfiglinker.truncatedlog fires during the test, confirming the cap is genuinely exercised rather than the assertion passing vacuously.scripts/test.sh: 438 passed acrossgraph_buffer configlink simhash semantic parallel pipeline. The full suite matches the pre-change baseline exactly — the two failures (cli,py_lsp_scale) reproduce identically on an unpatched tree.scripts/lint.sh: no new findings.pass_configlink.creports the same 10 pre-existing items as before the change.Risks
This changes which
CONFIGURESedges are produced — 416 → 465 on the test corpus — because a canonical cut necessarily differs from an arbitrary one. That is inherent to any deterministic fix here.The alternative, removing the caps so nothing is truncated, is not a drop-in:
config_entriesandcode_entriesare stack arrays (CBM_SZ_8K× 264 bytes plusCBM_SZ_4K× 520 bytes, ~4.3 MB of frame already), so lifting the bound would need heap allocation and a look at the O(config × code) match loop. Happy to take that direction instead if you would prefer it.Scope is limited to
pass_configlink.c; no other pass, the store format, or the MCP surface is touched.Links
Checklist
git commit -s)