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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix totals for areas in comments #27

Merged
merged 3 commits into from Dec 16, 2020
Merged

Conversation

sgomes
Copy link
Contributor

@sgomes sgomes commented Dec 9, 2020

GitHub comments by ICFY currently have an issue with calculating totals for a particular "area". For example, for sections:

**Sections** (~119368 bytes removed 馃搲 [gzipped])

name                   parsed_size            gzip_size
signup                   -159335 B  (-31.0%)   -37498 B  (-29.4%)
checkout                 -152990 B   (-9.3%)   -35377 B   (-8.7%)
gutenberg-editor         -149228 B  (-26.6%)   -35530 B  (-23.0%)
reader                     -2907 B   (-0.5%)     -508 B   (-0.3%)
plans                      -2625 B   (-0.3%)     -399 B   (-0.2%)
jetpack-connect            -2436 B   (-0.3%)     -471 B   (-0.3%)
jetpack-cloud-pricing      -2436 B   (-0.6%)     -471 B   (-0.4%)
hosting                    -2436 B   (-0.8%)     -471 B   (-0.6%)
woocommerce                -1107 B   (-0.0%)     -307 B   (-0.1%)
posts-custom                -677 B   (-0.2%)     -151 B   (-0.1%)
posts                       -677 B   (-0.2%)     -151 B   (-0.1%)
settings                    -540 B   (-0.1%)     -590 B   (-0.4%)
privacy                     -410 B   (-0.1%)    -1066 B   (-1.3%)
settings-security           -398 B   (-0.1%)      -34 B   (-0.0%)
settings-writing            -391 B   (-0.1%)      -64 B   (-0.0%)
plugins                     -380 B   (-0.1%)      -38 B   (-0.0%)
purchases                   -339 B   (-0.0%)     +113 B   (+0.0%)
site-blocks                 -314 B   (-0.1%)    -1037 B   (-1.2%)
account                     -303 B   (-0.1%)    -1038 B   (-1.0%)
media                       -273 B   (-0.1%)     +299 B   (+0.2%)
wp-super-cache              -247 B   (-0.1%)      -30 B   (-0.0%)
settings-performance        -247 B   (-0.1%)      -30 B   (-0.0%)
settings-discussion         -247 B   (-0.1%)      -30 B   (-0.0%)
marketing                   -247 B   (-0.0%)      -30 B   (-0.0%)
earn                        -247 B   (-0.1%)      -25 B   (-0.0%)
site-purchases              -234 B   (-0.0%)     +608 B   (+0.3%)
home                        -203 B   (-0.0%)     -347 B   (-0.3%)
stats                       -195 B   (-0.0%)      -49 B   (-0.0%)
google-my-business          -169 B   (-0.1%)      -24 B   (-0.0%)
security                    -163 B   (-0.0%)    -1031 B   (-0.8%)
notification-settings       -163 B   (-0.0%)    -1031 B   (-1.1%)
me                          -163 B   (-0.1%)    -1031 B   (-1.4%)
happychat                   -163 B   (-0.1%)    -1031 B   (-1.2%)
account-close               -163 B   (-0.0%)    -1031 B   (-1.1%)
zoninator                   -151 B   (-0.1%)      -10 B   (-0.0%)
people                      -142 B   (-0.0%)      -70 B   (-0.1%)
import                      -142 B   (-0.1%)     -119 B   (-0.2%)
domains                     -135 B   (-0.0%)     +145 B   (+0.1%)
pages                       +101 B   (+0.0%)     +142 B   (+0.2%)
devdocs                      -95 B   (-0.0%)       -7 B   (-0.0%)
help                         -52 B   (-0.0%)       -1 B   (-0.0%)
email                        +42 B   (+0.0%)     +453 B   (+0.5%)

We see that the total is being calculated simply by adding up all the individual diffs for each group. This is incorrect, since groups can share chunks, and if a shared chunk changes size, it will be counted multiple times with this approach.

This PR instead takes shared chunks into account, by adding up all the chunks in use across an entire area for the "before" and "after" commits, and subtracting those sums instead (note: the values are slightly different because the above is from a PR and the below is from its merge):

**Sections** (~54533 bytes removed 馃搲 [gzipped])

name                   parsed_size            gzip_size
signup                   -159335 B  (-31.0%)   -37497 B  (-29.3%)
checkout                 -152990 B   (-9.3%)   -35377 B   (-8.7%)
gutenberg-editor         -149228 B  (-26.6%)   -35525 B  (-23.0%)
reader                     -2907 B   (-0.5%)     -508 B   (-0.3%)
plans                      -2625 B   (-0.3%)     -399 B   (-0.2%)
jetpack-connect            -2436 B   (-0.3%)     -471 B   (-0.3%)
jetpack-cloud-pricing      -2436 B   (-0.6%)     -471 B   (-0.4%)
hosting                    -2436 B   (-0.8%)     -471 B   (-0.6%)
woocommerce                -1107 B   (-0.0%)     -305 B   (-0.1%)
posts-custom                -677 B   (-0.2%)     -151 B   (-0.1%)
posts                       -677 B   (-0.2%)     -151 B   (-0.1%)
settings                    -556 B   (-0.1%)     -606 B   (-0.4%)
privacy                     -410 B   (-0.1%)    -1066 B   (-1.3%)
settings-security           -398 B   (-0.1%)      -34 B   (-0.0%)
settings-writing            -391 B   (-0.1%)      -64 B   (-0.0%)
plugins                     -380 B   (-0.1%)      -38 B   (-0.0%)
purchases                   -339 B   (-0.0%)     +113 B   (+0.0%)
site-blocks                 -314 B   (-0.1%)    -1037 B   (-1.2%)
account                     -303 B   (-0.1%)    -1038 B   (-1.0%)
media                       -273 B   (-0.1%)     +299 B   (+0.2%)
wp-super-cache              -247 B   (-0.1%)      -30 B   (-0.0%)
settings-performance        -247 B   (-0.1%)     +106 B   (+0.1%)
settings-discussion         -247 B   (-0.1%)      -30 B   (-0.0%)
earn                        -247 B   (-0.1%)      -25 B   (-0.0%)
marketing                   -235 B   (-0.0%)     +362 B   (+0.3%)
site-purchases              -234 B   (-0.0%)     +608 B   (+0.3%)
people                      -219 B   (-0.1%)     -842 B   (-0.9%)
home                        -203 B   (-0.0%)     -347 B   (-0.3%)
stats                       -195 B   (-0.0%)      -49 B   (-0.0%)
security                    -163 B   (-0.0%)    -1031 B   (-0.8%)
notification-settings       -163 B   (-0.0%)    -1031 B   (-1.1%)
me                          -163 B   (-0.1%)    -1031 B   (-1.4%)
happychat                   -163 B   (-0.1%)    -1031 B   (-1.2%)
account-close               -163 B   (-0.0%)    -1031 B   (-1.1%)
zoninator                   -151 B   (-0.1%)      -10 B   (-0.0%)
import                      -142 B   (-0.1%)      -93 B   (-0.1%)
domains                     -135 B   (-0.0%)     +145 B   (+0.1%)
pages                       +101 B   (+0.0%)     +142 B   (+0.2%)
devdocs                      -95 B   (-0.0%)       -7 B   (-0.0%)
google-my-business           -80 B   (-0.0%)     +990 B   (+1.1%)
help                         -52 B   (-0.0%)       -1 B   (-0.0%)
email                        +42 B   (+0.0%)     +453 B   (+0.5%)

This should reduce confusion, and more accurately convey the impact of a PR.

@sgomes sgomes requested review from jsnajdr and a team December 9, 2020 18:25
Copy link
Member

@jsnajdr jsnajdr left a comment

Choose a reason for hiding this comment

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

Looks good, just pointing out one suspicious place in the code 馃檪

I pushed an extra commit that declares loop variables in for loops.

}
return acc;
},
{}
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible that the chunksInUse[ index ] array is empty for a particular area? Then the reduce would loop over an empty array and would return an empty object rather than { stat_size: 0, parsed_size: 0, gzip_size: 0 }.

It might be better to do the reduce with a { stat_size: 0, parsed_size: 0, gzip_size: 0 } initial value.

The delta.js module has a nice sumSizesOf helper to sum two size objects together.

@sgomes
Copy link
Contributor Author

sgomes commented Dec 16, 2020

I pushed an extra commit that declares loop variables in for loops.

Oops, good catch! I need to figure out a way of having my IDE warn me about those regardless of the project ESLint config.

@jsnajdr jsnajdr merged commit 50d14f4 into master Dec 16, 2020
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.

None yet

2 participants