wasmtime / PR #8649 Miscellaneous Explore tool improvements · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #8649 Miscellaneous Explore tool improvements

Wasmtime GitHub notifications bot (May 17 2024 at 16:28):

lpereira opened PR #8649 from lpereira:explore-enhancements to bytecodealliance:main:

I had some time last night, so I decided to clean up the explore tool frontend code. This is almost a full rewrite, that:

Creates fewer DOM nodes for the ASM view, by grouping all instructions with the same WASM offset in a single node. The performance differences in large files is considerable.

Uses CSS selectors and the dataset to query which ASM view node relates to which WAT view node (and vice versa). This removes all the maps that were being used to keep track of these relationships, simplifying the code quite a bit (and probably making it a bit quicker, but I haven't really measured.)

Changes the way colors are allocated. Instead of a set of hues that are chosen with round-robin from a fixed list, colors are now the CRC24 of the WASM offset, with a very special polynomial that gives us bright, unique, and vibrant colors. Text color can now be either light or dark, depending on the color contrast (fun fact: I used an approximation using only powers of two of the luminance channel in the NTSC colorspace to determine this.)

Avoid, as much as possible, using per-node properties and prefer using CSS classes.

Draws a polygon between hovered the WAT and ASM views to bridge hovered instructions: <img width="459" alt="image" src="https://github.com/bytecodealliance/wasmtime/assets/15001/61e6f407-dc09-4198-9160-b8f4f819b8d1">

Instead of changing the background color or opacity of the hovered instructions, draw an outline with the same background color to make them stand better.

This is just a draft for now, because there are still a couple of bugs I have to fix, but I'm happy with how this is turning out, and I'd like some feedback. (This builds on #8639.)

Wasmtime GitHub notifications bot (May 17 2024 at 22:27):

lpereira edited PR #8649:

I had some time last night, so I decided to clean up the explore tool frontend code. This is almost a full rewrite, that:

Creates fewer DOM nodes for the ASM view, by grouping all instructions with the same WASM offset in a single node. The performance differences in large files is considerable.

Uses CSS selectors and the dataset to query which ASM view node relates to which WAT view node (and vice versa). This removes all the maps that were being used to keep track of these relationships, simplifying the code quite a bit (and probably making it a bit quicker, but I haven't really measured.)

Changes the way colors are allocated. Instead of a set of hues that are chosen with round-robin from a fixed list, colors are now the CRC24 of the WASM offset, with a very special polynomial that gives us bright, unique, and vibrant colors. Text color can now be either light or dark, depending on the color contrast (fun fact: I used an approximation using only powers of two of a formula that gives me the luminance channel (in the NTSC colorspace) from a RGB triplet to determine this.)

Avoid, as much as possible, using per-node properties and prefer using CSS classes.

Draws a polygon between hovered the WAT and ASM views to bridge hovered instructions: <img width="459" alt="image" src="https://github.com/bytecodealliance/wasmtime/assets/15001/61e6f407-dc09-4198-9160-b8f4f819b8d1">

Instead of changing the background color or opacity of the hovered instructions, draw an outline with the same background color to make them stand better.

This is just a draft for now, because there are still a couple of bugs I have to fix, but I'm happy with how this is turning out, and I'd like some feedback. (This builds on #8639.)

Wasmtime GitHub notifications bot (May 17 2024 at 23:44):

lpereira updated PR #8649.

Wasmtime GitHub notifications bot (May 18 2024 at 07:26):

lpereira updated PR #8649.

Wasmtime GitHub notifications bot (May 18 2024 at 09:06):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (May 18 2024 at 09:06):

bjorn3 created PR review comment:

Did you come up with this or did you get it from somewhere else?

Wasmtime GitHub notifications bot (May 18 2024 at 16:04):

lpereira created PR review comment:

It's adapted from Wikipedia. I'll add a note in the code. (The polynomial is just "fastly" written using hex digits. It generates surprisingly bright and vibrant colors.)

Wasmtime GitHub notifications bot (May 18 2024 at 16:04):

lpereira submitted PR review.

Wasmtime GitHub notifications bot (May 18 2024 at 16:05):

lpereira updated PR #8649.

Wasmtime GitHub notifications bot (May 18 2024 at 16:37):

lpereira updated PR #8649.

Wasmtime GitHub notifications bot (May 20 2024 at 16:20):

lpereira updated PR #8649.

Wasmtime GitHub notifications bot (May 20 2024 at 17:06):

lpereira has marked PR #8649 as ready for review.

Wasmtime GitHub notifications bot (May 20 2024 at 17:06):

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

Wasmtime GitHub notifications bot (May 20 2024 at 17:06):

lpereira requested alexcrichton for a review on PR #8649.

Wasmtime GitHub notifications bot (May 20 2024 at 17:06):

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

Wasmtime GitHub notifications bot (May 20 2024 at 17:10):

alexcrichton requested fitzgen for a review on PR #8649.

Wasmtime GitHub notifications bot (May 20 2024 at 19:43):

lpereira updated PR #8649.

Wasmtime GitHub notifications bot (May 20 2024 at 21:22):

fitzgen commented on PR #8649:

(Will hold off on looking at this until https://github.com/bytecodealliance/wasmtime/pull/8639 merges)

Wasmtime GitHub notifications bot (May 21 2024 at 17:56):

lpereira updated PR #8649.

Wasmtime GitHub notifications bot (May 21 2024 at 17:57):

lpereira commented on PR #8649:

Rebased.

Wasmtime GitHub notifications bot (May 21 2024 at 21:19):

fitzgen submitted PR review:

Thanks! Looks good, just a few comments below.

Wasmtime GitHub notifications bot (May 21 2024 at 21:19):

fitzgen submitted PR review:

Thanks! Looks good, just a few comments below.

Wasmtime GitHub notifications bot (May 21 2024 at 21:19):

fitzgen created PR review comment:

Similar about matching existing code style: none of the rest of the code is adding parens to single-arg arrow functions.

Wasmtime GitHub notifications bot (May 21 2024 at 21:19):

fitzgen created PR review comment:

Can you avoid stylistic changes? Or configure your editor to avoid them?

Wasmtime GitHub notifications bot (May 21 2024 at 21:19):

fitzgen created PR review comment:

Sent a PR to add formatting/linting for this JS, so that we don't have to nitpick about it anymore in PRs: https://github.com/bytecodealliance/wasmtime/pull/8675

Wasmtime GitHub notifications bot (May 21 2024 at 21:19):

fitzgen created PR review comment:

This is using offsetToRgb as an object with a dynamic property that is a stringified integer. That is probably fine, I don't think any of this code is actually hot, and JS engines optimize the crap out of object access so this might actually be faster than actual map lookups, but we should be consistent so as not to confuse future code readers. That means doing one of the following two options:

Define offsetToRgb as {} to emphasize that it is being used as an object

Do offsetToRgb.get(offset) here instead of a property look up

Wasmtime GitHub notifications bot (May 21 2024 at 21:19):

fitzgen created PR review comment:

Walking the DOM every time seems more expensive than the old approach of maintaining a map from offset to list of elements associated with that offset. I'd prefer sticking with the previous approach unless there is strong motivation to do otherwise.

Wasmtime GitHub notifications bot (May 21 2024 at 21:19):

fitzgen created PR review comment:

Can we add a newline between all of these functions and a comment to the top describing what they are doing? It can certainly be gleamed by looking at the context in which they're used, but we shouldn't be forcing code readers to do that, we should help them out and save them the effort so they can focus on whatever they're really trying to do.

Wasmtime GitHub notifications bot (May 21 2024 at 21:19):

fitzgen created PR review comment:

Can we use camel case here? That is pretty standard JS style, and is also what the rest of the file uses. (We use snake case for the JSON-encoded data because it comes from Rust)

Wasmtime GitHub notifications bot (May 21 2024 at 21:19):

fitzgen created PR review comment:

I don't think it makes sense to scroll every asm element into view, since I think this is equivalent to scrolling just the last asm element into view that we call this method on. We should just choose one of them (first? last? middle?) and scroll that one into view. It looks like we used to do the first one, fwiw.

Wasmtime GitHub notifications bot (May 30 2024 at 17:46):

lpereira created PR review comment:

Done.

Wasmtime GitHub notifications bot (May 30 2024 at 17:46):

lpereira submitted PR review.

Wasmtime GitHub notifications bot (May 30 2024 at 17:46):

lpereira submitted PR review.

Wasmtime GitHub notifications bot (May 30 2024 at 17:46):

lpereira created PR review comment:

Sure.

Wasmtime GitHub notifications bot (May 30 2024 at 17:46):

lpereira submitted PR review.

Wasmtime GitHub notifications bot (May 30 2024 at 17:46):

lpereira created PR review comment:

I'm fine either way.

Wasmtime GitHub notifications bot (May 30 2024 at 17:46):

lpereira submitted PR review.

Wasmtime GitHub notifications bot (May 30 2024 at 17:46):

lpereira created PR review comment:

Of course.

Wasmtime GitHub notifications bot (May 30 2024 at 18:07):

lpereira updated PR #8649.

Wasmtime GitHub notifications bot (May 30 2024 at 18:08):

lpereira commented on PR #8649:

Rebased (to get your patch with prettier/eslint configs) and addressed your comments.

Wasmtime GitHub notifications bot (May 30 2024 at 20:05):

lpereira updated PR #8649.

Wasmtime GitHub notifications bot (May 30 2024 at 22:25):

lpereira updated PR #8649.

Wasmtime GitHub notifications bot (May 30 2024 at 22:26):

lpereira updated PR #8649.

Wasmtime GitHub notifications bot (May 30 2024 at 22:28):

lpereira updated PR #8649.

Wasmtime GitHub notifications bot (Jun 05 2024 at 14:13):

fitzgen submitted PR review:

Thanks for adding all the comments, that helps a bunch!

Wasmtime GitHub notifications bot (Jun 05 2024 at 14:13):

fitzgen submitted PR review:

Thanks for adding all the comments, that helps a bunch!

Wasmtime GitHub notifications bot (Jun 05 2024 at 14:13):

fitzgen created PR review comment:

I think we can remove this?

Wasmtime GitHub notifications bot (Aug 19 2024 at 01:20):

lpereira commented on PR #8649:

@fitzgen Could you please merge this? (It's unlikely I'll continue working on this as this was done as part of a job, but I don't want this to bitrot.)

Last updated: Nov 22 2024 at 17:03 UTC