-
Notifications
You must be signed in to change notification settings - Fork 6k
Implementation of two or more threads merging for multiple platform views #27662
Implementation of two or more threads merging for multiple platform views #27662
Conversation
For both lightweight multiple engines and standalone multiple engines.
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>
@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~ |
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 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.
Signed-off-by: eggfly <lihaohua90@gmail.com>
Besides I'm fixing the iOS test cases now. 😅 |
fml/shared_thread_merger_impl.h
Outdated
fml::RefPtr<fml::MessageLoopTaskQueues> task_queues_; | ||
std::atomic_int lease_term_; | ||
std::mutex mutex_; | ||
std::set<fml::RasterThreadMerger*> merge_callers_; |
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.
we could also get away with storing the task queue id of the caller, rather than a reference to the thread merger itself.
And format code
This comment has been minimized.
This comment has been minimized.
I solved my bug in testing, it's a member initializing order problem.. I should put 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. |
@iskakaushik @gaaclarke I put the parent rasterizer's merger into shell and so the static map can be removed. Please have a review~ 🤝 |
Remove RecordMergeCaller() method.
@gaaclarke @iskakaushik I submitted some new code, removed the WIP title. Now let's get a final review. Thanks! ❤️ |
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 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.
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); |
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.
attn @iskakaushik this is how this should work, right?
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.
LGTM
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? |
@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? |
…iews (flutter#27662) Implementation of two or more threads merging for both lightweight multiple engines and standalone multiple engines.
…iews (flutter#27662) Implementation of two or more threads merging for both lightweight multiple engines and standalone multiple engines.
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:
owner_of
inTaskQueueEntry
from aTaskQueueId
tostd::set<TaskQueueId> owner_of
to record all subsumed of the owner.raster_thread_merger
class as a handler (or a proxy) and introduced a newshared_thread_merger
class, useowner_id
andsubsumed_id
as the key of the map, and values are the shared instances ofshared_thread_merger
class.MergeWithLease()
,UnmergeNow()
,DecrementLease()
,IsMergedUnsafe()
was redirected to the methods insideshared_thread_merger
, so it can take the samelease_term_
counter to determine merge state.RecordMergeCaller()
was added and changedUnMergeNow()
toUnMergeNowIfLastOne()
to remember all merge callers and in order to "unmerge immediately" only if the merger is the last one, whenRasterizer::Teardown()
was called.shell_unittest
andfml_unittests
, and temporarily uncomment and enablded thefml_unittests
in run_tests.pyMerging cases for lightweight engines and standalone engines:
Allowed and disallowed merging operations:

My demos:
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 thesetZOrderOnTop()
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 ofIn https://github.com/flutter/samples/blob/master/add_to_app/multiple_flutters/multiple_flutters_android/app/src/main/java/dev/flutter/multipleflutters/DoubleFlutterActivity.kt
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
Pre-launch Checklist
writing and running engine tests.
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.