Repository navigation
fix(wasm): track reexport bindings beyond the static capacity - #136
Conversation
guybedford
left a comment
guybedford
left a comment
There was a problem hiding this comment.
Thanks, the fix is correct. Verified locally: the new test fails against the main wasm (binding 255 is dropped), the committed lib/lexer.wasm reproduces byte-identically via build-wasm-local, and the analysis memory budget holds under stress (100k minimal var a=require("a") declarations at both analysis_base alignments, since the shortest tracked declaration reserves 34 bytes against the 20-byte record).
Main suggestion is structural: with the overflow path in place, the static array is now just a fast path for the first 255 entries at the cost of two storage paths and two lookup loops. Putting all bindings in analysis memory as a single linked list ends up simpler and smaller, see inline comment.
Also worth rebasing on main (#131 landed, no overlap).
73e1781 to
0cf6148
Compare
guybedford
left a comment
guybedford
left a comment
There was a problem hiding this comment.
Verified locally: the new tests fail against the main wasm and pass in all three test modes with this change, including a stress check with 100k separator-less var a=require("") bindings at even and odd source lengths.
Reusing the analysis arena is the right call here. One non-blocking nit inline on documenting the arena bound now that two record types depend on it.
0cf6148 to
5d0ce15
Compare
guybedford
left a comment
guybedford
left a comment
There was a problem hiding this comment.
The binding change is unchanged since my approval and still looks good: rebased cleanly, the committed lib/lexer.wasm rebuilds byte-identically, and all tests pass in the three modes.
The new third commit is the len * 8 reservation from #134's first revision, which #134 has since replaced with growing on demand from sa()/_add*. I'd suggest dropping it here so the memory bound is owned by one PR. Note dense a, exports already trap at len * 4 on main with no bindings at all, so it isn't this PR's bindings that break the budget (they add at most ~1.2 bytes per code unit).
The one thing that does need coordinating is the binding allocation itself, see inline. Simplest order is probably #134 first, then rebase this on top with the capacity check added to the binding path.
5d0ce15 to
fe88243
Compare
|
@guybedford I just addressed the feedback and I believe this should be good to go |
The Wasm parser stops recording bindings when its fixed array fills, so later Babel-style reexports disappear from the result. Overflow bindings use the existing per-source analysis memory, and files within the static capacity retain the existing storage path. Signed-off-by: Ruben Bridgewater <ruben.bridgewater@datadoghq.com>
A minimal declaration reserves 34 analysis bytes for a 20-byte Wasm binding record. This lets the existing source allocation hold bindings without a separate overflow representation. Signed-off-by: Ruben Bridgewater <ruben.bridgewater@datadoghq.com>
Source-only reservation can leave too little analysis memory for a require binding, whose allocation bypasses the capacity check and traps.
guybedford
left a comment
guybedford
left a comment
There was a problem hiding this comment.
The capacity check and fresh-instance tests address the last point. Verified the committed binary rebuilds byte-identically, the full suite passes in all three modes, and the new capacity test traps without the check. Rebasing onto main for the binary conflict from #135 and landing.
fe88243 to
a18c20a
Compare
The Wasm parser stops recording require bindings after 255 declarations, so later Babel-style reexports disappear. Binding records now share per-source analysis memory and reset between parses.