Add ability to disable the raster thread merger #20800
Conversation
blasten
force-pushed
the
raster_thread_merger_disable
branch
from
e318fe0 to
7481a1f
Compare
blasten
force-pushed
the
raster_thread_merger_disable
branch
from
b658d77 to
1fa8467
Compare
| @@ -597,201 +597,6 @@ TEST_F(ShellTest, | |||
| DestroyShell(std::move(shell)); | |||
| } | |||
|
|
|||
| TEST_F(ShellTest, OnPlatformViewDestroyAfterMergingThreads) { | |||
There was a problem hiding this comment.
Ideally, this test should pass with this change.
| DestroyShell(std::move(shell)); | ||
| } | ||
|
|
||
| TEST_F(ShellTest, OnPlatformViewDestroyWhenThreadsAreMerging) { |
There was a problem hiding this comment.
Same here, this test should still pass
There was a problem hiding this comment.
LGTM modulo nits. This is great! Looks like a better and simpler solution than the "always merging threads" solution.
@iskakaushik
| const auto raster_thread_merger_ = | ||
| fml::MakeRefCounted<fml::RasterThreadMerger>(qid1, qid2); | ||
|
|
||
| raster_thread_merger_->Disable(); |
There was a problem hiding this comment.
Can we add some assert after this line to ensure the raster_thread_merge is truely disabled? So we know enable worked.
| // | ||
| // This incorrect assumption can lead to dead lock. | ||
| // See `should_post_raster_task` for more. | ||
| rasterizer_->DisableThreadMergerIfNeeded(); |
There was a problem hiding this comment.
Let's move this right before should_post_raster_task, so it's clear that should_post_raster_task is checked right after disabling thread merger.
| // | ||
| // This incorrect assumption can lead to dead lock. | ||
| // See `should_post_raster_task` for more. | ||
| rasterizer_->DisableThreadMergerIfNeeded(); |
There was a problem hiding this comment.
Let's move this right before should_post_raster_task, so it's clear that should_post_raster_task is checked right after disabling thread merger.
Description
Reworks dynamic thread merging, so there isn't need to merge threads on
Shell::OnPlatformViewDestroyed(). https://github.com/flutter/engine/pull/19919/filesRelated Issues
flutter/flutter#57067
Tests
I added the following tests: Unit tests
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.Breaking Change
Did any tests fail when you ran them? Please read handling breaking changes.