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

Implementation of two or more threads merging for multiple platform views #27662

Merged
merged 27 commits into from
Aug 6, 2021
Merged

Implementation of two or more threads merging for multiple platform views #27662

merged 27 commits into from
Aug 6, 2021

Conversation

eggfly
Copy link
Member

@eggfly eggfly commented Jul 23, 2021

This PR implements thread merging for both lightweight multiple engines and standalone multiple engines.

Fixed issues:
flutter/flutter#73620
flutter/flutter#78946

The design of this implementation:

  • This change makes sure to be platform independent. Android and iOS share the common logics and the design attempt to ensure that the code in the relevant directory of the different platforms has not been changed.
  • Removed the codes of block thread merging logics to let multiple merging allowed (aka removed this change: block thread merging with shared engines #23733)
  • Make the member owner_of in TaskQueueEntry from a TaskQueueId to std::set<TaskQueueId> owner_of to record all subsumed of the owner.
  • In order to reduce the changes count of existing code, I regard the old raster_thread_merger class as a handler (or a proxy) and introduced a new shared_thread_merger class, use owner_id and subsumed_id as the key of the map, and values are the shared instances of shared_thread_merger class.
  • Merge related method calls including MergeWithLease(), UnmergeNow(), DecrementLease() , IsMergedUnsafe() was redirected to the methods insideshared_thread_merger, so it can take the same lease_term_ counter to determine merge state.
  • A new method RecordMergeCaller() was added and changed UnMergeNow() to UnMergeNowIfLastOne() to remember all merge callers and in order to "unmerge immediately" only if the merger is the last one, when Rasterizer::Teardown() was called.
  • Added some more tests in shell_unittest and fml_unittests, and temporarily uncomment and enablded the fml_unittests in run_tests.py

Merging cases for lightweight engines and standalone engines:

merge_cases

Allowed and disallowed merging operations:

My demos:

  1. Lightweight engines (it merges same gpu thread into one platform thread) : Using this sample and add a flutter animation and a webview inside main.dart
    Link: https://github.com/flutter/samples/tree/master/add_to_app/multiple_flutters/multiple_flutters_android

View screen record video below:

mp4.mp4

I found if it uses FlutterFragment and make the surfaces of multiple flutter instances not rendering properly.
Then the code in FlutterSurfaceView.java will call the setZOrderOnTop()

  private void init() {
    // If transparency is desired then we'll enable a transparent pixel format and place
    // our Window above everything else to get transparent background rendering.
    if (renderTransparently) {
      getHolder().setFormat(PixelFormat.TRANSPARENT);
      setZOrderOnTop(true);
    }

I added .transparencyMode(TransparencyMode.opaque) to make it render again. Firstly I wrote the code of multiple thead merger and found this issue. It has no relationship of
In https://github.com/flutter/samples/blob/master/add_to_app/multiple_flutters/multiple_flutters_android/app/src/main/java/dev/flutter/multipleflutters/DoubleFlutterActivity.kt

            val flutterFragment =
                FlutterFragment.withCachedEngine(i.toString())
                    // Opaque is to avoid platform view rendering problem due to wrong z-order
                    .transparencyMode(TransparencyMode.opaque) // this is needed
                    .build<FlutterFragment>()
  1. Standalone engines (it merges multiple gpu threads into one platform thread)
    Link: https://github.com/eggfly/unable_to_merge_raster_demo
    This code is from the issue: Two webviews in separate flutter engines and show in one page will cause native crash. flutter#78946
    image

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Sorry, something went wrong.

For both lightweight multiple engines and standalone multiple engines.
@google-cla google-cla bot added the cla: yes label Jul 23, 2021
@eggfly eggfly changed the title Implementation of two or more threads merging Implementation of two or more threads merging for multiple platform views Jul 23, 2021
eggfly added 8 commits July 23, 2021 14:51

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Signed-off-by: eggfly <lihaohua90@gmail.com>
Signed-off-by: eggfly <lihaohua90@gmail.com>
Signed-off-by: eggfly <lihaohua90@gmail.com>
Signed-off-by: eggfly <lihaohua90@gmail.com>
Signed-off-by: eggfly <lihaohua90@gmail.com>
Signed-off-by: eggfly <lihaohua90@gmail.com>
@eggfly
Copy link
Member Author

eggfly commented Jul 23, 2021

@gaaclarke @iskakaushik @bruno-garcia @blasten @jason-simmons Would you like to take some time to review for the design and codes in this PR? Thanks~

Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. I read through the message loop changes and it seems very clean, mostly just switching an instance variable to a set. I don't know as much about this as @iskakaushik so we'll have to make sure the approach sounds good to him. It makes sense to me.

I'll come back go give this a deeper read and look at the rasterizer work later. I think there is a few gaps in code coverage of the new logic. I'd double check that, I'll try to identify them when I see them.

@gaaclarke gaaclarke requested a review from iskakaushik July 23, 2021 18:49
@eggfly
Copy link
Member Author

eggfly commented Jul 26, 2021

Besides I'm fixing the iOS test cases now. 😅

fml::RefPtr<fml::MessageLoopTaskQueues> task_queues_;
std::atomic_int lease_term_;
std::mutex mutex_;
std::set<fml::RasterThreadMerger*> merge_callers_;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could also get away with storing the task queue id of the caller, rather than a reference to the thread merger itself.

@zzjjob

This comment has been minimized.

@eggfly
Copy link
Member Author

eggfly commented Jul 30, 2021

@eggfly I don't think so. I filed a bug: flutter/flutter#87284 Even I don't have an easily accessible windows setup for the engine. We do have some engineers that do. I'll ask one nicely while we wait to hear from the infra team about changing the bot.

I solved my bug in testing, it's a member initializing order problem.. I should put thread member after latch and term.
And the code in thread may run before the member initialization of latch and term, and windows environment may have a more aggressive thread scheduling strategy..

struct TaskQueueWrapper {
  fml::MessageLoop* loop = nullptr;

  /// The waiter for message loop initialized ok
  fml::AutoResetWaitableEvent latch;

  /// The waiter for thread finished
  fml::AutoResetWaitableEvent term;

  /// This field must below latch and term member, because
  /// cpp standard reference:
  /// non-static data members are initialized in the order they were declared in
  /// the class definition
  std::thread thread;

  TaskQueueWrapper()
      : thread([this]() {
          fml::MessageLoop::EnsureInitializedForCurrentThread();
          loop = &fml::MessageLoop::GetCurrent();
          latch.Signal();
          term.Wait();
        }) {
    latch.Wait();
  }

Now it's ok.

@eggfly
Copy link
Member Author

eggfly commented Aug 4, 2021

@iskakaushik @gaaclarke I put the parent rasterizer's merger into shell and so the static map can be removed. Please have a review~ 🤝

@eggfly eggfly changed the title Implementation of two or more threads merging for multiple platform views WIP: Implementation of two or more threads merging for multiple platform views Aug 4, 2021
eggfly added 2 commits August 5, 2021 16:04

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@eggfly eggfly changed the title WIP: Implementation of two or more threads merging for multiple platform views Implementation of two or more threads merging for multiple platform views Aug 5, 2021
@eggfly
Copy link
Member Author

eggfly commented Aug 5, 2021

@gaaclarke @iskakaushik I submitted some new code, removed the WIP title. Now let's get a final review. Thanks! ❤️

Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome, a high quality contribution thanks @eggfly. I added a few comments that are mostly questions or nits.

Code: ✅
Testing: ✅
Design: ✅
Logically correct: ?

The logic sounds plausible, @iskakaushik will know better than me. I'm particularly interested to hear his take on the lease terms of the merging. Otherwise this LGTM.

Comment on lines +83 to +87
ASSERT_TRUE(task_queue->HasPendingTasks(platform_queue));
ASSERT_TRUE(task_queue->GetNumPendingTasks(platform_queue) == 2);
// The task count of subsumed queue is 0
ASSERT_FALSE(task_queue->HasPendingTasks(raster_queue));
ASSERT_TRUE(task_queue->GetNumPendingTasks(raster_queue) == 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

attn @iskakaushik this is how this should work, right?

@eggfly eggfly requested review from iskakaushik and gaaclarke August 6, 2021 09:51
Copy link
Contributor

@iskakaushik iskakaushik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@xuweishun
Copy link

In order to solve the multi-engine support platformview thread merging problem #27662 This time I submitted the cherry pick to the 2.5.1 release branch and there are still problems. Has anyone encountered it?

image
image

@eggfly
Copy link
Member Author

eggfly commented Oct 12, 2021

In order to solve the multi-engine support platformview thread merging problem #27662 This time I submitted the cherry pick to the 2.5.1 release branch and there are still problems. Has anyone encountered it?

image image

@SeaShrimper cherry picked to flutter engine and build a local engine? Are there some flutter engine related logs when your app's platform views are showing?

yzhtracy pushed a commit to BuildAndRelease/engine that referenced this pull request Feb 18, 2022
…iews (flutter#27662)

Implementation of two or more threads merging for both lightweight multiple engines and standalone multiple engines.
filmil pushed a commit to filmil/engine that referenced this pull request Apr 21, 2022
…iews (flutter#27662)

Implementation of two or more threads merging for both lightweight multiple engines and standalone multiple engines.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants