When sending to device, use gzip compression level 1 instead of the default (6) by jensjoha · Pull Request #52666 · flutter/flutter

jensjoha · 2020-03-16T14:31:56Z

Description

When sending files to the device it is first compressed with gzip in an effort to make the whole thing go faster.
As detailed here: dart-lang/sdk#41048 gzip encoding is pretty slow, and in all but cases where the transfer speed is very slow, it would be better to spend less time compressing, even if it means sending more data.

This change - tested on flutter gallery on an android emulator on my google issue desktop - has the following characteristics:

Before this change the time spend transferring to device (via this mechanism) was (timings in ms):

Initial transfer	13.5 MB partial dill transfer
1551	474
1567	476
1679	484
1632	470
1526	517

(Note for reproduction: The 13.4 MB partial dill file is the dill generated when changing package flutters "lib/src/widgets/text.dart".)

After this change the times are:

Initial transfer	13.5 MB partial dill transfer
1580	182
1510	187
1493	184
1443	172
1422	182

For the initial transfer that's (difference at 95.0% confidence): -101.4 +/- 91.0318 (-6.37335% +/- 5.72167%) ---- i.e. almost unchanged.

For the partial dill that's (difference at 95.0% confidence): -302.8 +/- 20.4702 (-62.5361% +/- 4.22764%) ---- i.e. at lot faster.

Digging into why the initial transfer is almost not different:

The dill isn't included in this, only assets (222 of them) - the dill is likely part of the apk that's installed via adb
The average (uncompressed file size) is ~161 KB; the median is ~59 KB
The compressed (original) output size is ~31.76 MB; the compressed (changed) output size is ~31.91 MB (~152 KB more)
It does this in parallel (it seems 6 at a time): Compressing it single-threaded would take (original) 6.2 seconds; (changed) 6.0 seconds (~0.2 seconds less)
Combined, it is probably ~33 ms faster (200 ms / 6 "threads") to compress with the changed scheme, but it has to send ~150 KB more.

The change for the partial dill file is consistent with the calculations on dart-lang/sdk#41048

Note (and again): This was tested on an emulator, a real device connected via either usb2, usb3, wifi or whatever else will probably behave differently. From the above though, I think the conclusion can be that it doesn't matter for the initial transfer, and unless the transfer speed is very slow, this change is for the better (I get that saving these 300 ms is not changing the fact that compiling the 13.5 MB file takes several seconds, but shaving of 300 ms is still something like an overall improvement of ~8% for the entire hot-reload).

Related Issues

dart-lang/sdk#41048
dart-lang/sdk#34001

Tests

No tests have been added or changed.

fluttergithubbot · 2020-03-16T14:31:59Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

jonahwilliams · 2020-03-16T16:47:53Z

I tested this change locally on a mac pro + physical android device. Without this change, a hot reload to text.dart takes 3,940ms. Afterwards it takes 3,573ms.

On a smaller change in home.dart I get times of about 420-450 ms before, 400-420ms after.

packages/flutter_tools/lib/src/devfs.dart

zanderso

I think we need to make an effort to add tests for changes like this. In particular for this change, the implementation of contentAsGzip1CompressedStream() can be moved to base/os.dart, which a test can then mock out and verify is called.

jensjoha · 2020-03-17T08:19:28Z

I think we need to make an effort to add tests for changes like this. In particular for this change, the implementation of contentAsGzip1CompressedStream() can be moved to base/os.dart, which a test can then mock out and verify is called.

I'm thinking you have probably read this wrong. Anyway, after implementing the feedback from @jonahwilliams the method doesn't exist anymore.

zanderso · 2020-03-17T19:20:11Z

I think we need to make an effort to add tests for changes like this. In particular for this change, the implementation of contentAsGzip1CompressedStream() can be moved to base/os.dart, which a test can then mock out and verify is called.

I'm thinking you have probably read this wrong. Anyway, after implementing the feedback from @jonahwilliams the method doesn't exist anymore.

There is currently no test that verifies that the stream is compressed.

This line of code:

return contentsAsStream().cast<List<int>>().transform<List<int>>(gzip.encoder);

should be abstracted out in a new method in OperatingSystemUtils. Then we can verify that the new method is called by DevFS through a mock of OperatingSystemUtils in the tests in devfs_test.dart.

If it is important that the compression level stay at level 1, then the GZipCodec can be exposed from OperatingSystemUtils as @visibleForTesting, and a test in os_utils_test.dart can query the level getter of the GZipCodec. The test and/or a comment on the GZipCodec member of OperatingSystemUtils can explain why the value was chosen, point to github issues, etc.

jensjoha · 2020-03-18T10:28:59Z

I think we need to make an effort to add tests for changes like this. In particular for this change, the implementation of contentAsGzip1CompressedStream() can be moved to base/os.dart, which a test can then mock out and verify is called.

I'm thinking you have probably read this wrong. Anyway, after implementing the feedback from @jonahwilliams the method doesn't exist anymore.

There is currently no test that verifies that the stream is compressed.

That's true, but this PR does not introduce compression, and that there's no testing of it is a pre-existing issue not made worse by this PR.

This line of code:
return contentsAsStream().cast<List<int>>().transform<List<int>>(gzip.encoder);
should be abstracted out in a new method in OperatingSystemUtils. Then we can verify that the new method is called by DevFS through a mock of OperatingSystemUtils in the tests in devfs_test.dart.

That would take a very small and basically risk free change, turn it bigger and increase it's risk.
It would obscure what this change actually does by introducing a refactor together with a concise and well documented performance improvement, as well as increasing the risk of new bugs --- all in the name of testing more.
I understand - and agree - that testing is generally good, but this request is out of scope for this change.

zanderso · 2020-03-18T15:39:06Z

Please see (5) under our Tree hygine polices here: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview.

As instructed above in the automated message from fluttergithubbot, if you feel this PR needs an exemption from the policy, please at-mention hixie on this PR. Thanks!

jensjoha · 2020-03-18T15:50:36Z

@Hixie

jensjoha · 2020-03-24T13:43:00Z

Ping @Hixie

zanderso · 2020-03-24T22:22:16Z

I spoke with Hixie about the procedure for getting an exception. He recommended that contacting him on the Flutter discord would be more effective. He's also working on updating the bot text.

jonahwilliams · 2020-04-09T17:43:38Z

Since the PR that @zanderso linked has landed I'm going to close this PR. Thanks for identifying this fix @jensjoha !

[flutter] Compress at leavel 1 instead of the default (6)

4cbbc8f

fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label Mar 16, 2020

googlebot added the cla: yes label Mar 16, 2020

jensjoha requested review from aam, jonahwilliams and zanderso and removed request for jonahwilliams and zanderso March 16, 2020 14:32

jonahwilliams reviewed Mar 16, 2020

View reviewed changes

packages/flutter_tools/lib/src/devfs.dart Outdated Show resolved Hide resolved

aam approved these changes Mar 16, 2020

View reviewed changes

zanderso requested changes Mar 16, 2020

View reviewed changes

jensjoha added 2 commits March 17, 2020 08:51

Feedback from jonahwilliams

ab1ed4a

Cleaned up

d743d42

jensjoha requested review from jonahwilliams and zanderso March 17, 2020 08:20

zanderso mentioned this pull request Apr 8, 2020

[flutter_tools] Use gzip level 1 for devfs transfer compression #54123

Merged

1 task

jonahwilliams closed this Apr 9, 2020

github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021

jensjoha deleted the use_worse_compression branch August 18, 2022 13:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

When sending to device, use gzip compression level 1 instead of the default (6)#52666

When sending to device, use gzip compression level 1 instead of the default (6)#52666
jensjoha wants to merge 3 commits intoflutter:masterfrom
jensjoha:use_worse_compression

jensjoha commented Mar 16, 2020

Uh oh!

fluttergithubbot commented Mar 16, 2020

Uh oh!

jonahwilliams commented Mar 16, 2020

Uh oh!

Uh oh!

zanderso left a comment

Uh oh!

jensjoha commented Mar 17, 2020

Uh oh!

zanderso commented Mar 17, 2020

Uh oh!

jensjoha commented Mar 18, 2020

Uh oh!

zanderso commented Mar 18, 2020

Uh oh!

jensjoha commented Mar 18, 2020

Uh oh!

jensjoha commented Mar 24, 2020

Uh oh!

zanderso commented Mar 24, 2020

Uh oh!

jonahwilliams commented Apr 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

jensjoha commented Mar 16, 2020

Description

Related Issues

Tests

Uh oh!

fluttergithubbot commented Mar 16, 2020

Uh oh!

jonahwilliams commented Mar 16, 2020

Uh oh!

Uh oh!

zanderso left a comment

Choose a reason for hiding this comment

Uh oh!

jensjoha commented Mar 17, 2020

Uh oh!

zanderso commented Mar 17, 2020

Uh oh!

jensjoha commented Mar 18, 2020

Uh oh!

zanderso commented Mar 18, 2020

Uh oh!

jensjoha commented Mar 18, 2020

Uh oh!

jensjoha commented Mar 24, 2020

Uh oh!

zanderso commented Mar 24, 2020

Uh oh!

jonahwilliams commented Apr 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants