Skip to content

Commit

Permalink
Send RPC request to switch assets directory on hot reload. (#12872)
Browse files Browse the repository at this point in the history
* Send RPC request to switch assets directory on hot reload.

This is needed to pick up updated assets that are expected to be picked up on hot reload.

* Assert assets directory is not null.

* Better multiple future wait

* Add type annotation
  • Loading branch information
aam committed Jan 4, 2018
1 parent 885e969 commit 8da5af5
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 0 deletions.
9 changes: 9 additions & 0 deletions packages/flutter_tools/lib/src/resident_runner.dart
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,15 @@ class FlutterDevice {
return reports;
}

Future<Null> resetAssetDirectory() async {
final Uri deviceAssetsDirectoryUri = devFS.baseUri.resolveUri(
fs.path.toUri(getAssetBuildDirectory()));
assert(deviceAssetsDirectoryUri != null);
await Future.wait(views.map(
(FlutterView view) => view.setAssetDirectory(deviceAssetsDirectoryUri)
));
}

// Lists program elements changed in the most recent reload that have not
// since executed.
Future<List<ProgramElement>> unusedChangesInLastReload() async {
Expand Down
2 changes: 2 additions & 0 deletions packages/flutter_tools/lib/src/run_hot.dart
Original file line number Diff line number Diff line change
Expand Up @@ -516,6 +516,8 @@ class HotRunner extends ResidentRunner {
int countExpectedReports = 0;
for (FlutterDevice device in flutterDevices) {
// List has one report per Flutter view.
await device.resetAssetDirectory();

final List<Future<Map<String, dynamic>>> reports = device.reloadSources(
entryPath,
pause: pause
Expand Down
9 changes: 9 additions & 0 deletions packages/flutter_tools/lib/src/vmservice.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1318,6 +1318,15 @@ class FlutterView extends ServiceObject {
await subscription.cancel();
}

Future<Null> setAssetDirectory(Uri assetsDirectory) async {
assert(assetsDirectory != null);
await owner.vmService.vm.invokeRpc('_flutter.setAssetBundlePath',
params: <String, dynamic>{
'viewId': id,
'assetDirectory': assetsDirectory.toFilePath(windows: false)
});
}

bool get hasIsolate => _uiIsolate != null;

Future<Null> flushUIThreadTasks() async {
Expand Down

10 comments on commit 8da5af5

@aam
Copy link
Member Author

@aam aam commented on 8da5af5 Jan 5, 2018

Choose a reason for hiding this comment

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

According to flutter benchmarks this PR seems to have regressed hot_mode_dev_cycle__benchmark hotReloadVMReloadMilliseconds significantly (from ~25 ms to ~472 ms).
I've tried reproducing this regression locally(linux with android emulator), but could not. On this PR I get similar hotReloatVMReloadMilliseconds numbers:

on this commit 8da5af55c

Task result:
{
  "success": true,
  "data": {
    "hotReloadInitialDevFSSyncMilliseconds": 5533,
    "hotRestartMillisecondsToFrame": 1342,
    "hotReloadMillisecondsToFrame": 498,
    "hotReloadDevFSSyncMilliseconds": 312,
    "hotReloadFlutterReassembleMilliseconds": 173,
    "hotReloadVMReloadMilliseconds": 12,
    "hotReloadDevFSSyncMillisecondsAfterChange": 371,
    "hotReloadFlutterReassembleMillisecondsAfterChange": 154,
    "hotReloadVMReloadMillisecondsAfterChange": 152
  },
═══════════╡ ••• Finished task "hot_mode_dev_cycle__benchmark" ••• ╞════════════
on previous commit 885e96914
Task result:
{
  "success": true,
  "data": {
    "hotReloadInitialDevFSSyncMilliseconds": 5464,
    "hotRestartMillisecondsToFrame": 1428,
    "hotReloadMillisecondsToFrame": 536,
    "hotReloadDevFSSyncMilliseconds": 344,
    "hotReloadFlutterReassembleMilliseconds": 183,
    "hotReloadVMReloadMilliseconds": 8,
    "hotReloadDevFSSyncMillisecondsAfterChange": 265,
    "hotReloadFlutterReassembleMillisecondsAfterChange": 157,
    "hotReloadVMReloadMillisecondsAfterChange": 120
  },
═══════════╡ ••• Finished task "hot_mode_dev_cycle__benchmark" ••• ╞════════════

@yjbanov , any ideas on how the numbers on the dashboard could be so different from what you get locally?

@yjbanov
Copy link
Contributor

@yjbanov yjbanov commented on 8da5af5 Jan 5, 2018

Choose a reason for hiding this comment

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

I can't think of anything. One thing that gives this regression credibility is that it is happening across different devicelab agent profiles: Linux and Windows. So it seems unlikely that it's caused by something going on in the hardware.

@aam
Copy link
Member Author

@aam aam commented on 8da5af5 Jan 5, 2018

Choose a reason for hiding this comment

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

hot_mode_dev_cycle_linux__benchmark and hot_mode_dev_cycle__preview_dart_2_benchmark don't look reasonable - they fluctuate wildly feeding concerns that something else is going on with benchmarking tool or hardware:

majnwnjcgwk

Is there some way to troubleshoot those? Run them manually on the testing infrastructure?

@Hixie
Copy link
Contributor

@Hixie Hixie commented on 8da5af5 Jan 5, 2018

Choose a reason for hiding this comment

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

We've seen that kind of thing before, where once you go past a certain threshold of runtime / memory usage / number of items in some hash table / whatever, you cut into a different codepath and hit a performance cliff; when the codebase is right on the edge of that boundary, you then see this effect, where some runs are fast and some are slow. What hardware or OS you're testing on obviously can impact this in many ways. The regressions are still "real" in the sense that it's a problem in our code, usually, it's just that we're seeing the bimodal behaviour of our code at that boundary.

@yjbanov can hook you up with devices to run locally for trouble-shooting this. I would start by getting a device, since it's unlikely the host is the cause if it's happening across both Windows and Linux.

In the meantime we should probably revert the change so that we don't miss any other regressions.

@aam
Copy link
Member Author

@aam aam commented on 8da5af5 Jan 5, 2018

Choose a reason for hiding this comment

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

Sure we can revert, but how can we re-land it if we are unable to reproduce the regression locally?

Other thing is that this hotReloadVMReloadMilliseconds benchmark is part of hotReloadMillisecondsToFrame benchmark, where essentially hotReloadMillisecondsToFrame=hotReloadDevFSSyncMilliseconds+hotReloadVMReloadMilliseconds
+hotReloadFlutterReassembleMilliseconds.

Looking at how hotReloadFlutterReassembleMilliseconds went down, while hotReloadVMReloadMilliseconds went up, so that total hotReloadMillisecondsToFrame regressed slightly(which is reasonable considering that we are making one more rpc call) points to some kind of issue with attribution of time spent to the right step:
v1stgk4fsxg

I also have #13934 which should only make this additional RPC call on initial reload/restart only.

@Hixie
Copy link
Contributor

@Hixie Hixie commented on 8da5af5 Jan 6, 2018

Choose a reason for hiding this comment

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

Sure we can revert, but how can we re-land it if we are unable to reproduce the regression locally?

We clearly need to be able to reproduce it, but that should definitely be possible since there's hardware physically present in our building that's reproducing it already.

some kind of issue with attribution of time

Interesting, maybe there's a race condition in how we time this stuff?

@yjbanov
Copy link
Contributor

@yjbanov yjbanov commented on 8da5af5 Jan 6, 2018

Choose a reason for hiding this comment

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

We have a couple of devices on the device rack you could grab from. I have one on my desk. If that doesn't help, we could just walk upstairs to the lab and try to reproduce on the lab hardware. I'm wondering if things like USB speed/latency can affect these numbers.

@aam
Copy link
Member Author

@aam aam commented on 8da5af5 Jan 6, 2018

Choose a reason for hiding this comment

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

Are we running these benchmarks on G4? I would imagine we use different devices for hot_mode_dev_cycle_win__benchmark/hot_mode_dev_cycle__benchmark and hot_mode_dev_cycle_linux__benchmark since they show different behavior on this benchmark.

I was unsuccessfully trying to reproduce this on emulator and on Pixel.

@yjbanov
Copy link
Contributor

@yjbanov yjbanov commented on 8da5af5 Jan 6, 2018

Choose a reason for hiding this comment

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

We use Moto G4 for everything.

@aam
Copy link
Member Author

@aam aam commented on 8da5af5 Jan 8, 2018

Choose a reason for hiding this comment

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

This reproduces on Moto G4. The variation in performance I would imagine is due to the fact that we post messages on UI thread to process RPC call on the device and message/queue processing is inherently unpredictable.

The RPC call is needed, so performance hit is expected. We don't need to make RPC call every time though, that is what #13934 addresses.

#13934 also "fixes" the benchmark regression because in our benchmarks hot reload happens after restart and with this PR RPC call won't be issued because restart have switched to running from sources.

Please sign in to comment.