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

Duplicate Modules: Add option to include their bytes #4069

Merged
merged 6 commits into from
Feb 20, 2024

Conversation

danielbeardsley
Copy link
Contributor

@danielbeardsley danielbeardsley commented Dec 4, 2023

Two bundles may have large byte differences mainly because of some
modules being duplicated more or less often.

The totals show if the whole bundle has grown or shrunk but the modules
section couldn't tell you which modules had increased or decreased the
number of duplicates. Thus you could know the change was because of
altered duplication, but it would be hard to identify which modules
caused the change.

This introduces a checkbox on the Modules tab to include the byte size
of the duplicates when comparing module sizes.

There are other ways of getting this done, but I tried to choose the
least invasive way.

Here's a few others I thought of:

  • Calculating deltas right before display instead of during setup would
    allow altering the calculation
    • We'd have to break the symmetry of the different comparison types
    • Include a new duplicates: number property in each entry
  • Make a new "Modules Wtih Duplicates" tab or similar that includes the
    true size of all duplicates
    • This would be a much more involved change and since the actual
      difference is slight I don't think it's appropriate.
  • Probably other ways I haven't thought of cause I have very little
    experience with this project.

This also (in the second commit) augments the Module Info dialog to
list out the chunks from each Job instead just the first job.

This somewhat solves the concerns of #3929

@danielbeardsley
Copy link
Contributor Author

Before

Using the fixture data, this is a view of duplicated modules without counting duplicate bytes. It's hard to tell what changed:
image

After

You can see how the duplicates affect the total bytes each module is contributing:
image

Copy link

relativeci bot commented Dec 4, 2023

Job #10608: Bundle Size — 301.24KiB (+0.36%).

e23d926(current) vs e874e04 master#10598(baseline)

Bundle metrics  Change 4 changes Regression 2 regressions
                 Current
Job #10608
     Baseline
Job #10598
Regression  Initial JS 263.54KiB(+0.39%) 262.51KiB
Regression  Initial CSS 37.7KiB(+0.13%) 37.65KiB
Change  Cache Invalidation 41.82% 29.27%
No change  Chunks 3 3
No change  Assets 4 4
Change  Modules 531(+0.19%) 530
No change  Duplicate Modules 0 0
No change  Duplicate Code 0% 0%
No change  Packages 23 23
No change  Duplicate Packages 0 0
Bundle size by type  Change 2 changes Regression 2 regressions
                 Current
Job #10608
     Baseline
Job #10598
Regression  JS 263.54KiB (+0.39%) 262.51KiB
Regression  CSS 37.7KiB (+0.13%) 37.65KiB

View job #10608 reportView iFixit:duplicate-true-sizes branch activityView project dashboard

@danielbeardsley
Copy link
Contributor Author

I've rebased this on master to resolve the conflicts.

@danielbeardsley
Copy link
Contributor Author

@vio -- I've incorporated your metric selector UI into this pull.

@danielbeardsley
Copy link
Contributor Author

@vio -- I don't how to re-record webpack snapshots for the tests, otherwise I would.

@vio
Copy link
Member

vio commented Feb 17, 2024

@danielbeardsley, the failed test is not related to your changes. We relied on webpack stats snapshots, and the latest webpack stopped generating deterministic chunk IDs. I updated the tests and they are passing now with the latest webpack, the PR should be ok once you rebase with the latest master.

vio and others added 5 commits February 18, 2024 00:23
In the module info dialog we list the chunks that a module is in.
Previously, we only showed the list of chunks that it's in from *one* of
the jobs. Listing all chunks allows you to see if a module is
duplicated and how that duplication is different across jobs.
Two bundles may have large byte differences mainly because of some
modules being duplicated more or less often.

The totals show if the whole bundle has grown or shrunk but the modules
section couldn't tell you which modules had increased or decreased the
number of duplicates. Thus you could know the change was because of
altered duplication, but it would be hard to identify which modules
caused the change.

This hooks up the ModuleMetric selector to include the byte size
of the duplicates when comparing module sizes.

There are other ways of getting this done, but I tried to choose the
least invasive way. doing this operation as a selector allows pretty
easy extension to measuring other types (like duplicate count)
They were added for a prior implementation of something and are no
longer used /needed.
@danielbeardsley
Copy link
Contributor Author

once you rebase with the latest master.

Done!

Copy link
Member

@vio vio left a comment

Choose a reason for hiding this comment

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

Thank you @danielbeardsley, looks great!

onClick={() => setModuleMetric(ModuleMetric.TOTAL_SIZE)}
title="Size (including duplicate modules)"
>
Total size
Copy link
Member

Choose a reason for hiding this comment

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

Should we have the label + title be more explicit, for example: Module total size? I think we are using Total size for the entire bundle size in some places.

Also, if we need to make the title stand up, we can use a Tooltip/Hovercard to render it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! Went for <Tooltip> and I did change the name. The names aren't the best, but I couldn't think of anything better and the tooltips clear it up I think.

packages/utils/src/webpack/selectors.js Show resolved Hide resolved
To use the same UI components that are used elsewhere.

Also, I changed Total Size to `Module Total Size`.

It's not obvious how to better describe these metrics succintly... but
that's why we have tooltips.
@vio vio merged commit c86e729 into relative-ci:master Feb 20, 2024
25 checks passed
@vio
Copy link
Member

vio commented Feb 20, 2024

Thank you @danielbeardsley, looks great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants