Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@chinmaygarde
Copy link
Member

This reverts commit 572fea3.

Fixes flutter/flutter#28184

@dnfield
Copy link
Contributor

dnfield commented Feb 21, 2019

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

@matthew-carroll
Copy link
Contributor

+1 to make this opt-in

@chinmaygarde
Copy link
Member Author

Isn't that WAI? Can we update the baselines instead? We currently leak the VM and all associated data.

@dnfield
Copy link
Contributor

dnfield commented Feb 21, 2019

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.

@matthew-carroll
Copy link
Contributor

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?

@chinmaygarde
Copy link
Member Author

chinmaygarde commented Feb 21, 2019

There is now a Settings.leak_vm flag that is true by default. The unit tests have been updated to turn off this flag before running the lifecycle tests. Once embedders are updated, they can set leak_vm to false and eventually remove the flag.

Clarifications @dnfield:

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.

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:

Has anyone even considered the possible implications of this for state saving solutions on Android?

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.

@matthew-carroll
Copy link
Contributor

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.

@chinmaygarde
Copy link
Member Author

Ah. Right. There has been no investigation done to enumerate the unusual workarounds currently in play to work around the state saving issues.

@chinmaygarde
Copy link
Member Author

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.

@dnfield
Copy link
Contributor

dnfield commented Feb 21, 2019

LGTM.

@dnfield
Copy link
Contributor

dnfield commented Feb 21, 2019

Looks like tests are failing in profile/release mode on the host for this...

@chinmaygarde
Copy link
Member Author

Those test harnesses were not setup for AOT mode. I think you enabled those recently, I'll skip those in profile and release.

@dnfield
Copy link
Contributor

dnfield commented Feb 21, 2019

Should be able to use the SKIP_IF_AOT() macro in there

@chinmaygarde chinmaygarde mentioned this pull request Feb 21, 2019
@chinmaygarde
Copy link
Member Author

Blocked by #7913

@chinmaygarde chinmaygarde force-pushed the reland_shutdown branch 2 times, most recently from 7d251be to 8fc104f Compare February 22, 2019 22:57
@chinmaygarde chinmaygarde force-pushed the reland_shutdown branch 2 times, most recently from 289b199 to 8ec786e Compare March 28, 2019 19:53
@chinmaygarde
Copy link
Member Author

Abandoned in favor of #8397.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants