Repository navigation
Don't walk the main module's table in pthread workers - #28105
clementperon wants to merge 4 commits into
Conversation
The first call to getFunctionAddress walks the whole table, which creates a JS function object for every entry. With MAIN_MODULE and pthreads, every worker does this when it instantiates the main module, and keeps those objects for its lifetime: about 8 MiB per worker for a 50k-entry table. The dynamic linker only looks up the main module's exported functions, so the main thread now records their table slots and passes them to workers, which add just those to the map. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request optimizes worker initialization in pthreads builds with MAIN_MODULE by passing only the table slots of the main module's exported functions to workers, rather than having them walk the entire function table. Feedback suggests adding a defensive check in src/lib/libaddfunction.js to ensure getWasmTableEntry(slot) returns a valid object before adding it to the WeakMap, preventing potential runtime crashes in workers.
|
@sbc100 is this something you want to merge upstream ? |
|
Mostly likely yes, I'm just a little review constrained right now. I'll try to get to it. |
|
Thank you, take your time and don't hesitate if you want me to perform more test. |
| // As a consequence, a main-module function that is in the table but not | ||
| // exported is not found here, so addFunction in a worker gives it a new | ||
| // slot rather than the address it has on the main thread (ASSERTIONS=2 | ||
| // reports this in addFunction). |
There was a problem hiding this comment.
If I'm reading this right it sounds like this means that an address might not represent the same code on all threads? I don't think that can ever work since function pointer can be passed via linear memory and must be callable from all threads.
There was a problem hiding this comment.
Agree I can fix it by having workers scan the main module's table slots once, the first time addFunction misses after startup.
4e022bb to
509af0f
Compare
Note in getFunctionAddress and the ChangeLog that a non-exported main-module function passed to addFunction in a worker now gets a new slot. Add an ASSERTIONS=2 variant of test_pthread_dylink, whose addFunction check catches export slots missing from a worker's map. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Workers start with only the main module's exported functions in functionsInTableMap. Previously, addFunction in a worker on a main-module function that is not exported (e.g. one obtained with getWasmTableEntry) missed the lookup and allocated a new, worker-local slot instead of returning the address it has on every thread. Now, when a lookup in a worker misses, getFunctionAddress first adds the rest of the main module's part of the table to the map (once). Lookups from the dynamic linker (updateGOT, reportUndefinedSymbols, dlsym and its catch-up) pass isDylinkSymbol, since they only resolve module exports and JS functions, so worker startup and dlopen still skip the walk. This restores the previous addFunction behaviour, so drop the documented limitation, and test it in test_pthread_dylink. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
509af0f to
12b8416
Compare
The first call to
getFunctionAddresswalks the whole table and keeps a JS function object for every entry. WithMAIN_MODULEand pthreads, every worker makes that call when it instantiates the main module (updateGOT), so each worker holds a wrapper for every address-taken function of the main module for its whole lifetime.The dynamic linker only looks up the main module's exported functions, so the main thread now records their table slots, before
updateGOT, and passes them to workers in theloadmessage next tosharedModules. A worker adds just those slots to its map, plus anything beyond the main module's initial table (side modules, lateraddFunctionslots).Impact
Kodi's WebAssembly port links Kodi as
MAIN_MODULE=2so binary add-ons can bedlopened. Its table has 50,551 slots, of which 102 hold exported functions. Measured in Chrome with the same wasm, applying this change to the Emscripten 6.0.9 glue:Behavior
Lookups in workers return the same slots as before:
addFunctionwrapped is still found through the entryaddFunctionrecords.The difference is a non-exported main-module function passed to
addFunctionin a worker. JS can only reach such a function through the table itself (for example withgetWasmTableEntry). Before, the walk found its existing slot; now the worker gives it a new slot, so it has a different address there than on the main thread. Keeping the old behavior would need the full walk this PR removes. WithASSERTIONS=2,addFunctionalready reports this case ("function in Table but not functionsInTableMap"). The limitation is noted in a comment ingetFunctionAddressand in the ChangeLog entry.The main thread still walks the table once. #27993 creates the main module's GOT entries lazily and doesn't change the walk, so the two changes are independent.
Testing
test_pthread_dylinknow asserts that a side module sees the same&bar_mainas the main module, on the main thread and in a worker. That assertion fails if workers don't add the recorded slots.test_pthread_dylinkalso has a newassertions2variant that builds with-sASSERTIONS=2. In that mode,addFunctionchecks that every function already in the table is also infunctionsInTableMap. With the worker loop over the recorded slots removed, this variant aborts in the worker with that message.Passing: all
test_pthread_dl*,test_pthread_dylink*andtest_pthread_tls_dylinktests incore0,core2andwasm64,other.test_dylink_pthread*, and the dylink codesize tests (unchanged).test_pthread_dylink_assertions2passes incore0,core2andwasm64.🤖 Generated with Claude Code