Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support init engine asynchronous #20433

Closed
wants to merge 3 commits into from

Conversation

lybeen
Copy link

@lybeen lybeen commented Aug 12, 2020

Description

In Engine initialization, we have to wait for all other threads ready, it has an opportunity to improve it, so I make this asynchronous.

Comparison

Before

4C888FFB-440C-4314-874F-9F9AA6CC7337

After

2459FB6E-7F2C-41F7-B5C9-B1BEBDBC1E29

Conclusion:

It's clearly show that it makes ui thread asynchronous , platform thread does't have to wait for it in engine initialization.
It may make about 200ms time for platform thread.

Related Issues

19641

This pr seems a little hackable, my solution seems a bit more sensible.

Tests

I added the following tests:

Go through all test cases

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the [contributor guide] and followed the process outlined there for submitting PRs.
  • I signed the [CLA].
  • I read and followed the [C++, Objective-C, Java style guides] for the engine.
  • I read the [tree hygiene] wiki page, which explains my responsibilities.
  • I updated/added relevant documentation.
  • All existing and new tests are passing.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read [handling breaking changes].

  • No, no existing tests failed, so this is not a breaking change.

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@googlebot
Copy link

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.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

1 similar comment
@googlebot
Copy link

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.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@lybeen
Copy link
Author

lybeen commented Aug 13, 2020

@googlebot I fixed it.

@chinmaygarde
Copy link
Member

cc @jason-simmons @liyuqian

@liyuqian
Copy link
Contributor

This approach looks similar to #18047. That PR is very close to pass all the tests but there's some mysterious "Windows Host Engine" failure. @lybeen : can you please check if that PR is helpful in your case, and whether you're able to revise your PR to pass all tests?

@gaaclarke
Copy link
Member

gaaclarke commented Oct 12, 2020

I've worked with this issue in the past with another contributor. There are too many places where we assume we have an initialized Shell. I don't want to create the opportunity to create bugs by allowing shell to be in yet another state.

I think this is a good goal but the change should use std::future to represent a shell that hasn't been initialized yet so that we can safely migrate the code to using it. The previous contributor did most of the work but ran into memory problems on windows which they didn't address so it stalled out.

Edit: the other PR: #18047

@chinmaygarde
Copy link
Member

Closing this as we want to go with another approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants