Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Miscellaneous Explore tool improvements #8649

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

lpereira
Copy link
Contributor

@lpereira lpereira commented May 17, 2024

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: image
  • 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.)

@github-actions github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label May 17, 2024
@lpereira lpereira marked this pull request as ready for review May 20, 2024 17:06
@lpereira lpereira requested review from a team as code owners May 20, 2024 17:06
@lpereira lpereira requested review from alexcrichton and removed request for a team May 20, 2024 17:06
@alexcrichton alexcrichton requested review from fitzgen and removed request for alexcrichton May 20, 2024 17:10
@fitzgen
Copy link
Member

fitzgen commented May 20, 2024

(Will hold off on looking at this until #8639 merges)

@lpereira
Copy link
Contributor Author

Rebased.

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks good, just a few comments below.

crates/explorer/src/index.js Outdated Show resolved Hide resolved
const existingHueForOffset = offset => {
return offsetToHue.get(offset);
};
const state = (window.STATE = new State(window.WAT, window.ASM));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sent a PR to add formatting/linting for this JS, so that we don't have to nitpick about it anymore in PRs: #8675

anyByOffset.set(offset, []);
}
anyByOffset.get(offset).push(elem);
const rgbToLuma = (rgb) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

crates/explorer/src/index.js Outdated Show resolved Hide resolved
const eachElementWithSameWasmOff = (event, closure) => {
let offset = event.target.dataset.wasmOffset;
if (offset !== null) {
for (const elem of selector(offset)) closure(elem);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

crates/explorer/src/index.js Outdated Show resolved Hide resolved
crates/explorer/src/index.js Outdated Show resolved Hide resolved
In the ASM view, instead of creating one element per instruction,
create one per WASM chunk.  This significantly reduces the amount
of elements we have to take care of, including event listeners.

For both views, get rid of all the maps to look up DOM elements by
WASM offset and use CSS selectors to find them.  This made things
quite a bit smoother.  To highlight items, we now add a class to
elements with the same WASM offset, and remove it when we don't
want to highlight them anymore.

In addition to this, use a bit more tricks to get brighter colors
from the CRC24 algorithm we're now using to pick the colors.  (Since
the colors can now be very dark, we get the luminance by using the
NTSC color space, or YIQ, and use contrasting colors.)
Instead of using opacity, which slow down rendering, use a solid
outline, which looks nicer and is more efficient!
Sometimes the ASM block contains disjoint sets of instructions
that relate to one WASM instruction, so link all of them at the
same time.
Some instructions with NULL wasm_offset were being group together
with the first instruction in the stream that had an offset.
I don't know why this happens, but sometimes, a block of instructions
in the ASM side, that have a WASM offset in the generated JSON, won't
have an equivalent in the WAT side.  Guard against this.
@lpereira
Copy link
Contributor Author

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

@lpereira lpereira force-pushed the explore-enhancements branch 2 times, most recently from 31cfc61 to 055ef03 Compare May 30, 2024 22:26
Since this code runs in a browser, we need to tell eslint that
it's going to run in a browser enviroment so things like `window`
and `document` are defined.

Signed-off-by: L. Pereira <l.pereira@fastly.com>
Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +21 to +22
.hovered {
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove this?

@lpereira
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmtime:api Related to the API of the `wasmtime` crate itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants