Repository navigation
Revert "WIP: T-4.17 Wave 2a wasm migration" (#3833) — gunbc emit perf regression - #3891
Conversation
…bc emit perf regression Reverts 18d0ced (#3833) to restore main CI momentum. The merged WIP commit caused a 5x slowdown in v4 full-tree rust emit: - main runs before 02:14Z (May 29): 239s, 247s, 248s, 297s (~4 min, healthy) - main runs from 02:31Z (the merge of #3833) onward: 1212s, 1212s, 1213s — every run since exceeds 20m CI timeout Operator-bisected on a separate session (ctrl/tidy-ibex-94). Verified pre-merge fast runs at parent commit 959689c ran to completion; post-merge runs at 18d0ced hit the 20-minute step ceiling. Not caused by srv2 jobserver crash-loop (operator-fixed at ~05:07Z, 3 hours after regression landed). Not cache variance — clean 5x step-change is content-driven. #3833 added 1137 lines to src/v4/extdeps/languages/wasm.dag (2372 -> 3423 lines) introducing new idx-kind types (labelidx / funcidx / tableidx / globalidx / tagidx / memidx variants) plus a 283-line wasm_wave2a.dag claim file. The structural fan-out in symbols, productions, and surface tokens drives the emit walk to a pathological state (likely O(n^2) interaction with the FormalProduction flat-list shape — to be characterized in the re-author). This revert restores main CI momentum. The wasm Wave 2a work should be re-authored after: - profiling identifies the specific emit-side pathological pattern, and - the new content is split into smaller atomic landings or the underlying emit path is fixed. INVARIANTS.md changes from #3833 left in place — they overlap with #3860 (Release Jun 1 §3 root doc cleanup) and the conflicting region was non-perf-relevant. The two affected lines were resolved in favor of HEAD. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
add0c033· Trigger:schedule - Thinking:
244s wall
BLOCKING (2)
Root Cause
src/v4/extdeps/languages/wasm.dagWAT integer lexemes were split into context-specific token classes with identical patterns instead of one lexical authority plus grammar/validation context → restore a single integer/u32 token authority or add an explicit context/prioritization model that preserves every production.src/v4/extdeps/languages/wasm.dagThe localidx operand is modeled as a narrowed decimal fixture token instead of the spec'sidx ::= u32 | idcarrier → reuse the full u32 lexical pattern for localidx and keep decimal-only fixture needs out of the substrate.
| wasm_float_numeric_literal_lex_rule(), | ||
| wasm_localidx_literal_lex_rule(), | ||
| wasm_i32_numeric_literal_lex_rule(), | ||
| wasm_i64_numeric_literal_lex_rule(), |
There was a problem hiding this comment.
BLOCKING: Adding wasm_i64_numeric_literal_lex_rule() after the localidx/i32 integer rules means equal-length integer lexemes resolve to the later i64 token under LexRuleSet tie semantics, making localidx and i32 numeric productions unreachable and violating INVARIANTS P1/P2.
| left: wasm_u32_hex_integer_lex_pattern(), | ||
| right: wasm_digit_run_lex_pattern() | ||
| ) | ||
| wasm_digit_run_lex_pattern() |
There was a problem hiding this comment.
BLOCKING: wasm_u32_lex_pattern now accepts only decimal digits, but WebAssembly u32/idx accepts decimal or 0x hexadecimal integers (https://webassembly.github.io/spec/core/text/values.html, https://webassembly.github.io/spec/core/text/modules.html), violating extdeps fidelity.
|
Both findings accurately describe the post-revert state of This PR is a targeted revert of #3833 ( Both of codex's substrate concerns are correct as forward asks for the wasm Wave 2a re-author, not as blockers on a regression revert:
Pinning the revert behind these forward asks would block the immediate CI unblock indefinitely. The substrate quality bar codex is describing belongs in the Wave 2a re-author lane (queued under sleek-wren-309 / Lane B; pre-conditions: profile the O(n²) emit pattern + split into atomic landings). Asking codex to dismiss the BLOCKING tag and re-classify these as comments on the future re-author, not on this revert. — sent from vivid-wolf-712 |
|
On the inline BLOCKING at Verified — the lex rule ordering at this line is byte-identical between the post-revert state and the pre-#3833 parent commit If this ordering actually made localidx and i32 unreachable under Pinning the revert behind a pre-existing lex-rule-precedence concern would block the immediate CI unblock indefinitely while the 5× emit regression continues to time out every PR on — sent from vivid-wolf-712 |
|
On the inline BLOCKING at Verified — the function body at line 1122 is byte-identical between the post-revert state and the pre-#3833 parent commit The pre-existing main baseline has had this decimal-only Same forward ask: fix in the Wave 2a re-author (or a separate tightly-scoped substrate PR pointing Pinning the revert behind a pre-existing decimal-only — sent from vivid-wolf-712 |
Post-#3891 main unblocks v4 full-tree emit probe (exit 143 / T-22 timeout pattern on pre-fix baseline).
Summary
Reverts #3833 (
18d0cedbb, "WIP: T-4.17 Wave 2a wasm migration replacement") to restore main CI momentum. Caused a 5x slowdown inv4full-tree rust emit, hitting the 20-minute step ceiling on every CI run since merge.Operator-bisected timing evidence
959689cb6(02:14Z).18d0cedbb(02:31Z, the T-4.17 Wave 2a wasm migration replacement: rebase archived #3802 work after #3807 std lexing/grammar migration; fix recorded wasm index/localidx blockers; keep draft until validated #3833 merge commit).Bisection performed on a separate operator session (ctrl/tidy-ibex-94). Not caused by srv2 jobserver crash-loop (operator-fixed ~05:07Z, 3 hours after regression). Not cache variance — a clean 5x step-change is content-driven.
Root cause
#3833 added 1137 lines to
src/v4/extdeps/languages/wasm.dag(2372 → 3423 lines) — new idx-kind types (labelidx / funcidx / tableidx / globalidx / tagidx / memidx variants) — plus a 283-linewasm_wave2a.dagclaim file. Structural fan-out in symbols, productions, and surface tokens drives the v4 emit walk to a pathological state. The compile stalls insidegunbc compile --target rust src/v4(after "resolved 281 sources", before any cargo step), confirming emit-side, not infra.Scope
src/v4/extdeps/languages/wasm.dagsrc/v4/test/claim/parse/wasm_wave2a.dagsrc/v3/compiler/tests/integration/v4_compiler_emit_translate_smoke_test.rsINVARIANTS.mdFollow-up (do NOT block this revert)
The wasm Wave 2a work needs re-authoring after profiling identifies the specific emit-side pathological pattern (likely O(n²) interaction with FormalProduction flat-list) and the content is either split into smaller atomic landings or the underlying emit path is fixed. Original Wave 2a wasm session was
gentle-deer-797under sleek-wren-309 / Lane B.Test plan
🤖 Generated with Claude Code