[kernel/flutter] Improve speed of first hot reload - #13699

The first hot reload is the second upload of the images, no? The app has already been running for a while by then and has fetched images from its asset bundle.

This needs a test. The best test would be good to create a devicelab end-to-end test that verifies that hot reload successfully changes the assets (e.g. you could check if the dimensions of an image change after the image has been changed on disk and a hot reload triggered, checking the dimensions on initial load, after the initial hot reload, and after a second hot reload).

Maybe I didn't explain properly (or I didn't quite understand your comment).

For a normal hot reload scenario, what's happening is this:

• New and changed files are uploaded and (if assets) they're put on a list of assets to evict.
• They are then evicted and the list is cleared. There is thus no 'carry-over' into the next hot reload.

For the initial upload what's happening is this:

• All files are uploaded and (if assets) they're put on a list of assets to evict.
• The list is not used and not cleared. Thus there is a 'carry-over' into the next hot reload.

What should happen - it seems to me - on the initial upload is this:

• All files are uploaded. Nothing is put on the list of assets to evict.

or, alternatively, and what I've changed the PR to be because it seems cleaner:

• All files are uploaded and (if assets) they're put on a list of assets to evict.
• The list of assets to evict is cleared so there is no 'carry-over' into the next hot reload.

This seems reasonable to me:
The initial upload shouldn't trigger the eviction of anything, it's just the initial upload and there really should be nothing to evict. (If there is in fact anything to evict it should happen right away and not after the user asked for a hot reload.)
Anything that changed between the initial upload and the first hot reload will still be re-uploaded, marked as having to be evicted and then be evicted (the normal hot reload scenario). Anything that wasn't changed between the first upload and the first hot reload will not be uploaded again and thus shouldn't be evicted.

I'm all for adding tests, but a) I don't know how tests work in flutter, any pointers would be appreciated, and b) it seems to me that #7302 will block such a test...

There's three kinds of loads:

1. The initial upload.
2. The first hot reload.
3. Subsequent hot reloads.

The first happens when you first call flutter run. The second happens when you first hit r. Images can get cached from the first before the second.

We need to evict everything after the second, unless we know that it hasn't changed between 1 and 2.

cc @aam

We need to evict everything after the second, unless we know that it hasn't changed between 1 and 2.

Sure. But the same applies for between 2 and 3 (where there is no 'carry-over'), and another way to say "everything but unchanged" is to say "changed", and if a file isn't re-uploaded as part of "2" it haven't changed, so it shouldn't be evicted.

If I understand correctly, flutter tools keeps track of changes to the assets after 2, but the way things are now, it doesn't know what changed between 1 and 2 - potentially content of archive created for 1 is outdated by the time 2 is requested. So when 2(first hot reload) is requested, it evicts and forces reload of all assets.

If I understand correctly, flutter tools keeps track of changes to the assets after 2, but the way things are now, it doesn't know what changed between 1 and 2 - potentially content of archive created for 1 is outdated by the time 2 is requested. So when 2(first hot reload) is requested, it evicts and forces reload of all assets.

If that was the case it would also have to re-upload the same assets.

Am I reading this code wrong?

What I'm reading is this:

• Go through all known files
• If it's modified (content.isModified i.e. if it's either a new file or the timestamp is newer) it's added to the dirtyEntries map, and if it's an asset we also add it to the assetPathsToEvict.

Then further down the entries in dirtyEntries are uploaded to the device.

To me that means that one of two things are happening:

1. Either it does keep track of what's changed, and it only uploads changed things; or
2. It doesn't keep track of what's changed and it uploads everything again (content.isModified returns true because it thinks its a new file).

In both cases however, assetPathsToEvict is updated correctly, i.e. it will contain all assets that are re-uploaded, which are also the only assets that make sense to evict as it's the only assets that has changed on the device.

Btw, printing the content of dirtyEntries, what's happening is 1. above. Only changed files are re-uploaded.

First time code quoted above is executed it will add all files and assets to the dirty list, yes.
This is happening in a background on second stage of initial upload when app is already being launched from the uploaded bundle(apk/ipa). None of the uploaded code or assets are actually used until user does hot reload/restart. This is done so that when user requests hot reload they won't have to wait for the whole app to be uploaded to the device, only changed delta will be uploaded at that point.

So what you're saying is that any change to the assets between the packaging of the files into a bundle (apk/ipa) to the initial upload will have to be evicted?

So the process actually looks something like this:

1. APK/IPK build with all data, uploaded to the device and everything from here is used to run the application.
2. "Initial" upload uploads everything, though some things might have changed since the apk was build. They go unused for now.
3. First hot reload triggers the switch-over from everything that was included in the bundle (1) to everything that was first uploaded (2) (incl any changes since then (3)).
4. Subsequent hot reloads uploads new/changed things and evicts those.

So a test that will make this PR fail would be this:

1. Package the APK
2. Update an asset
3. Check that the asset is the one from the APK (i.e. the unchanged one)
4. Upload everything (first upload) (with the changed asset)
5. Perform hot reload (with this PR that would equal no evict)
6. Confirm that the asset uploaded in (4) is also the one used (which would then fail with this PR because it wasn't evicted).

And a way to change this PR to pass the above-sketched test would be to instead:

1. Record the time
2. Package the APK
3. Upload everything (first upload) (record things as having to be evicted iff changed after the time recorded in (1)).

I.e. instead of clearing the evict list after uploading everything (as this PR currently does), then in the first place only record things as having to be evicted if they have changed since the first-recorded time?

Does that sound reasonable?

I.e. instead of clearing the evict list after uploading everything (as this PR currently does), then in the first place only record things as having to be evicted if they have changed since the first-recorded time?

Yeah, something along these lines. This was recently mentioned in the context of hot reload initial sync getting slower here: dart-lang/sdk#31669 (comment). There is also #7946 for this which has more ideas and discussions on this topic.

@jensjoha Are you planning on making the updates you suggested? Should we close the PR in the meantime to keep it off the "pending review" queue?

I made a first stab at the suggested algorithm.

I here include some timings on a real device (Nexus 4), though on a non-updated flutter/engine. I will update and redo them...

Now (--preview-dart-2):
First: 1,626ms.
Second: 897ms.
Third: 897ms.
fourth: 889ms.

Now (without --preview-dart-2):
First: 8,731ms.
Second: 837ms.
Third: 809ms.
fourth: 853ms.

Before (--preview-dart-2):
First: 6,406ms.
Second: 969ms.
Third: 881ms.
fourth: 941ms.

Before (without --preview-dart-2):
First: 12,979ms.
Second: 853ms.
Third: 797ms.
fourth: 801ms.

Timings after rebase:

Nexus 4, preview dart 2, with vs without

First: 1,914ms. vs 6,606ms. (~29%)
Second: 898ms.vs 881ms.
Third: 834ms. vs 930ms.
fourth: 946ms.vs 966ms.

Nexus 4, without preview dart 2, with vs without

First: 9,174ms. vs 13,254ms. (~69%)
Second: 850ms. vs 830ms.
Third: 822ms. vs 794ms.
fourth: 869ms. vs 847ms.

x64 Android emulator, preview dart 2, with vs without

First: 747ms. vs 2,002ms. (~37%)
Second: 594ms. vs 590ms.
Third: 538ms. vs 638ms.
fourth: 558ms. vs 542ms.

x64 Android emulator, without preview dart 2, with vs without

First: 1,481ms. vs 1,840ms. (~80%)
Second: 461ms. vs 430ms.
Third: 497ms. vs 469ms.
fourth: 432ms. vs 392ms.

Any new thoughts on this?

Sorry, I'm not following how you record the time when APK was built.
Basically when you do flutter run, it does flutter build internally which results in APK built from initial set of sources and assets, then there is a gap until sources/assets are fully transferred to the device to be used for subsequent hot reload. In order to optimize asset eviction we need to ensure that timestamp used by flutter build and timestamp used for transfers are the same.

This change basically records the time before doing anything else (https://github.com/flutter/flutter/pull/13699/files#diff-e78651ad5bd5c46811f2b76bc465220bR217).
It then uses that time to decide whether assets needs to be evicted after the first upload (https://github.com/flutter/flutter/pull/13699/files#diff-2d3eb6a9612741b5ca2fe16872fddc9aR472).

This way any change made to an asset in the gap between apk build and initial upload will have a newer timestamp upon upload, thus being added to the list of assets to evict.

Ah, thanks for the explanation. lgtm.

Merge conflicts fixed.
Assuming the "lgtm" was for the PR will you merge it? (I have to permissions and so can't do it).

Somehow I don't see any impact on hot_mode_dev_cycle__preview_dart_2 benchmarks on https://flutter-dashboard.appspot.com/benchmarks.html

The benchmarked data is not the first hot reload. It is basically the second hot reload (it might be a hot reload after a hot restart) - but the first hot reload is not measured in the benchmark.
It is surely a measure it would make sense adding though.

If you manually do a "flutter run --preview-dart-2" and press "r" you should definitely see the difference on the first hot reload (compared to before).

Got you, thanks!