Stream: git-wasmtime

Topic: wasmtime / PR #8639 Show function names in explore tool ...

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2024 at 21:24):

lpereira opened PR #8639 from lpereira:show-func-names-in-explore to bytecodealliance:main:

Another one in the "let's make debugging a bit easier": show function names in the "explore" tool. (There's a bit of code repetition here to clean up the names, but it's slightly different when we're considering HTML output, so I think that's OK.)

Here's how it works on my machine:
<img width="1023" alt="image" src="https://github.com/bytecodealliance/wasmtime/assets/15001/9bb4b160-1bb9-44d3-a59f-615694f24a99">

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2024 at 21:24):

lpereira requested pchickey for a review on PR #8639.

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2024 at 21:24):

lpereira requested wasmtime-core-reviewers for a review on PR #8639.

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2024 at 21:32):

lpereira updated PR #8639.

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2024 at 22:04):

jameysharp commented on PR #8639:

Very nice!

This can be further improved by demangling the function names first. For some example code that does that, see: https://github.com/bytecodealliance/wasmtime/blob/54e53cc77a29c8e4eb819333d81ef4158685690a/crates/wasmtime/src/runtime/profiling.rs#L194-L200

I don't think we need to be nearly so careful about sanitizing function names in this setting. These strings are first serialized to JSON, and that should ensure that any string we provide is valid JSON. The JSON string is then used in a <script> tag; we might need to escape the JSON for embedding in HTML, but <script> tags are implicitly treated as CDATA in HTML5, right? Finally, the strings embedded in the JSON are spliced into the DOM using textContent, so they don't need to be escaped at that point.

As a result I think we should just give the user whatever they provided, preferably after demangling.

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2024 at 22:23):

fitzgen submitted PR review:

Thanks! One nitpick below before we merge this.

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2024 at 22:23):

fitzgen submitted PR review:

Thanks! One nitpick below before we merge this.

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2024 at 22:23):

fitzgen created PR review comment:

We can avoid looking up the function index via instruction offset since function_locations should already return the functions in order:

        self.function_locations().enumerate().map(|(idx, (offset, len))| {
            let idx = u32::try_from(idx).unwrap();

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2024 at 22:25):

fitzgen commented on PR #8639:

Agreed demangling would be great -- also fine with deferring that to a follow up PR.

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2024 at 23:10):

lpereira requested wasmtime-default-reviewers for a review on PR #8639.

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2024 at 23:10):

lpereira updated PR #8639.

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2024 at 23:13):

lpereira updated PR #8639.

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2024 at 23:13):

lpereira commented on PR #8639:

Pushed the requested changes. Current output here is now this one (notice the pop-up showing the unmangled name):

<img width="656" alt="image" src="https://github.com/bytecodealliance/wasmtime/assets/15001/53a2838d-71d2-452a-a8f1-c47fc7cb334b">

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2024 at 23:15):

lpereira edited a comment on PR #8639:

Pushed the requested changes. Current output here is now this one (notice the pop-up showing the mangled name):

<img width="656" alt="image" src="https://github.com/bytecodealliance/wasmtime/assets/15001/53a2838d-71d2-452a-a8f1-c47fc7cb334b">

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2024 at 23:17):

lpereira updated PR #8639.

view this post on Zulip Wasmtime GitHub notifications bot (May 17 2024 at 00:10):

jameysharp commented on PR #8639:

Ooh, the pop-up is a nice touch. There's a lot more that could be done with wasmtime explore eventually.

I'd be happy to review this next week, or just as happy if you want to approve it @fitzgen.

view this post on Zulip Wasmtime GitHub notifications bot (May 18 2024 at 16:22):

lpereira updated PR #8639.

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2024 at 21:13):

fitzgen submitted PR review:

Thanks! Really excited to get this, and your other PR, in wasmtime explore.

I have a few comments and suggestions below that we should address before merging this however. I'll also hold off on reviewing the other PR until after this one merges.

Let me know if anything I said isn't clear or something like that. Thanks again!

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2024 at 21:13):

fitzgen created PR review comment:

Let's revert this change; it just introduces unnecessary parens.

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2024 at 21:13):

fitzgen submitted PR review:

Thanks! Really excited to get this, and your other PR, in wasmtime explore.

I have a few comments and suggestions below that we should address before merging this however. I'll also hold off on reviewing the other PR until after this one merges.

Let me know if anything I said isn't clear or something like that. Thanks again!

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2024 at 21:13):

fitzgen created PR review comment:

Ditto.

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2024 at 21:13):

fitzgen created PR review comment:

Same thing here. AFAICT, this just changes indentation and adds unnecessary parens around the argument. This illustrates why it is preferrable to avoid stylistic changes in functional PRs: it makes it harder for reviewers to determine if there are any functional changes involved in a particular diff region or not.

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2024 at 21:13):

fitzgen created PR review comment:

AFAICT, this is the only non-stylistic change to the whole JS file? Can we remove all the other changes to this file?

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2024 at 21:13):

fitzgen created PR review comment:

Ditto. Let's not change code style when making unrelated changes.

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2024 at 21:13):

fitzgen created PR review comment:

Ditto.

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2024 at 21:13):

fitzgen created PR review comment:

I think it would be better to let demangled_name be an Option<String> rather than making a clone of name and then have the JS choose the demangled name when available and fall back to the plain name.

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2024 at 21:13):

fitzgen created PR review comment:

Instead of stringifying the name with the index here, and then forcing the consumer of this API in crates/explore/src/lib.rs to parse the name out from stringified name out, can we just give the caller the various components before stringification? That is, return an iterator that yields items of type

(
    u32,            // wasm function index
    Option<String>, // name, if available
    usize,          // text section offset
    usize,          // length of function in text section
)

I think we could also update the existing function_locations method, and perhaps rename it, rather than defining a whole new method, since that method exists to basically enable wasmtime-explore.

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2024 at 22:20):

lpereira submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2024 at 22:20):

lpereira created PR review comment:

Alright.

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2024 at 22:20):

lpereira created PR review comment:

OK.

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2024 at 22:20):

lpereira submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2024 at 22:20):

lpereira submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2024 at 22:20):

lpereira created PR review comment:

Sure. It was the auto-formatting tool that my editor uses. I'll have to force-push to fix this easily.

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2024 at 22:21):

lpereira submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2024 at 22:21):

lpereira created PR review comment:

OK.

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2024 at 22:21):

lpereira updated PR #8639.

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2024 at 22:24):

lpereira commented on PR #8639:

Alright, force-pushed (sorry!) due to the formatting changes, but it should be a whole lot clearer now. I'll rebase the other PR once this one lands.

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2024 at 23:20):

lpereira updated PR #8639.

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2024 at 15:05):

fitzgen submitted PR review:

Thanks! Looks just about ready to merge, couple tiny things still to go first.

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2024 at 15:05):

fitzgen submitted PR review:

Thanks! Looks just about ready to merge, couple tiny things still to go first.

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2024 at 15:05):

fitzgen created PR review comment:

I think this condition is backwards, we either want to !== or we want to swap the arms.

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2024 at 15:05):

fitzgen created PR review comment:

I think function_loactions is only used by this method, so we can we remove it or make it non-pub, as I suggested previously?

Also I like the named struct with named fields, that's indeed better than a large-ish tuple, but the docs here should match that now.

And since this is really more than just function locations and names (it also has an index, for example) maybe we should call this method function_metadatas and the struct FunctionMetadata? Feel free to take-or-leave this last one...

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2024 at 17:05):

lpereira updated PR #8639.

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2024 at 17:05):

lpereira submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2024 at 17:05):

lpereira created PR review comment:

Good catch, let me clean this up.

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2024 at 17:06):

lpereira submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2024 at 17:06):

lpereira created PR review comment:

Oops!

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2024 at 17:06):

lpereira commented on PR #8639:

Alright, @fitzgen, pushed a new revision that should address your comments.

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2024 at 17:27):

fitzgen submitted PR review:

Looks great, thanks! And thanks for your patience with review :)

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2024 at 17:42):

fitzgen merged PR #8639.

Last updated: Nov 22 2024 at 17:03 UTC