Repository navigation
fix(wasm): reserve enough analysis space for dense exports - #134
Conversation
guybedford
left a comment
guybedford
left a comment
There was a problem hiding this comment.
Thanks, the analysis is right: Slice is 12 bytes and shorthand literal exports (a,) can produce one every two code units, so 8 * len is exactly the worst-case bound, and the tests reproduce the trap on main.
Rather than scaling the fixed reservation though, I'd prefer to bound-check in the allocator and grow on demand, which is what es-module-lexer does now: sa() snapshots analysis_limit = __builtin_wasm_memory_size(0) << 16, and each _add* calls an inline ensureAnalysisCapacity(sizeof(Slice)) that falls into a noinline growAnalysis doing __builtin_wasm_memory_grow when analysis_head + size > analysis_limit. The JS side can then keep len * 4 (or lower) as a floor. Wasm memory never shrinks, so with 8 * len every parse of a large bundle permanently pins 8x its size for a case that almost never occurs; growing on demand keeps retained memory proportional to actual results, at the cost of one compare per recorded export. The JS wrapper only touches wasm.memory.buffer before parseCJS, so in-wasm growth detaching the buffer is safe.
This needs a wasm rebuild, so happy to help with that if useful. The tests here are good as the regression either way.
f45d1a0 to
07b764e
Compare
guybedford
left a comment
guybedford
left a comment
There was a problem hiding this comment.
Thanks, this looks good to me. Verified locally:
- Confirmed the bug on main: a fresh instance parsing
module.exports={+'v,'.repeat(2047)+v};throwsRuntimeError: memory access out of bounds. The new tests fail 12 cases against main and pass here in all three modes. - Both this PR's and main's committed
lib/lexer.wasmare byte-identical to a rebuild with the pinned docker toolchain. - Since the JS now reserves source only, the terminator can land in the last two bytes of memory. I probed ~13,800 inputs placing every keyword prefix the lexer matches at that boundary and saw no traps, which checks out since
str_eqNshort-circuits at the terminator. - Bench is within noise versus main.
Growing on demand from Wasm rather than over-reserving the worst case (8 bytes per code unit) in JS is the right trade-off here, since analysis is tiny for real files.
One adjacent pre-existing issue noted inline that this PR may want to fold in since it is already touching the reservation line, otherwise happy for it to be a follow-up.
Dense object exports can trap in WebAssembly because the host reserves four bytes per UTF-16 code unit, below the result-list requirement. A result node needs 12 bytes and can occur every two code units. Reserve eight bytes per unit, including the source. This doubles the input-based capacity allowance. Application measurements show no consistent CPU regression. Signed-off-by: Ruben Bridgewater <[email protected]>
Dense shorthand exports produce a 12-byte record for every two code units, so a source-sized analysis allowance can overflow. Reserving for that worst case retains excess memory on sparse inputs. A 1,048,596-code-unit sparse input retains 4,259,840 bytes instead of 8,454,144 bytes. Dense shorthand parses cost 2.18-3.99% more time (Node 22.23.3, V8 12.4.254.21-node.57, seven alternating trials, trimmed means, two fresh processes).
The proportional analysis allowance wastes linear memory on ordinary modules with few exports. Parsing a 296-file Mocha, Babel, and Terser corpus retains 2.19 MiB of Wasm linear memory instead of 4.25 MiB.
A shorter parse can report phantom exports because the source allocator receives a length that already includes the terminator. It writes the terminator one code unit too far, leaving a previous assignment token at the actual end. Signed-off-by: Ruben Bridgewater <[email protected]>
df28812 to
52c7c54
Compare
guybedford
left a comment
guybedford
left a comment
There was a problem hiding this comment.
Thanks, the terminator fix is folded in cleanly. Re-verified locally on the rebased branch:
- Committed
lib/lexer.wasmis byte-identical to a fresh rebuild with the pinned docker toolchain. - Full suite passes in all three modes. Against main's
src/lexer.jsandlib/lexer.wasm, 13 of the new tests fail, including the new source termination test, so it does catch the phantom-export bug. parse('exports.a=1;'); parse('exports.a')now returns no exports on the second call.
Keeping the 64-bit head/limit is fine given the measurement.
Dense shorthand exports can exhaust analysis capacity because a 12-byte record can occur every two code units. Grow Wasm memory before writing beyond capacity.
A 296-file Mocha, Babel, and Terser corpus retains 2.19 MiB of Wasm linear memory.