Miscellaneous Explore tool improvements #8649

lpereira · 2024-05-17T16:28:40Z

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

crates/explorer/src/index.js

fitzgen · 2024-05-20T21:22:20Z

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

lpereira · 2024-05-21T17:57:24Z

Rebased.

fitzgen

Thanks! Looks good, just a few comments below.

crates/explorer/src/index.js

fitzgen · 2024-05-21T19:53:41Z

crates/explorer/src/index.js

-const existingHueForOffset = offset => {
-  return offsetToHue.get(offset);
-};
+const state = (window.STATE = new State(window.WAT, window.ASM));


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

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

fitzgen · 2024-05-21T19:55:07Z

crates/explorer/src/index.js

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


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

fitzgen · 2024-05-21T21:05:52Z

crates/explorer/src/index.js

+  const eachElementWithSameWasmOff = (event, closure) => {
+    let offset = event.target.dataset.wasmOffset;
+    if (offset !== null) {
+      for (const elem of selector(offset)) closure(elem);


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

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.

This saves quite a bit of memory!

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.

… element

lpereira · 2024-05-30T18:08:11Z

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

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>

fitzgen

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

fitzgen · 2024-06-04T19:53:15Z

crates/explorer/src/index.css

+.hovered {
+}


I think we can remove this?

github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label May 17, 2024

lpereira force-pushed the explore-enhancements branch from dabb932 to 94ef8b5 Compare May 18, 2024 07:26

bjorn3 reviewed May 18, 2024

View reviewed changes

crates/explorer/src/index.js Outdated Show resolved Hide resolved

lpereira marked this pull request as ready for review May 20, 2024 17:06

lpereira requested review from a team as code owners May 20, 2024 17:06

lpereira requested review from alexcrichton and removed request for a team May 20, 2024 17:06

alexcrichton requested review from fitzgen and removed request for alexcrichton May 20, 2024 17:10

lpereira force-pushed the explore-enhancements branch from 280884e to 6fe57dc Compare May 21, 2024 17:56

fitzgen reviewed May 21, 2024

View reviewed changes

lpereira added 16 commits May 30, 2024 10:47

Auto-format explorer index.js

a7a5bfe

Use CRC24 to pick colors for offsets in explore

7bdb73f

Draw polygon bridging WAT and ASM views

83029d2

Scale luma calculation to a power of two to avoid integer division

ed88016

Outline hovered lines

e831cc4

Instead of using opacity, which slow down rendering, use a solid outline, which looks nicer and is more efficient!

Adjust bridge width to account for different scrollbar widths

8d6be49

Use simpler math in rgbToLuma() to avoid multiplications too

2780e84

Slight improvement to crc24 calculation

be93ce6

Draw multiple conections from WAT to ASM views

c78fbc9

Sometimes the ASM block contains disjoint sets of instructions that relate to one WASM instruction, so link all of them at the same time.

Show WASM offset on hover in the ASM view

6a81603

Fix grouping of ASM instructions

b938653

Some instructions with NULL wasm_offset were being group together with the first instruction in the stream that had an offset.

Only set the title attribute to show WASM offset on hover

3a212ec

This saves quite a bit of memory!

Fix grouping of instructions in ASM mode

401acda

Add note about the origin of the CRC-24 calculation

0edd617

Tweak color generation (let some brighter colors pass the filter)

759a745

lpereira added 4 commits May 30, 2024 10:49

Ensure we have enough elements to compute the bridge polygon

2e4c3d9

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.

Don't obscure the hovered line with the outline

ea6cd9d

Simplify bridge width calculation

3782a9b

Optimize hovering of blocks when one 1 WAT element matches with 1 ASM…

1de200e

… element

lpereira force-pushed the explore-enhancements branch from 6fe57dc to 2aaa7b4 Compare May 30, 2024 18:07

Address review comments

d0f6635

lpereira force-pushed the explore-enhancements branch 2 times, most recently from 31cfc61 to 055ef03 Compare May 30, 2024 22:26

Make eslint happy

0c9ee7c

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>

lpereira force-pushed the explore-enhancements branch from 055ef03 to 0c9ee7c Compare May 30, 2024 22:28

fitzgen approved these changes Jun 5, 2024

View reviewed changes

crates/explorer/src/index.css

Comment on lines +21 to +22

.hovered {

}

Copy link

Member

fitzgen Jun 4, 2024

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?

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Miscellaneous Explore tool improvements #8649

Miscellaneous Explore tool improvements #8649

lpereira commented May 17, 2024 •

edited

fitzgen commented May 20, 2024

lpereira commented May 21, 2024

fitzgen left a comment

fitzgen May 21, 2024

fitzgen May 21, 2024

fitzgen May 21, 2024

fitzgen May 21, 2024

lpereira commented May 30, 2024

fitzgen left a comment

fitzgen Jun 4, 2024

Miscellaneous Explore tool improvements #8649

Are you sure you want to change the base?

Miscellaneous Explore tool improvements #8649

Conversation

lpereira commented May 17, 2024 • edited

fitzgen commented May 20, 2024

lpereira commented May 21, 2024

fitzgen left a comment

Choose a reason for hiding this comment

fitzgen May 21, 2024

Choose a reason for hiding this comment

fitzgen May 21, 2024

Choose a reason for hiding this comment

fitzgen May 21, 2024

Choose a reason for hiding this comment

fitzgen May 21, 2024

Choose a reason for hiding this comment

lpereira commented May 30, 2024

fitzgen left a comment

Choose a reason for hiding this comment

fitzgen Jun 4, 2024

Choose a reason for hiding this comment

lpereira commented May 17, 2024 •

edited