github-actions[bot] commented on Issue #2452:
Subscribe to Label Action
cc @peterhuene
<details>
This issue or pull request has been labeled: "wasmtime:api"Thus the following users have been cc'd because of the following labels:
- peterhuene: wasmtime:api
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
fitzgen commented on Issue #2452:
Can we test on some of whitequark's yowasp stuff?
Some instructions here: https://github.com/YosysHQ/yosys/pull/1483
alexcrichton commented on Issue #2452:
Currently the memory overhead is the size of the dwarf debug information itself, we copy that from the original wasm blob into a side table which is then connected to
CompilationArtifacts
. That's then copied again to have an independent version living inStore
, but this second copy could be removed with anArc
pretty easily (just needs some serde-fu). This is also all pretty easy to turn off, and I currently haven't added a knob to turn it off at all (and opted to not connect it todebug_info
just yet because that's off-by-default).It looks like I'd have to compile the yosys blob from source, I'll see if I can play around with a few things here and there.
alexcrichton commented on Issue #2452:
Ok so fiddling with yosys module compilation, looks like yosys is about a 10MB wasm file but has 270MB (ish) of debuginfo. Loading the wasm file (with debuginfo) and retaining it as expected takes about 200MB more of resident memory. Instantiation would then take another 200 more until the module itself is dropped, freeing one of the blocks.
In the spirit of having knobs as necessary I went ahead and added a
Config
option to disable retaining parsed debuginfo. I left it as default-true
, however, since I suspect that this is useful enough we'll want it most of the time.
tschneidereit commented on Issue #2452:
I'm a bit concerned about adding that much potential memory overhead by default :/ I assume it's not possible to instead use pointers into the original wasm file that we then resolve only when needed?
Assuming so, could we instead disable this by default, but print out instructions for how to enable it, similar to the "note: run with
RUST_BACKTRACE=1
environment variable to display a backtrace" line rustc compiled code prints?
alexcrichton commented on Issue #2452:
I don't think it's easy to optimize this too much more (apart from the one copy from the module to the store which we could elide with more effort), because unlike with native debuginfo situations we aren't guaranteed to have a disk to store all the pesky debuginfo to read later.
I think that it should be possible to print out a message, but I'm not sure what it would say. Enabling debuginfo parsing is likely embedding-dependent and would have a different flag on the command line, for example, than it would if it were embedded into a different application. I also think there's some good value from newcomers coming to wasmtime and automatically seeing filenames/line numbers in backtraces without having to do anything. There's always a knob to turn it off it it becomes a pressing isssue, but having a good new-user experience I think is pretty valuable.
fitzgen commented on Issue #2452:
We can make
Module::from_file
remember the file path and lazily re-read the debug info (we could even remember the debug info section offsets in the file to avoid parsing the Wasm again in addition to parsing DWARF).In general, using
from_file
is nice since we can mmap other things directly from the wasm file at a later date without keeping it eagerly in memory (thinking about the data segment lazy + CoW stuff, which would require creating temporary in-memory file descriptors viamemfd
if we didn't useModule::from_file
).
alexcrichton commented on Issue #2452:
I'm hesitant to get too fancy with the optimizations just yet without a motivating use case, but if you'd prefer I can just turn this off-by-default and do something like hook it up to the CLI flag and call it a day.
tschneidereit commented on Issue #2452:
I think that it should be possible to print out a message, but I'm not sure what it would say.
I guess I'm thinking about something along the lines of
Caused by: 0: failed to invoke command default 1: wasm trap: unreachable wasm backtrace (note: run with `WASM_BACKTRACE_DETAILS=1` environment variable to display a richer backtrace for wasm frames) 0: 0x6c1c - panic_abort::__rust_start_panic::abort::h2d60298621b1ccbf ...
tschneidereit commented on Issue #2452:
There's always a knob to turn it off it it becomes a pressing isssue, but having a good new-user experience I think is pretty valuable.
I should emphasize that I very much agree with this, yes! Unfortunately this seems like a situation where we have to trade off two dimensions of exactly this. Having the default be to increase memory usage, potentially quite drastically, seems really risky to me, in a similar way to disabling perf optimizations by default. That's why I feel like giving pointers on how to improve the debugging situation would be preferable to eating the overhead by default.
alexcrichton commented on Issue #2452:
Ok, I think using an env var seems pretty reasonable for this too. I've added a commit which makes a few changes.
Config
how has a tri-state configuration for parsing debuginfo for wasm backtraces -- enabled, disabled, read an env var. The default is read an env var (the env var beingWASM_BACKTRACE_DETAILS
and enabling being seeing that to1
). This means that backtrace information is disabled by default but can be enabled with an env var. Embedders can also unconditionally enable it or unconditionally disable it.The
Display for Trap
will also print a hint to set the env var, but only when we're in environment variable mode and the original module actually had debug information. Otherwise no hint is printed because it won't do anything.
tschneidereit commented on Issue #2452:
That looks great, thank you! (Not adding a review, because I'm clearly not the right person to review the meat of this PR :grimacing:)
Last updated: Dec 23 2024 at 12:05 UTC