v3: cut the self-host check phase from ~425ms to ~290ms - #27974
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 198cb98230
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // labelled loop control statement. | ||
| fn (tc &TypeChecker) valid_labelled_loop_control(id flat.NodeId, name string) bool { | ||
| mut current := id | ||
| for _ in 0 .. 256 { |
There was a problem hiding this comment.
When a labeled break or continue is nested more than 256 AST-parent edges below its target loop, this fixed bound exits before reaching the loop and incorrectly reports invalid label name, even though the label is lexically valid. The previous source-containment check had no such depth limit; walk until reaching the function/root boundary instead of imposing an arbitrary cap.
Useful? React with 👍 / 👎.
|
Added a second commit: in-place node-source span scans ( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe73686acf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if memo.filled[mi] != 0 { | ||
| return memo.types[mi] |
There was a problem hiding this comment.
When an expression such as return value is Concrete && value.member > 0 is resolved before check_node (for example, check_return resolves the whole infix at raw_child_type), the RHS identifier is memoized with its unspecialized interface or sum type. check_node later installs the Concrete smartcast before checking the RHS, but this early return bypasses smartcast_type and the existing smartcast-aware cache bypass in resolve_type_uncached, so valid field or method access can be rejected as unknown. Avoid memo hits for identifiers/selectors while smartcasts are active, or invalidate those entries when the smartcast context changes.
Useful? React with 👍 / 👎.
|
Follow-up experiments, both measured and one conclusion worth recording: I implemented the per-interface fan-out of |
|
String-key refactor, first tranche (commits
Where this tranche stops: the next candidates (method-existence pre-filters for |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb0a5a3bd8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| tasks << workers.Task{ | ||
| run: check_top_level_decls_thread | ||
| arg: voidptr(tc) | ||
| force_sync: true |
There was a problem hiding this comment.
When parallel checking is enabled for at least 256 functions, this synchronous task runs concurrently with the already-submitted body tasks. check_top_level_declarations() can mutate compilation-wide state—for example, check_const_field_values() assigns tc.const_types[qname] = Type(void_) for a cyclic constant—while each checker fork shares and reads the same const_types map. A large invalid program containing such a constant can therefore produce scheduling-dependent diagnostics or unsafe concurrent map access; perform mutating declaration checks before launching the body workers or give the task private state that is merged afterward.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fb0af2d. The walk is split via a shared filtered traversal: the initializer-value checks (top-level statements, struct field defaults, enum values, const values — everything that can mutate compilation-wide state like the const-cycle poisoning of tc.const_types) now run on the master before any chunk is created or submitted, so their effects are ordered before every body worker exactly as in the serial flow. Only the read-only signature/type-string checks (audited: no shared-table writes; parse/intern goes through the master's private overlay and the node-cache writes follow the existing sparse discipline) remain overlapped with the pool. Serial flows call the combined walk and keep the original interleaved declaration order. Verified: a 300-function program with a cyclic const bad = bad + 1 produces byte-identical diagnostics across repeated parallel runs and against -no-parallel; generated C stays byte-identical and the v4→v5 bootstrap is green. The prepass costs only a few ms serial — the expensive type-string checks are the bulk of the overlapped work, so pool.run is unchanged (~170-190ms).
| if verbose { | ||
| eprintln(' [ttime] ck iface embed ${f64(cvsw.elapsed().microseconds()) / 1000.0:7.2f} ms') | ||
| } |
There was a problem hiding this comment.
Under verbose timing output, the stopwatch is not restarted after this new ck iface embed checkpoint, nor after ck unsup gen. Consequently, ck unsup gen includes interface-embedding time and ck iface idx includes all three phases, so these labels do not report the individual phase costs and can mislead the performance measurements this change introduces.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fb0af2d — the stopwatch now restarts after the ck iface embed and ck unsup gen checkpoints, so each label reports its own phase. With the fix the breakdown reads: embed ~0.04ms, unsup gen ~0.00ms, iface idx ~12.8ms (i.e. the interface-index build is genuinely the whole cost of that step, which the earlier fan-out experiments also concluded).
Review feedback on #27974: check_top_level_declarations ran concurrently with body chunks while check_const_field_values can poison the shared tc.const_types map for cyclic constants, making diagnostics scheduling dependent. The walk is now split: initializer-value checks (top-level statements, struct field defaults, enum values, const values) run on the master before any chunk is submitted, and only the read-only signature and type-string checks overlap the pool. Serial flows keep the original interleaved order through the shared filtered walk. Also restart the verbose stopwatch after the ck iface embed and ck unsup gen checkpoints so each label reports its own phase. Cyclic-const diagnostics verified identical across repeated parallel runs and against -no-parallel; generated C byte-identical; v4->v5 bootstrap green.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb0af2da4c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Alias-cycle and C-redeclaration checks are diagnostics over tables pass 1 | ||
| // completed; check_top_level_declarations runs them off the serial path, | ||
| // concurrently with body checking in the parallel flow. |
There was a problem hiding this comment.
When TypeChecker.collect() processes a recursive alias such as type Recursive = []Recursive, this relocation leaves tc.errors empty until semantic checking. That breaks direct collection callers and the existing test_preflight_errors_do_not_skip_function_semantics, which explicitly asserts that this diagnostic is present immediately after tc.collect(a). Keep the alias-cycle check in the collection phase or otherwise preserve the established collection-time diagnostic.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3f44eb7. The relocation is reverted outright rather than patched around: check_alias_declaration_cycles and the two C-redeclaration checks are back at their original pass-1 position in collect_after_index (the pass-1 timer shows they cost nothing measurable there — the relocation had bought no time), and the copies in check_semantics and the filtered declaration walk are removed so each diagnostic is reported exactly once. test_preflight_errors_do_not_skip_function_semantics passes (both want_parallel modes, including the collection-time assertion and the exactly-one-occurrence assertion); generated C stays byte-identical and the v4→v5 bootstrap is green.
Review feedback on #27974: relocating these checks out of collect() left tc.errors empty until semantic checking, breaking direct collection callers and test_preflight_errors_do_not_skip_function_semantics. Restore them to their original pass-1 position (they cost nothing measurable there) and drop the relocated copies from check_semantics and the filtered declaration walk, so each diagnostic is still reported exactly once.
The fixture-compatibility merge tripled checker CPU. This restores most of it with output-identical optimizations (generated C verified byte-identical against master, v4->v5 bootstrap green): - per-work-item resolve_type memo (BodyResolveMemo): call info, argument checks, and child traversal re-resolved the same subtrees; one function body now resolves each node once. Unknowns are never memoized (cycle guards, provisional generics). - oversubscribed check chunks (4x workers): span costs undercount construct-heavy bodies, so one chunk per worker left the pool idling behind the slowest chunk. - declaration-level checks (type strings, signatures, const/enum values, alias cycles, C redeclarations) moved off the serial path onto the master as a synchronous pool task that overlaps body checking; master runs in sparse-cache mode so its writes stay out of worker-owned ranges. - pointer_diagnostic_binding_type_name: gate on pointer-flavored types, scan bounded to the enclosing function, no whole-file substr copies. - closest_identifier_span: search outward from the anchor instead of scanning the file from the top. - valid_labelled_loop_control/label_starts_loop: ancestor walk over the parent index instead of full-AST scans plus source brace matching. - new shared collect-time indexes replacing per-call linear scans: import alias/suffix paths (current_file_import_path_for_alias, diagnostic_type_name), struct embed receivers (embedded method/field candidates), strict_map_index files, and visible-mutation field lookups through type_declaration_ids.
- node_source_contains/node_source_starts_with replace source_text_for_node(...).contains/starts_with call sites: the substr copied the node's whole span per query. The match-statement typeof probe copied the entire match span once per condition (dispatch-table matches span thousands of lines); it is now one in-place scan per match. - every `&x` prefix expression, unsafe-argument probe, negative-literal probe, and $d(/$( probe scans the file text in place. - explicit_generic_source_param_is_mut uses the naive scanner instead of KMP for its 3-byte needle (runs per call argument). Generated C stays byte-identical; v4->v5 bootstrap green.
check_interface_embedding_limits and set_unsupported_generic_files were billed to the interface-index step; the sub-timers show the step's cost is prepare_interface_query_indexes itself.
- cache the concrete-generic-receiver-specialization flag in FunctionCheckContext: should_diagnose derived it by re-parsing the current function's name (contains + all_before_last substr) on every diagnostic gate. - index fn declarations by short name for expr_raw_fn_type_text, whose ident path scanned every top-level declaration with an all_after_last substr allocation per declaration per query; post-collect declarations fall back to an allocation-free suffix compare. - visible_mutation_fn_lookup_name: skip the receiver substring allocation unless the receiver actually carries generic arguments. Generated C byte-identical; v4->v5 bootstrap green.
smartcast_type and assignment_preserves_smartcast built the (allocating) selector expression key before probing tc.smartcasts; most code runs with no active smartcasts, so the empty-map check skips the key entirely. Generated C byte-identical; v4->v5 bootstrap green.
infix_read_type consults mut_param_base_types for every infix operand; the empty-map gate avoids the string-keyed probe in the common case.
Review feedback on #27974: check_top_level_declarations ran concurrently with body chunks while check_const_field_values can poison the shared tc.const_types map for cyclic constants, making diagnostics scheduling dependent. The walk is now split: initializer-value checks (top-level statements, struct field defaults, enum values, const values) run on the master before any chunk is submitted, and only the read-only signature and type-string checks overlap the pool. Serial flows keep the original interleaved order through the shared filtered walk. Also restart the verbose stopwatch after the ck iface embed and ck unsup gen checkpoints so each label reports its own phase. Cyclic-const diagnostics verified identical across repeated parallel runs and against -no-parallel; generated C byte-identical; v4->v5 bootstrap green.
Review feedback on #27974: relocating these checks out of collect() left tc.errors empty until semantic checking, breaking direct collection callers and test_preflight_errors_do_not_skip_function_semantics. Restore them to their original pass-1 position (they cost nothing measurable there) and drop the relocated copies from check_semantics and the filtered declaration walk, so each diagnostic is still reported exactly once.
3f44eb7 to
46a99ad
Compare
|
Rebased on master (f5851da) — two small conflicts in checker.v resolved (master's new void-subtree early-return in check_prefix_expr kept alongside the in-place Heads-up on a separate master regression found while re-validating the bootstrap chain (this PR reproduces it identically since its output is byte-identical to master): since |
|
Follow-up on the bootstrap regression reported above: root-caused and fixed in #27978. |
1 participant
The fixture-compatibility merge added ~30k LOC of checker logic and roughly tripled check-phase CPU (`check (parallel)` ~129ms → ~425-580ms on the self-host build). This PR restores most of it while keeping the generated C byte-identical to master (verified by compiling the same source with the master-built compiler and this branch's compiler and diffing the emitted C; also validated v1 → v3 → v4 → v5 bootstrap).
Measured on `./v3 -nocache -building-v -o v4 v3.v` (prod, prealloc, quiet machine): ~425ms → ~287ms, with the phase's run-to-run variance dropping sharply. RSS peak during check also fell (~1.7GB → ~1.1GB).
Structural
Hot-path fixes (same happy-path-diagnostics disease as #27973)
New shared collect-time indexes replacing per-call linear scans
Remaining to get under 200ms (follow-up): the serial `ck collect` pass2 (~34ms of signature/field type-text parsing) could pre-warm the parse cache in parallel, and the worker profile is now dominated by diffuse small-string interpolation keys and map traffic rather than any single function.