Repository navigation
Conversation
…ols-only` mode. NFC Previously, `getTransitiveDeps` was called inside `addFromLibrary` while `symbolHandler` was still iterating through `symbolsNeeded` and mutating `__deps` (such as adding `setTempRet0` for i64-returning functions without `WASM_BIGINT`). Compute `symbolDeps` in the `if (symbolsOnly)` block after `symbolHandler` completes so that all direct `__deps` are populated before traversing the dependency graph. Split out from emscripten-core#27995.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a module-level cache (transitiveDepsCache) in src/jsifier.mjs to optimize the getTransitiveDeps function and avoid quadratic behavior. The review feedback highlights two key areas for improvement: first, to prevent cross-compilation contamination in multi-run environments (like test suites), the global cache should be cleared at the start of runJSify; second, the logic for retrieving and filtering dependencies can be simplified and made more idiomatic using nullish coalescing.
| return false; | ||
| } | ||
|
|
||
| const transitiveDepsCache = new Map(); |
There was a problem hiding this comment.
Since runJSify can be called multiple times within the same Node.js process (for example, in test suites or in-process build pipelines), a module-level global Map like transitiveDepsCache will persist across different compilations. This can lead to stale cache entries and incorrect dependency resolution if different compilations use different libraries or settings.
To prevent this cross-compilation contamination, transitiveDepsCache should be cleared at the start of runJSify:
export async function runJSify(outputFile, symbolsOnly) {
transitiveDepsCache.clear();
...| const cached = transitiveDepsCache.get(sym); | ||
| let deps = cached; | ||
| if (!deps) { | ||
| deps = LibraryManager.library[sym + '__deps'] ?? []; | ||
| deps = deps.filter((d) => typeof d === 'string'); | ||
| } |
There was a problem hiding this comment.
This block can be simplified and made more idiomatic by using the nullish coalescing operator (??) and declaring deps as a const.
const cached = transitiveDepsCache.get(sym);
const deps = cached ?? (LibraryManager.library[sym + '__deps'] ?? [])
.filter((d) => typeof d === 'string');
|
The benefit seems to be just a few ms here - is it worthwhile? |
|
5% seems like a big enough speedup to warrant landing to me. |
|
5% of jsifier time? or total link time? |
The total link time for hello world is dominated by 3 things:
Each of these in my machine is around ~150. The cost of actually running wasm-ld is marginal. So this is really 5% of 33% of the total. And aggressively cache the result of |
Cache the computed transitive dependency list for each symbol in `transitiveDepsCache` and reuse cached results when traversing dependencies in subsequent `getTransitiveDeps` calls. Only completed top-level traversals are inserted into the cache so that cyclic dependencies (such as `$FS` <-> `$MEMFS`) are handled safely without caching partial results. In `tools/compiler.mjs --symbols-only`, this reduces graph node visits in `getTransitiveDeps` by ~9x (from 39,349 to 4,381), cutting the `symbolDeps` calculation time by ~41% (from ~16.0ms to ~9.5ms) and overall `--symbols-only` execution time by ~4.6% (from ~152ms to ~145ms). See: emscripten-core#28052
749186b to
8546616
Compare
|
Yeah, if it's 1/3 of the total, then 1/3 of 5% is 1.67% which is pretty small - I'd prefer code simplicity over such a small win. |
|
Change |
Cache the computed transitive dependency list for each symbol in
transitiveDepsCacheand reuse cached results when traversing dependencies in subsequentgetTransitiveDepscalls. Only completed top-level traversals are inserted into the cache so that cyclic dependencies (such as$FS<->$MEMFS) are handled safely without caching partial results.In
tools/compiler.mjs --symbols-only, this reduces graph node visits ingetTransitiveDepsby ~9x (from 39,349 to 4,381), cutting thesymbolDepscalculation time by ~41% (from ~16.0ms to ~9.5ms) and overall--symbols-onlyexecution time by ~4.6% (from ~152ms to ~145ms).See: #28052