🏗 Stop reporting gzipped compressed size in the bundle-size task

[danielrozenberg]

Fixes #21275

Step 3.i. in #21275 (comment)

[danielrozenberg]

This is weird. This PR should not change the bundle-size. @rsimha could this be some git/github merge issue?

[rsimha]

This is weird. This PR should not change the bundle-size. @rsimha could this be some git/github merge issue?

Weird indeed. I did a sanity check. The Travis job for this PR uses 2ee0c08 as its baseline (bundle size is 72.18KB). The brotli size for this PR is 72.13KB even though there are zero runtime changes.

@jridgewell any way to explain this reduction in brotli size in the absence of runtime code changes?

[jridgewell]

Newer brotli installed?

[danielrozenberg]

They're two hours apart...

[jridgewell]

🤷‍♂️

[danielrozenberg]

Oh oh... brotli sizes aren't consistent at all - they keep changing between master builds, even when two consecutive builds only change extensions or docs.

I should have checked this in advance. I'm going to revert the Bundle-Size bot to use gzip for now, which remains consistent, and investigate tomorrow

[rsimha]

Oh oh... brotli sizes aren't consistent at all - they keep changing between master builds, even when two consecutive builds only change extensions or docs.

This is unfortunate ☹️ Are you seeing different sizes when you run brotli-size multiple times on the same SHA?

Paging @erwinmombay, author of brotli-size to see if there's a way to make it return a consistent size for the same code.

[erwinmombay]

we're using nodeJS' built in zlib library's zlib.BrotliCompress function and brotli-size is merely a wrapper so im not sure what's going on. @danielrozenberg could you link me to two master builds that have different brotli sizes? i can download the v0.js file from travis correct?

[rsimha]

I did some experimentation, and it turns out that changing the RTV string in v0.js changes the brotli size, even if there are no other code changes.

In the example below, we're computing the gzipped and brotli bundle size of v0.js for two commits, where the only difference is the RTV string. The gzipped size remains unchanged, but the brotli size swings by 50 bytes. (During other tries, the swing was as high as 100 bytes.)

I tried passing {mode: 1} to brotliSize.fileSync() to indicate that we're compressing a text file, but there still was a size fluctuation when the RTV string changed.

$ gulp bundle-size --on_local_build
[16:56:26] Using gulpfile ~/src/amphtml/gulpfile.js
[16:56:26] Starting 'bundle-size'...
[16:56:26] Computing bundle size for version 1909102024530 at commit fa43674.
[16:56:26] Running npx bundlesize -f "./dist/v0.js"...
[16:56:31] Bundle size (gzipped) is 84.41KB
[16:56:31] Bundle size (brotli) is 72.09KB
[16:56:31] Finished 'bundle-size' after 4.97 s

$ gulp bundle-size --on_local_build
[16:58:21] Using gulpfile ~/src/amphtml/gulpfile.js
[16:58:21] Starting 'bundle-size'...
[16:58:21] Computing bundle size for version 1909102056380 at commit cc3aa51.
[16:58:21] Running npx bundlesize -f "./dist/v0.js"...
[16:58:25] Bundle size (gzipped) is 84.41KB
[16:58:26] Bundle size (brotli) is 72.14KB
[16:58:26] Finished 'bundle-size' after 4.69 s

[kristoferbaxter]

This appears correct to me, the size of the resource should change when the contents shift.

If we're looking for an apples - apples comparison, we should inject the same values into the resulting files.

[rsimha]

we should inject the same values into the resulting files.

Chatted offline with @jridgewell and came up with the idea of making a copy of v0.js, normalizing it by replacing the RTV with a constant string, and computing the brotli size. There will still be size fluctuations due to other changes that ideally shouldn't change the reported size (say, if someone fixes a typo in an error string), but the hope is that these are rare, and will be under the size increase threshold.

One invariant we can guarantee by doing this is that if the code is the same, the size is the same.

@danielrozenberg how easy will it be to retrofit this into gulp bundle-size?

[kristoferbaxter]

say, if someone fixes a typo in an error string

@alanorozco how close are we to replacing all error strings with integernumeric representations?

Once this is resolved, error strings shouldn't be included in production output.

Edit: integer -> numeric

[rsimha]

Once this is resolved, error strings shouldn't be included in production output.

Good to know. My original point still remains though. For instance, I just discovered that changing a single instance of the number 42 in v0.js to 41 causes a 50 byte swing in brotli size.

I'm concerned this will be annoying when (if? 😃) we deploy brotli as the default means of verifying bundle size increases.

[danielrozenberg]

we should inject the same values into the resulting files.

Chatted offline with @jridgewell and came up with the idea of making a copy of v0.js, normalizing it by replacing the RTV with a constant string, and computing the brotli size. There will still be size fluctuations due to other changes that ideally shouldn't change the reported size (say, if someone fixes a typo in an error string), but the hope is that these are rare, and will be under the size increase threshold.

One invariant we can guarantee by doing this is that if the code is the same, the size is the same.

@danielrozenberg how easy will it be to retrofit this into gulp bundle-size?

This seems like a semi-reasonable idea. There still remains the issue that the real served size will fluctuate because the RTV number will be different in reality, but this check isn't about absolute sizes, it's about relative sizes.

I think it shouldn't be a big deal to make this happen, I'll give it a quick attempt today and report back. Other than the RTV, are there any other values that change based on unpredictable factors? Once we start collecting data about extension files as well that could become a relevant question

[rsimha]

Right now, it's just the RTV. Thanks for making the change.

[rsimha]

Blocking review so this isn't mistakenly merged. Feel free to dismiss when the time comes 😃

[rsimha]

@danielrozenberg Seems like this is breaking master?

https://travis-ci.org/ampproject/amphtml/jobs/592809416#L519

[danielrozenberg]

Oops, another data migration plan gone wrong... @rsimha can you please revert this for me and I'll fix it later today?

…

On Wed., Oct. 2, 2019, 8:17 p.m. Raghu Simha, ***@***.***> wrote: @danielrozenberg <https://github.com/danielrozenberg> Seems like this is breaking master? https://travis-ci.org/ampproject/amphtml/jobs/592809416#L519 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#24418?email_source=notifications&email_token=AAOBE6QM7P2X3QL75ZT4N3LQMU23DA5CNFSM4IVAE4M2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAGSOUI#issuecomment-537732945>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOBE6RPYOWKE7Y774YJXR3QMU23DANCNFSM4IVAE4MQ> .