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.)
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.)
lpereira updated PR #8649.
lpereira updated PR #8649.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
Did you come up with this or did you get it from somewhere else?
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.)
lpereira submitted PR review.
lpereira updated PR #8649.
lpereira updated PR #8649.
lpereira updated PR #8649.
lpereira has marked PR #8649 as ready for review.
lpereira requested wasmtime-core-reviewers for a review on PR #8649.
lpereira requested alexcrichton for a review on PR #8649.
lpereira requested wasmtime-default-reviewers for a review on PR #8649.
alexcrichton requested fitzgen for a review on PR #8649.
lpereira updated PR #8649.
fitzgen commented on PR #8649:
(Will hold off on looking at this until https://github.com/bytecodealliance/wasmtime/pull/8639 merges)
lpereira updated PR #8649.
lpereira commented on PR #8649:
Rebased.
fitzgen submitted PR review:
Thanks! Looks good, just a few comments below.
fitzgen submitted PR review:
Thanks! Looks good, just a few comments below.
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.
fitzgen created PR review comment:
Can you avoid stylistic changes? Or configure your editor to avoid them?
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
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
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.
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.
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)
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.
lpereira created PR review comment:
Done.
lpereira submitted PR review.
lpereira submitted PR review.
lpereira created PR review comment:
Sure.
lpereira submitted PR review.
lpereira created PR review comment:
I'm fine either way.
lpereira submitted PR review.
lpereira created PR review comment:
Of course.
lpereira updated PR #8649.
lpereira commented on PR #8649:
Rebased (to get your patch with
prettier
/eslint
configs) and addressed your comments.
lpereira updated PR #8649.
lpereira updated PR #8649.
lpereira updated PR #8649.
lpereira updated PR #8649.
fitzgen submitted PR review:
Thanks for adding all the comments, that helps a bunch!
fitzgen submitted PR review:
Thanks for adding all the comments, that helps a bunch!
fitzgen created PR review comment:
I think we can remove this?
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: Dec 23 2024 at 12:05 UTC