-
Notifications
You must be signed in to change notification settings - Fork 6k
Reland ”Shut down and restart the Dart VM as needed." #7905
Conversation
|
Can we make this API opt in for the time being? It creates a performance regression on Android (and potentially iOS, although I haven't tested that). /cc @matthew-carroll |
|
+1 to make this opt-in |
|
Isn't that WAI? Can we update the baselines instead? We currently leak the VM and all associated data. |
|
Right now if you hit the back button on android, the engine stays around. With this change, if you hit the back button on Android, the engine goes away. |
|
I think this needs to be opt-in until we have had sufficient opportunity to update embeddings in a way that either doesn't apply to existing apps, or intelligently applies this so that no negative effects result. Has anyone even considered the possible implications of this for state saving solutions on Android? |
|
There is now a Clarifications @dnfield:
That is not quite correct. Hitting the back button destroys the engine. This has not changed. It used to be that the second engine did not have to pay the price of starting the VM back up. Without leaking the engine, going back to the application after pressing the back button has to launch the VM again. If you want to get back the old behavior without leaking the VM, hold onto the engine when the application is backgrounded. With this change, hitting the back button and coming back to the application is exactly as expensive as starting the application the first time around. Clarifications @matthew-carroll:
This patch has no implications to state saving because we don't save state today. If isolate state needs to be maintained, you can keep the shell around while the application is backgrounded (but the process still running). This will keep the VM around as well. If the process is killed, the new shell will need to launch the VM. In that case startup is more involved as well and will likely need solving some of the issues listed in flutter/flutter#6827. |
|
What I meant about state saving is that because we do not save state today, developers out there may be doing a number of unusual things for the purposes of saving state. We would not want to inadvertently break that work. And any such work is probably being done at the Java level via various lifecycle hooks, not anything as low level as the native code. |
|
Ah. Right. There has been no investigation done to enumerate the unusual workarounds currently in play to work around the state saving issues. |
|
CC @Hixie It seems like deciding not to leak the VM requires more deliberation w.r.t its effect on existing embedders. Not sure how to proceed on this. W.r.t the engine changes in this patch, the engine can now cleanup the VM and tests same, and, gives embedders the opportunity to leak the vm to get back the old behavior. May I get the LGTM to land this patch. |
|
LGTM. |
|
Looks like tests are failing in profile/release mode on the host for this... |
|
Those test harnesses were not setup for AOT mode. I think you enabled those recently, I'll skip those in profile and release. |
|
Should be able to use the |
|
Blocked by #7913 |
7d251be to
8fc104f
Compare
289b199 to
8ec786e
Compare
This reverts commit 572fea3. Fixes flutter/flutter#28184
8ec786e to
305141e
Compare
|
Abandoned in favor of #8397. |
This reverts commit 572fea3.
Fixes flutter/flutter#28184