Replace per-crate trait impl queries with merged foreign impl indexes - #107
Draft
xmakro wants to merge 1 commit into
Draft
Replace per-crate trait impl queries with merged foreign impl indexes#107xmakro wants to merge 1 commit into
xmakro wants to merge 1 commit into
Conversation
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.
Prototype: replace the per-crate trait impl queries with two merged foreign impl indexes.
Currently every
CrateMetadataeagerly decodes its trait impl index and incoherent impl index at crate load time, andtrait_impls_ofthen issues oneimplementations_of_traitquery per loaded crate for every foreign trait it is asked about. Each of those calls also forces acrate_hashread for dependency tracking. On a leaf check of the cargo benchmark (301 loaded crates) that comes out to 63210implementations_of_traitexecutions (226 foreign traits times roughly 280 crates), about 161ms inside the providers plus untimed query machinery, and 63k dep graph nodes with edges, about 5 percent of the whole graph.This change:
trait_implsandincoherent_implsdecoding fromCrateMetadata::new.foreign_trait_impls_index, a single query that walks all crates once and records, per trait, where each crate's impl list lives in the metadata blob (crate, position, length). The impl lists themselves are not decoded. Dependency tracking mirrors the extern provider macro: the provider readscrates(())andcrate_hash(cnum)for every crate, so it re-executes exactly when a dependency changes and is green on unchanged incremental builds.foreign_implementations_of_trait(trait_id), which decodes just that trait's impl lists via the index.trait_impls_ofnow makes one query per trait instead of one per crate and trait.foreign_incoherent_impls(fully decoded, the data is tiny).implementations_of_traitandcrate_incoherent_implsqueries.Defaultsuggestion in resolver diagnostics, now scans the direct extern crates through the cstore without going through the query system, because forcingcrates(())during resolution would freeze the crate store while crates can still be loaded (behavior preserved, verified by the existing suggestion tests).Impl ordering is preserved everywhere (crate loading order, encoding order within a crate), so query results are bit identical to before.
An earlier version of this decoded all impl lists eagerly inside the merged query. That won on dependency-heavy crates but regressed small crates badly (helloworld check full +9 percent, incr-full +19 percent) because it decoded every sysroot impl list up front. The index/list split removes those regressions while keeping the wins.
Perf (local dist-fidelity harness: stage2, thin LTO, jemalloc, from-scratch builds on both sides, instructions:u, full primary+secondary suite, 551 comparable cells):
Geomean by profile, all scenarios: check -0.135%, debug -0.098%, opt -0.069%. Every profile/scenario geomean is negative or flat; check incr-unchanged -0.31% (n=45), check incr-patched println -0.28% (n=22).
Dependency-heavy benchmarks:
Worst remaining regressions are confined to two secondary stress benchmarks (coercions incr-full +0.63%, helloworld isolated cells up to +0.39%). Self-profile shows the new queries execute in about 110us total on coercions with 33us of result hashing, so those cells are codegen layout noise rather than added work.
Correctness: x check clean for compiler, rustdoc and clippy; tests/ui/coherence, tests/ui/traits and the Default suggestion tests pass (1731 tests). The first draft deadlocked in resolver diagnostics by freezing the cstore mid-resolution, which is what motivated the untracked path described above.