Shut down and restart the Dart VM as needed. #7832

chinmaygarde · 2019-02-15T02:48:26Z

The shell was already designed to cleanly shut down the VM but it couldnt
earlier as |Dart_Initialize| could never be called after a |Dart_Cleanup|. This
meant that shutting down an engine instance could not shut down the VM to save
memory because newly created engines in the process after that point couldn't
restart the VM. There can only be one VM running in a process at a time.

This patch separate the previous DartVM object into one that references a
running instance of the DartVM and a set of immutable dependencies that
components can reference even as the VM is shutting down.

Unit tests have been added to assert that non-overlapping engine launches use
difference VM instances.

chinmaygarde · 2019-02-15T02:51:58Z

Thanks to @a-siva & @jason-simmons for helping to arrive at this approach. The stuff in dart_vm_lifecycle.cc has the implementation details about thread safety that are important and ought to be closely reviewed.

The shell was already designed to cleanly shut down the VM but it couldnt earlier as Dart_Initialize could never be called after a Dart_Cleanup. This meant that shutting down an engine instance could not shut down the VM to save memory because newly created engines in the process after that point couldn't restart the VM. There can only be one VM running in a process at a time.

runtime/dart_vm_lifecycle.cc

chinmaygarde · 2019-02-15T20:44:32Z

Verified on Android that all memory related to Flutter is reclaimed when the last Flutter view goes away.
This is the memory usage as I kept adding and removing a Flutter view in the application.

a-siva · 2019-02-15T19:02:09Z

runtime/dart_vm_data.cc

@@ -0,0 +1,79 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.


New file, I guess we should move the copyright year to 2019

All our files apparently need to have this preamble. @Hixie knows the details.

a-siva · 2019-02-15T19:02:27Z

runtime/dart_vm_data.h

@@ -0,0 +1,47 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.


a-siva · 2019-02-15T19:06:28Z

runtime/dart_vm_data.h

+
+  const Settings& GetSettings() const;
+
+  const DartSnapshot& GetVMSnapshot() const;


You are treating this snapshot differently and not returning a fml::RefPtr I suppose because it is used only in Dart_Init(). You might be starting a new VM again after deleting the previous VM, is it guaranteed that these calls to Dart_Init will always happen on the same thread?

No. There is no such guarantee. However, all the constructors and destructors of the DartVM are guaranteed to be guarded by a single global mutex. Also, the the new vm_data_ is owned by the DartVM object so the second launch should get the updated VM data.

a-siva · 2019-02-15T19:06:47Z

runtime/dart_vm_lifecycle.cc

@@ -0,0 +1,106 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.


runtime/dart_vm_lifecycle.cc

cbracken

lgtm modulo a couple nits.

runtime/dart_vm_lifecycle.h

runtime/dart_vm_lifecycle.cc

…7832)

flutter/engine@033f207...163a2fd git log 033f207..163a2fd --no-merges --oneline 163a2fd Revert "Android embedding refactor pr3 add remaining systemchannels (#7738)" (flutter/engine#7849) 0d6ff16 Shut down and restart the Dart VM as needed. (flutter/engine#7832) 256db4b Android embedding refactor pr3 add remaining systemchannels (flutter/engine#7738) 02d7ca3 Don't call static method from instance variable (flutter/engine#7841) 10cee61 Delete GL textures when they are released from the texture registry. (flutter/engine#7836) 69e4606 Fix NullPointerException in SurfaceTextureRegistryEntry (flutter/engine#7837) 9a965bc Fix NullPointerException in ResourceCleaner (flutter/engine#7838) 2acd794 Add fml::FileExists implementation for Windows (flutter/engine#7845) fa38a98 Update buildroot to 7f64ff4928e to unblock Mac builds. (flutter/engine#7846) 1ba3295 Add support for calling into other plugins from a background context on iOS (flutter/engine#7843) df2fc97 Roll src/third_party/skia 154acd7a1374..be9aff25bddc (13 commits) (flutter/engine#7842) 0ca1d1f Remove the Dart JIT snapshot data from AOT builds of the embedder library (flutter/engine#7806) c51ea41 Roll src/third_party/skia 7a74c7cb6da0..154acd7a1374 (1 commits) (flutter/engine#7840) ecbaea1 Ensure to pass dill file after VM options for gen_snapshot (flutter/engine#7839) 5d3f714 Roll src/third_party/skia b00f7b34751b..7a74c7cb6da0 (1 commits) (flutter/engine#7835) 7337399 Roll src/third_party/skia 9a88bee122f1..b00f7b34751b (24 commits) (flutter/engine#7831) 6f318ef Move up ndk version that is being downloaded(old one no longer available) (flutter/engine#7830) a8aa1ee Track flow id (flutter/engine#7826) b1ce6b7 Roll src/third_party/skia a0dcd29f536b..9a88bee122f1 (4 commits) (flutter/engine#7825) dbc1663 Roll src/third_party/skia 6152470dc69e..a0dcd29f536b (2 commits) (flutter/engine#7824) 9426776 Roll src/third_party/skia 4037f7f5d8b4..6152470dc69e (1 commits) (flutter/engine#7823) e2394ad Add flow events connecting pointer events to frames (flutter/engine#7807) 0a5a7c5 Roll src/third_party/skia 09c01e9df260..4037f7f5d8b4 (3 commits) (flutter/engine#7821) b867f48 Roll src/third_party/skia 186669c4128b..09c01e9df260 (5 commits) (flutter/engine#7820) af39e22 Fix tests that were committed after cirrus ran (flutter/engine#7819) 1f2cbf7 Roll src/third_party/dart 0a7dcf17eb..c92d5ca288 (64 commits) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff (liyuqian@google.com), and stop the roller if necessary.

This reverts commit 0d6ff16.

This reverts commit 572fea3.

chinmaygarde requested review from cbracken, a-siva and jason-simmons February 15, 2019 02:48

googlebot added the cla: yes label Feb 15, 2019

chinmaygarde mentioned this pull request Feb 15, 2019

Shut down and restart the Dart VM as needed. #6387

Closed

chinmaygarde force-pushed the shutdown_vm branch from 9894648 to d210788 Compare February 15, 2019 20:07

Licenses.

f588aa7

cbracken reviewed Feb 15, 2019

View reviewed changes

runtime/dart_vm_lifecycle.cc Outdated Show resolved Hide resolved

cbracken reviewed Feb 15, 2019

View reviewed changes

runtime/dart_vm_lifecycle.cc Show resolved Hide resolved

cbracken reviewed Feb 15, 2019

View reviewed changes

runtime/dart_vm_lifecycle.cc Outdated Show resolved Hide resolved

Address PR comments.

8c26fcb

a-siva approved these changes Feb 15, 2019

View reviewed changes

cbracken approved these changes Feb 15, 2019

View reviewed changes

jason-simmons reviewed Feb 15, 2019

View reviewed changes

runtime/dart_vm_lifecycle.h Outdated Show resolved Hide resolved

runtime/dart_vm_lifecycle.cc Outdated Show resolved Hide resolved

chinmaygarde added 2 commits February 15, 2019 13:57

PR

1c58131

PR

d0b8d0a

chinmaygarde merged commit 0d6ff16 into flutter:master Feb 15, 2019

chinmaygarde deleted the shutdown_vm branch February 15, 2019 22:16

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 16, 2019

0d6ff16 Shut down and restart the Dart VM as needed. (flutter/engine#…

aca30e4

…7832)

engine-flutter-autoroll mentioned this pull request Feb 16, 2019

Roll engine 033f2072c315..163a2fd5e140 (26 commits) flutter/flutter#28035

Merged

cbracken mentioned this pull request Feb 19, 2019

Free resources after a Flutter view is destroyed flutter/flutter#16995

Closed

dnfield added a commit that referenced this pull request Feb 19, 2019

Revert "Shut down and restart the Dart VM as needed. (#7832)"

2fb8de6

This reverts commit 0d6ff16.

dnfield added a commit that referenced this pull request Feb 20, 2019

Revert "Shut down and restart the Dart VM as needed. (#7832)" (#7877)

572fea3

This reverts commit 0d6ff16.

dnfield mentioned this pull request Feb 20, 2019

Roll engine to f45572e95f93edb89b6750a4f36ad8ed7c0a2560 flutter/flutter#28178

Merged

chinmaygarde added a commit to chinmaygarde/flutter_engine that referenced this pull request Feb 20, 2019

Re-land "Shut down and restart the Dart VM as needed. (flutter#7832)"

a6051f9

This reverts commit 572fea3.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shut down and restart the Dart VM as needed. #7832

Shut down and restart the Dart VM as needed. #7832

chinmaygarde commented Feb 15, 2019 •

edited by cbracken

Loading

chinmaygarde commented Feb 15, 2019

chinmaygarde commented Feb 15, 2019

a-siva Feb 15, 2019

chinmaygarde Feb 15, 2019 •

edited

Loading

a-siva Feb 15, 2019

a-siva Feb 15, 2019

chinmaygarde Feb 15, 2019

a-siva Feb 15, 2019

cbracken left a comment

		@@ -0,0 +1,79 @@
		// Copyright 2013 The Flutter Authors. All rights reserved.

		@@ -0,0 +1,47 @@
		// Copyright 2013 The Flutter Authors. All rights reserved.


		const Settings& GetSettings() const;

		const DartSnapshot& GetVMSnapshot() const;

		@@ -0,0 +1,106 @@
		// Copyright 2013 The Flutter Authors. All rights reserved.

Shut down and restart the Dart VM as needed. #7832

Shut down and restart the Dart VM as needed. #7832

Conversation

chinmaygarde commented Feb 15, 2019 • edited by cbracken Loading

chinmaygarde commented Feb 15, 2019

chinmaygarde commented Feb 15, 2019

a-siva Feb 15, 2019

Choose a reason for hiding this comment

chinmaygarde Feb 15, 2019 • edited Loading

Choose a reason for hiding this comment

a-siva Feb 15, 2019

Choose a reason for hiding this comment

a-siva Feb 15, 2019

Choose a reason for hiding this comment

chinmaygarde Feb 15, 2019

Choose a reason for hiding this comment

a-siva Feb 15, 2019

Choose a reason for hiding this comment

cbracken left a comment

Choose a reason for hiding this comment

chinmaygarde commented Feb 15, 2019 •

edited by cbracken

Loading

chinmaygarde Feb 15, 2019 •

edited

Loading