Repository navigation
Emitter: variant membership reads the key set the carrier already holds (emit phase 253s -> 135s) - #12558
Closed
briansrls wants to merge 1 commit into
Closed
Emitter: variant membership reads the key set the carrier already holds (emit phase 253s -> 135s)#12558briansrls wants to merge 1 commit into
briansrls wants to merge 1 commit into
Conversation
v1.compiler.infer_emit_info is_known_variant rescanned map_values(type_summaries) on every question, rebuilding the set derive_variant_to_enum already stores on EmitGraphInfo as variant_to_enum. Its keys are exactly every variant_name_set key of every EnumRepr summary, and every EmitGraphInfo constructor pairs the two fields from one base, so membership is map_contains_key(variant_to_enum, n). In a profile of the v2.compiler.compile emission at 5bf2b13, that rescan was the largest cost of the emit phase, mostly under import_module_enum_scope. - is_known_variant takes variant_to_enum, so a caller can only hand it the key set the carrier derived. The seven call sites in v1.compiler.emit_rust read it; is_import_graph_type_name, import_module_enum_scope and import_variant_parent_for_name carry it from emit_specific_import_use_lines. - reference_derived_candidate_disposition answers membership and parent with ONE read of variant_to_enum, and its dead type_summaries parameter goes. The arm that was unreachable while membership was a separate scan is gone. - The census witness drops that argument and the two fixtures only it used. - gunbc.emit_summary_map_consumer_partition: is_known_variant no longer reads the summary map, so its consumer row leaves; its guard row stays, re-worded (the key set is still quantified over every enum summary). - gunbc.recurring_failure_mode emit_stage_cost_dominates_its_closure gains the attribution for the v2.compiler.compile closure, scoped to that closure. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
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.
The seed's Rust emitter spent most of its emit phase recomputing a fact it already carries. This is the first of the repairs from a perf profile of that phase.
The defect
v1.compiler.infer_emit_infois_known_variantanswers "does any enum in the closure declare this variant spelling?". It did so by scanningmap_values(type_summaries)on every call.But
derive_variant_to_enumalready builds exactly that key set, from everyvariant_name_setkey of everyEnumReprsummary, and everyEmitGraphInfocarries the result asvariant_to_enum.build_emit_graph_infoderives it from the samebuilt.type_summaries, and every other constructor copies the two fields from one base (all eleven.dagconstructors were checked).So each call rebuilt a set its predecessor had already stored. The link broke its own contract, which makes it the earliest unjustified boundary in the chain (DESIGN §6b). The repair is local to it.
Measured. The profile was perf at 199 Hz with frame pointers, attached to the seed for exactly the
compile.emitphase ofgunbc compile --source-root dag --source-root src/v2 --entry src/v2/compiler/00_compile.dag --target rust. At5bf2b13fb8e(current main, which already includes #12454),is_known_variantaccounted for 48% of emit-phase samples inclusive, mostly underimport_module_enum_scope. Rendering (emit_typed_item) was 14%.Change
is_known_variant(variant_to_enum, name)=map_contains_key(variant_to_enum, name). The parameter is the derived map, so a caller can only hand it the key set the carrier built.v1.compiler.emit_rustcall sites. The seven call sites readvariant_to_enum:emit_specific_import_use_linesbinds it once and passes it tois_import_graph_type_name,import_module_enum_scopeandimport_variant_parent_for_name;reference_derived_use_line_plananddiscriminant_zero_field_variant_tagread it fromemit_info.reference_derived_candidate_dispositionreadsvariant_to_enumonce, for both membership and parent. The oldAbsent → CandidateVariantParentUnresolvedarm was unreachable while membership was a separate scan. Now the shape cannot express it. The function's now-deadtype_summariesparameter is removed.v1.tests.claim.reference_derived_disposition_census_witness_testdrops that argument, the two fixtures only it used, and their import.gunbc.emit_summary_map_consumer_partition.is_known_variantno longer reads the summary map, so its consumer row is removed. Its guard row stays and is re-worded: the key set is still quantified over every enum summary, so the "answers true more often as the input degrades" finding still holds.gunbc.recurring_failure_modeemit_stage_cost_dominates_its_closuregains the attribution for thev2.compiler.compileclosure. It is scoped to that closure; the 547-module specimen's trigger stays open.v1_compiler_infer_emit_info.rs,v1_compiler_emit_rust.rs, and the census witness mirror.Controls
Byte identity. Base seed (main) and new seed each emitted
00_compile.dag3 times, as concurrent pairs, from one fixed snapshot of this commit. All 6 output trees are identical: 214 files, 0 differences, and no base-vs-base flip.5bf2b13fb8e)The profile-based estimate for this change was about 135 s. The stage0 regen round, whose seed emission runs the same predicate, fell from 618 s to 539 s.
claim_executor --required-regen)first_generation_equal=true, 161/161--required-regen-fixed-pointfixed_point_equal=trueat3b5fdf6d855cargo clippy --all-targets -- -D warningsgunbc run --source-root src/v1 --source-root dag --entry src/v1/tests/claim/reference_derived_disposition_census_witness_test.dag --claim-run)E, an ambiguous""is unresolved, type position goes to the registry, a non-variant is registry-absent. See the finding below for the three failurestools.seed_growth_change_population mainFinding, pre-existing and not fixed here. Three rows of that census witness fail identically on main and on this PR:
candidate_provided_by_this_module_is_own_module,cross_module_candidate_without_export_proof_is_export_proof_failedandcross_module_candidate_with_export_proof_survives. The pass/fail sets are equal. The cause is thatlookup_item_by_leafnow resolves throughemit_info.item_leaf_owner_modules, while those fixtures passempty_emit_graph_info(), so every registry lookup answersItemNotFound. The file's own note records that no CI step runs it, which is how the rot went unseen.Not in this PR
These are the next steps from the same derivation, each its own change with the same controls:
struct_candidates_by_field_names, which still scans every summary per record literal (16% of emit on main);find_unique_struct_name_by_fieldsfallbacks;emit_specific_import_use_lines;import_module_enum_scopeonce per provider rather than per import block, gated on a differential;unique_variant_parentreadingvariant_to_enum, which comes after Seed emitters render each child once: remove six 2^depth double renders #12448, because it edits lines beside Seed emitters render each child once: remove six 2^depth double renders #12448's hunk.This change does not advance the trigger of
an_authored_import_emits_no_use_line_when_its_spelling_is_forked. The predicate is still consulted in import planning, just in O(1) now.Contention. #12448 and #12401 also regenerate
v1_compiler_emit_rust.rs. There is no textual overlap in the.dag, but whichever lands second regenerates the mirror.🤖 Generated with Claude Code