-
Notifications
You must be signed in to change notification settings - Fork 6k
Peformance improvement : reduce engine init time 50%off at least #17192
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
1 similar comment
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
1 similar comment
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
d1aabd7
to
9fd15dc
Compare
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
1 similar comment
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
1 similar comment
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
326a1c4
to
7c4d86b
Compare
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
1 similar comment
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
1 similar comment
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
44e8981
to
1ef74bf
Compare
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
@lucky-chen : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good start and this model is definitely something we want to get to but there are some concerns we need to address first.
All Flutter engine APIs assume that the shell is ready to use after a valid reference to the same has been obtained via a call to Shell::Create
. Now, depending on whether the Shell::Create
call was made with the async_init_callback
, all components that have access to the shell now have to wait for the callback to run. We want an API that doesn’t put the burden on the owner of a shell reference to know when said reference is safe to use. For example, if I were to vend a reference to the shell created using this callback specified to a component that needed its engine reference (by calling Shell::GetEngine
), that component would have no idea if and how long to wait till it's safe to call GetEngine
(or really, any other call on the shell). In fact, more worryingly, since the engine weak pointer has not been initialized yet, the call to GetEngine
will now return a null weak pointer which will make the caller think that the shell has died already! As you can see, this API breaks some invariants in the Flutter Engine.
I think we can achieve the improvements without breaking the invariant described above though. Instead of adding just another argument to the Shell::Create
call and vending a semi-instantiated shell, consider an API that just returns the shell itself in the callback. Now, shell initialization can begin but the reference will be vended to the caller only when it is safe to use. The caller can’t work on a semi-initialized engine because it won’t have a reference to it. The API would look something like. bool CreateShell(std::function<std::unique_ptr<Shell>()> shell_callback, … other arguments)
. When updating the iOS and Android embeddings, you can wait for the entire shell reference in the callback. This should not be an issue as it was not safe to use the shell reference before the callback anyway.
Also, please add a test for this in the shell_unittests
target as that is the primary test target for this component.
@kangwang1988 I already left a review and see no updates or followup comments. Would you like me to clarify something? |
@chinmaygarde |
852fe7e
to
4acbe5e
Compare
hi,all, update finish . benchmark@liyuqian add benchmarks on
ShellTestCase@chinmaygarde add testcase on
Introduction changeCause there are 15 files change, so Introduction to changes. In fact , core modification all in
ApiUsage
|
4acbe5e
to
9e36331
Compare
- add Shell::CreateAsync mode - supprot android && ios - add testcase for engine、Android、iOS - add benchmamrk on shell_benchmark.cc
9e36331
to
3fc6fb4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for adding more benchmarks and tests!
A quick question: do you only expect the 50% init time reduction on add-to-app, or do you also expect such reduction on standalone Flutter Android/iOS apps?
Here's why I'm asking that:
While BM_ShellInitializationAsyncBlockTime
and BM_ShellInitializationAsyncTotalTime
are certainly nice to have, they don't tell a full story of your improvement. Specifically, those two benchmarks can't be run before your PR, so we couldn't use them to quantify your speedup.
Therefore, I tried to use our complex_layout__start_up
test to compare the startup time before and after your PR, but I can't see a significant difference. You can run that test locally by running ../../bin/cache/dart-sdk/bin/dart bin/run.dart -t complex_layout__start_up --local-engine=android_profile
inside flutter/flutter/dev/devicelab once you get your local engine compiled in profile mode.
I'd encourage you to write an independent benchmark like complex_layout__start_up
to reveal your improvement. You can write a smaller PR to just include that benchmark. Smaller PRs also make it much easier for code reviews. As a result, your change will be approved and get merged faster 😄
if (asyncInitCallback == null) { | ||
Log.w( | ||
TAG, | ||
"initAsync: callback is null, you should care lifecycle, and called other api after initCallback called"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the Chinese version of this message? I'm having a little difficulty in understanding the English...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
在异步模式中您需要关心初始化完成的时机,建议设置callback,并在callback触发后再调用其它api。😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the following warning is a better translation: "initAsync: asyncInitCallback is null. Please be careful about the lifecycle and when the initialization is done. We recommend to set an asyncInitCallback, and call other APIs after such callback is executed."
add to app/StandaloneJust for benchmark:Should compare
storyWhen using in an add2app scenario where flutter page will not be triggered until some user operations. Reduce time of app startUp, make homepage display as soon as possible
complex_layout__start_up PRWill create another pr of asyncstartup benchmarks like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @lucky-chen for the clarification!
Since this is specific to add-to-app, let's loop @gaaclarke into the code review as he used to work on add-to-app, and landed a similar PR that parallelize the engine initialization (#10182).
if (asyncInitCallback == null) { | ||
Log.w( | ||
TAG, | ||
"initAsync: callback is null, you should care lifecycle, and called other api after initCallback called"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the following warning is a better translation: "initAsync: asyncInitCallback is null. Please be careful about the lifecycle and when the initialization is done. We recommend to set an asyncInitCallback, and call other APIs after such callback is executed."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this PR is perilous as-is because when creating the shell asynchronously there are no guarantees that the shell wont' be used anyway.
I think you what you've done here is good but could be better if we changed it a bit. Instead of creating 2 separate ways to make a shell, make creation always async and instead of relying on a callback to notify people when it's done, use a std::future. That way we can guarantee that the shell exists before it is used, no one can get it wrong.
@@ -166,6 +168,13 @@ class Shell final : public PlatformView::Delegate, | |||
CreateCallback<PlatformView> on_create_platform_view, | |||
CreateCallback<Rasterizer> on_create_rasterizer); | |||
|
|||
static void CreateAsync(ShellCreateCallback callBack, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a docstring. Please include the threads that will be invoking each callback.
init(empty); | ||
} | ||
|
||
void AndroidShellHolder::init(AsyncInitCallback holderCallback) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we come up with a better name than holderCallback
?
); | ||
|
||
if (holderCallback) { | ||
Shell::CreateAsync( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What ensures that shell_
is set before someone calls AndroidShellHolder::Launch
?
@@ -24,6 +24,11 @@ NS_ASSUME_NONNULL_BEGIN | |||
*/ | |||
extern NSString* const FlutterDefaultDartEntrypoint; | |||
|
|||
/** | |||
* the callback of engine async init mode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Full sentence documentation with proper punctuation please.
/** | ||
* the callback of engine async init mode | ||
*/ | ||
typedef void (^InitCallBackBlock)(bool); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/s/CallBack/Callback
@@ -128,6 +133,10 @@ FLUTTER_EXPORT | |||
*/ | |||
- (BOOL)run; | |||
|
|||
/** like run() method, but this method not block mainThread when init |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proper punctuation with complete English, please.
- (BOOL)createShell:(NSString*)entrypoint libraryURI:(NSString*)libraryURI { | ||
- (BOOL)createShell:(NSString*)entrypoint | ||
libraryURI:(NSString*)libraryURI | ||
asyncInitCallback:(InitCallBackBlock)oc_async_init_block { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No abbreviations please.
FML_LOG(ERROR) << "Could not start a shell FlutterEngine on asyncInitMode"; | ||
oc_async_init_block(false); | ||
} else { | ||
selfRef->_shell = std::move(shell); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's to stop someone from invoking sendOnChannel:message:
before _shell
is set? How would they even know when it was safe to call it?
null); | ||
} | ||
|
||
public FlutterEngine() {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is dangerous since it bifurcates the meaning of having a constructed engine. Rather, create an asynchronous factory that calls back with a FlutterEngine instance when it's ready.
Generally, prefer composition over adding more functionalities and state to a single class.
flutterLoader.startInitialization(context.getApplicationContext()); | ||
flutterLoader.ensureInitializationComplete(context, dartVmArgs); | ||
|
||
flutterJNI.addEngineLifecycleListener(engineLifecycleListener); | ||
attachToJni(); | ||
if (asyncInitListener != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is all about doing FlutterJNI.attachToNative() concurrently, you can just call that directly and not mix it into the state of the FlutterEngine instance. Do it outside this class.
/** like run() method, but this method not block mainThread when init | ||
* @param block callbackBlock, called when async init end. must not be nil! | ||
*/ | ||
- (void)asyncRun:(InitCallBackBlock)block; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you're adding an entirely new API, again for the sake of composable API and not mixing concepts together, you can actually just create a prepareVmAsync method or some such which gives you a callback when the Shell is created, at which point you can run run
yourself.
Then we don't have to multiplex all the variants of run
with an async variant.
@liyuqian need help : build host_release/profile failed after update repo (Android/iOS build success)
Durning february, all of type build success. But seems to start from the march, after update repo, host always build failed detail errormsg
Env
Step
|
@lucky-chen : try |
You can also add your name to the AUTHORS file when you're ready for another look. |
hi, all. I split these changes into 3 PR. This way may be better to review and merge。
after #18047 merged , i will submit the remaining 2 pr(Android/iOS) @chinmaygarde
|
Hi, this pr try to reduce engine init time. In our data, init engine cost too much time: Android 300ms, iOS 380ms(most are mid-end and low-end machine)。
In our local test. Android reduce about 66.3% time,and iOS reduce about 46.6% time.
update:
Introduction change
Cause there are 15 files change, so Introduction to changes.
In fact , core modification all in
shell.cc
,new methodCreateAsync
andCreateShellAsyncOnPlatformThread
, other modification most are adapter code(android、iOS) and test。ApiUsage