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

misc(treemap): unused-bytes view mode #12142

Merged
merged 6 commits into from
Mar 16, 2021
Merged

Conversation

connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented Feb 26, 2021

ref #11254

https://lighthouse-1gxbe1yl8-googlechrome.vercel.app/gh-pages/treemap/?debug

This adds a "view modes" data structure, which defines how to interpret the data when walking currentTreemapRoot. unused-bytes is the first view mode, which highlights nodes with especially large amounts of unused bytes, and partitions the treemap based on percentage of total unused bytes on the page.

This only shows unused bytes based on javascript coverage, but there's no reason in the future that we couldn't determine unused bytes of images, css, etc. and plug them into this view mode.

image

@connorjclark connorjclark requested a review from a team as a code owner February 26, 2021 02:42
@connorjclark connorjclark requested review from patrickhulce and removed request for a team February 26, 2021 02:42
@google-cla google-cla bot added the cla: yes label Feb 26, 2021
Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

code mostly LGTM just minor stuff 💯

I had a few UX notes while I was playing around with it, but it's been a while since I remember talking about this so might have already been covered.

  1. It takes a fair bit of time to orient oneself to a new treemap, identifying your bundles and what files you're looking at. When you click "unused" then, it was quite jarring to have the entire thing reshuffle orders, so I'm second guessing if that's the best way to interact with this information. IIRC, our rationale was that luminance intensity is pretty useless? But I was surprised in practice how powerful the use of white v. no color was already. My current thinking is that it might be a more powerful call to action if you get to see the unused modules in the context of your entire bundle for the easy wins. Right now it's weird that I'm looking at 100% unused bytes, so it feels like there's no priority.
  2. The percentages in the module labels were confusing in the context of an unused bytes visualization. I get that it's a treemap and we're saying "hey this module's unused bytes were 2% of your overall unused bytes" but I don't think I care. I kinda want to know how much of the module itself was unused in that percentage.
  3. The usage breakdown feels pretty unactionable when it's an entire bundle without a source map and there's quite a bit of visual space dedicated to them. I don't have a suggestion on how to fix yet, maybe we split them into source mapped and non-source mapped with less emphasis on the giant blobs?

lighthouse-treemap/app/src/main.js Outdated Show resolved Hide resolved
TreemapUtil.walk(this.currentTreemapRoot, (node, path) => {
// Only highlight leaf nodes of a certain size.
if (node.children) return;
if (!node.unusedBytes || node.unusedBytes < 50 * 1024) return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

extract this 50KiB to a constant? feels like it will get buried otherwise :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

also should this account for relative usage some? like if 90% of a module 500kb dep is used, feels a bit weird to flag it as unused (though a bloat visualization should 😆 )

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copied the constants from the unused-audit for this, but I also refactored things a bit...

Now, it will only highlight anything in a d1 (depth one) node if it is greater than some number of unused bytes. The audit uses a 20KiB threshold to ignore an entire bundle, so I did the same here. For a specific leaf node (a module in a bundle), only highlight if more than half of the bytes are unused.

Later PRs will add a pie graph/something similar to show how much of a node is unused.

lighthouse-treemap/app/src/main.js Show resolved Hide resolved
lighthouse-treemap/app/src/main.js Outdated Show resolved Hide resolved
lighthouse-treemap/app/src/main.js Outdated Show resolved Hide resolved
lighthouse-treemap/app/src/main.js Outdated Show resolved Hide resolved
@connorjclark
Copy link
Collaborator Author

It takes a fair bit of time to orient oneself to a new treemap, identifying your bundles and what files you're looking at. When you click "unused" then, it was quite jarring to have the entire thing reshuffle orders, so I'm second guessing if that's the best way to interact with this information. IIRC, our rationale was that luminance intensity is pretty useless? But I was surprised in practice how powerful the use of white v. no color was already. My current thinking is that it might be a more powerful call to action if you get to see the unused modules in the context of your entire bundle for the easy wins. Right now it's weird that I'm looking at 100% unused bytes, so it feels like there's no priority.

I removed the unusedBytes partition for this view mode and I agree, it is much less jarring. For purposes of quick assessment I think this is the better experience so let's do that. I'm not certain that a "partition by unused bytes" couldn't be useful to someone. For myself, if I'm tackling the problem "let's reduce how much unused JS we are shipping", I'd appreciate the ability to partition by the metric I'm trying to reduce. But that can be relegated to the settings module.

The percentages in the module labels were confusing in the context of an unused bytes visualization. I get that it's a treemap and we're saying "hey this module's unused bytes were 2% of your overall unused bytes" but I don't think I care. I kinda want to know how much of the module itself was unused in that percentage.

Later PRs will add a gauge to each module to indicate how much is unused. also, the table component that is coming up soon offers another way to view this data. see gallery.googleplex.com/projects/MCHbtQVoQ2HCZeok9-CasvMb/files/MCFFTHkODMy6RyA6lXJuFGJb

The usage breakdown feels pretty unactionable when it's an entire bundle without a source map and there's quite a bit of visual space dedicated to them. I don't have a suggestion on how to fix yet, maybe we split them into source mapped and non-source mapped with less emphasis on the giant blobs?

I don't want to deemphasize something just because it doesn't have a source map. Perhaps an indicator for these nodes that "A source map will provide more granular data" will suffice?

for (const d1Node of this.currentTreemapRoot.children || []) {
// Only highlight leaf nodes if entire node (ie a JS bundle) has greater than a certain
// number of unused bytes.
if (!d1Node.unusedBytes || d1Node.unusedBytes < UNUSED_BYTES_IGNORE_THRESHOLD) continue;
Copy link
Member

Choose a reason for hiding this comment

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

based on the UNUSED_BYTES_IGNORE_THRESHOLD over in unused-bytes.... wouldnt it meant that there is no node created in those cases?

regardless.. this seems fine to me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no, the treemap data sets unusedBytes straight from the coverage data, not the unused-javascript audit.

@@ -191,6 +244,14 @@ class TreemapViewer {
// Ran out of colors.
}

// A view can set nodes to highlight. If so, don't color anything else.
if (this.currentViewMode.highlightNodePaths) {
Copy link
Member

Choose a reason for hiding this comment

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

Array.isArray

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

but, typescript?

lighthouse-treemap/app/src/main.js Outdated Show resolved Hide resolved
lighthouse-treemap/app/src/main.js Outdated Show resolved Hide resolved
Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

lgtm. one small thing i noticed.

@@ -65,6 +65,12 @@ body {
background-color: lightblue;
}

.view-mode__button {
Copy link
Member

Choose a reason for hiding this comment

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

btw you're losing your cursor styling because label has a default (who knew)

image

@connorjclark connorjclark merged commit aae0d0e into master Mar 16, 2021
@connorjclark connorjclark deleted the treemap-unused-bytes branch March 16, 2021 22:50
@connorjclark connorjclark mentioned this pull request Mar 17, 2021
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants