Stream: git-wasmtime

Topic: wasmtime / Issue #2452 Provide filename/line number infor...

Wasmtime GitHub notifications bot (Nov 25 2020 at 21:41):

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>

Wasmtime GitHub notifications bot (Nov 25 2020 at 21:44):

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

Wasmtime GitHub notifications bot (Nov 25 2020 at 22:08):

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 in Store, but this second copy could be removed with an Arc 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 to debug_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.

Wasmtime GitHub notifications bot (Nov 30 2020 at 17:19):

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.

Wasmtime GitHub notifications bot (Nov 30 2020 at 17:24):

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?

Wasmtime GitHub notifications bot (Nov 30 2020 at 17:56):

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.

Wasmtime GitHub notifications bot (Nov 30 2020 at 18:13):

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 via memfd if we didn't use Module::from_file).

Wasmtime GitHub notifications bot (Nov 30 2020 at 19:18):

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.

Wasmtime GitHub notifications bot (Dec 01 2020 at 17:01):

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
...

Wasmtime GitHub notifications bot (Dec 01 2020 at 18:14):

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.

Wasmtime GitHub notifications bot (Dec 01 2020 at 18:55):

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 being WASM_BACKTRACE_DETAILS and enabling being seeing that to 1). 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.

Wasmtime GitHub notifications bot (Dec 01 2020 at 19:14):

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: Nov 22 2024 at 16:03 UTC