Run wasm-emscripten-finalize only when needed, and add ERROR_ON_WASM_CHANGES_AFTER_LINK option #12173
Conversation
There was a problem hiding this comment.
Looks pretty good overall.
I had one other idea; what does emscripten do for O1 builds? Might it make sense to just run LLVM's O1 but also skip binaryen optimizations in that case, given that the speed payoff isn't huge? It would then become pretty useful for debugging, as LLVM's O1 does decently well at getting low-hanging fruit. IIUC some of our existing users with large codebases are using O1 for debugging already since O0 is just too big.
| write_source_map = shared.Settings.DEBUG_LEVEL >= 4 | ||
| if write_source_map: | ||
| building.emit_wasm_source_map(base_wasm, base_wasm + '.map') | ||
| building.save_intermediate(base_wasm + '.map', 'base_wasm.map') | ||
| args += ['--output-source-map-url=' + shared.Settings.SOURCE_MAP_BASE + os.path.basename(shared.Settings.WASM_BINARY_FILE) + '.map'] | ||
|
|
||
| modify_wasm = True |
There was a problem hiding this comment.
why does source map support modify the wasm?
There was a problem hiding this comment.
--output-source-map-url modifies the wasm by writing the URL into it.
There was a problem hiding this comment.
Hmm .. that could be trivial transform done in python perhaps? (you could add it to this new file: #12177)
There was a problem hiding this comment.
Maybe. But as source maps are not recommended and we'll remove support for them once DWARF is stable, I think it's not worth any work on them?
There was a problem hiding this comment.
yeah I had the same idea (I was going to suggest using objcopy) but I agree that it might not be worth it. Although objcopy might be easier than python.
| outfile=wasm, | ||
| args=args, | ||
| stdout=subprocess.PIPE) | ||
| stdout = building.run_binaryen_command( |
There was a problem hiding this comment.
So the reason we are still running it, is to get the metadata?
There was a problem hiding this comment.
Yes, we always need the metadata (to know which JS library functions to call, etc.).
|
@dschuff Interesting idea about |
|
Yes, definitely a separate change. I didn't mean to hold this PR back, it was just a drive-by idea. |
Suggested in #12173 (review) This appears to regress code size by around 5-10% on average for -O1. Given that this mode is kind of a "compromise" build anyhow, with most opts in return for fast iteration, it seems like a better tradeoff to lose that 5-10% in return for much faster link times - almost 2x faster - and also avoiding DWARF rewriting.
emscripten.pynow tracks whether we actually need to runwasm-emscripten-finalize.Currently we don't need to in a specific subset of
-O0builds, in which we don'tneed legalization (by using BigInt support) and we don't need invokes (which means
we don't use longjmp or C++ exceptions; this restriction will be lifted once
WebAssembly/binaryen#3081 is fixed).
This new option, proposed by @bmeurer , will show an error if any binaryen work
is done after wasm-ld. That is, it ensures that we do not run
wasm-emscripten-finalizeor
wasm-opt. That is useful to verify that the link is as fast as possible and also thatit does not rewrite DWARF, which is necessary to support split DWARF (binaryen can
rewrite normal DWARF, but it's simpler and better to support split DWARF by just
not doing any binaryen work after wasm-ld).
See WebAssembly/binaryen#3043