-
Notifications
You must be signed in to change notification settings - Fork 6k
Reland Skia Caching improvements #10434
Changes from 3 commits
c82f8d0
871ead7
a831de6
612c38f
9456798
43038a2
e130727
9105082
f3892af
cd687f8
b50cab1
aa93b16
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,6 +43,7 @@ Rasterizer::Rasterizer( | |
| : delegate_(delegate), | ||
| task_runners_(std::move(task_runners)), | ||
| compositor_context_(std::move(compositor_context)), | ||
| user_override_resource_cache_bytes_(false), | ||
| weak_factory_(this) { | ||
| FML_DCHECK(compositor_context_); | ||
| } | ||
|
|
@@ -355,7 +356,15 @@ void Rasterizer::FireNextFrameCallbackIfPresent() { | |
| callback(); | ||
| } | ||
|
|
||
| void Rasterizer::SetResourceCacheMaxBytes(int max_bytes) { | ||
| void Rasterizer::SetResourceCacheMaxBytes(size_t max_bytes, bool from_user) { | ||
| user_override_resource_cache_bytes_ |= from_user; | ||
|
|
||
| if (!from_user && user_override_resource_cache_bytes_) { | ||
| // We should not update the setting here if a user has explicitly set a | ||
| // value for this over the flutter/skia channel. | ||
| return; | ||
| } | ||
|
|
||
| GrContext* context = surface_->GetContext(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Making this call after
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
| if (context) { | ||
| int max_resources; | ||
|
|
@@ -364,6 +373,16 @@ void Rasterizer::SetResourceCacheMaxBytes(int max_bytes) { | |
| } | ||
| } | ||
|
|
||
| size_t Rasterizer::GetResourceCacheMaxBytes() const { | ||
| GrContext* context = surface_->GetContext(); | ||
| if (context) { | ||
| size_t max_bytes; | ||
| context->getResourceCacheLimits(nullptr, &max_bytes); | ||
| return max_bytes; | ||
| } | ||
| return 0; | ||
| } | ||
|
|
||
| Rasterizer::Screenshot::Screenshot() {} | ||
|
|
||
| Rasterizer::Screenshot::Screenshot(sk_sp<SkData> p_data, SkISize p_size) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -374,8 +374,28 @@ class Rasterizer final { | |
| /// | ||
| /// @param[in] max_bytes The maximum byte size of resource that may be | ||
| /// cached for GPU rendering. | ||
| /// @param[in] from_user Whether this request was from user code, e.g. via | ||
| /// the flutter/skia message channel, in which case | ||
| /// it should not be overridden by the platform. | ||
| /// | ||
| void SetResourceCacheMaxBytes(int max_bytes); | ||
| void SetResourceCacheMaxBytes(size_t max_bytes, bool from_user); | ||
|
|
||
| //---------------------------------------------------------------------------- | ||
| /// @brief The current value of Skia's resource cache size. | ||
| /// | ||
| /// @attention This cache setting will be invalidated when the surface is | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This attention block is not true anymore. You just fixed it :) |
||
| /// torn down via `Rasterizer::Teardown`. This call must be made | ||
| /// again with new limits after surface re-acquisition. | ||
| /// | ||
| /// @attention This cache does not describe the entirety of GPU resources | ||
| /// that may be cached. The `RasterCache` also holds very large | ||
| /// GPU resources. | ||
| /// | ||
| /// @see `RasterCache` | ||
| /// | ||
| /// @return The size of Skia's resource cache. | ||
| /// | ||
| size_t GetResourceCacheMaxBytes() const; | ||
|
|
||
| private: | ||
| Delegate& delegate_; | ||
|
|
@@ -384,6 +404,7 @@ class Rasterizer final { | |
| std::unique_ptr<flutter::CompositorContext> compositor_context_; | ||
| std::unique_ptr<flutter::LayerTree> last_layer_tree_; | ||
| fml::closure next_frame_callback_; | ||
| bool user_override_resource_cache_bytes_; | ||
| fml::WeakPtrFactory<Rasterizer> weak_factory_; | ||
|
|
||
| void DoDraw(std::unique_ptr<flutter::LayerTree> layer_tree); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -682,6 +682,16 @@ void Shell::OnPlatformViewSetViewportMetrics(const ViewportMetrics& metrics) { | |
| FML_DCHECK(is_setup_); | ||
| FML_DCHECK(task_runners_.GetPlatformTaskRunner()->RunsTasksOnCurrentThread()); | ||
|
|
||
| // This is the formula Android uses. | ||
| // https://android.googlesource.com/platform/frameworks/base/+/master/libs/hwui/renderthread/CacheManager.cpp#41 | ||
| size_t max_bytes = metrics.physical_width * metrics.physical_height * 12 * 4; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can move this to a constant in rasterizer and use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I'm clear on which part should be the constant. Just the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe a static method on rasterizer that takes |
||
| task_runners_.GetGPUTaskRunner()->PostTask( | ||
| [rasterizer = rasterizer_->GetWeakPtr(), max_bytes] { | ||
| if (rasterizer) { | ||
| rasterizer->SetResourceCacheMaxBytes(max_bytes, false); | ||
| } | ||
| }); | ||
|
|
||
| task_runners_.GetUITaskRunner()->PostTask( | ||
| [engine = engine_->GetWeakPtr(), metrics]() { | ||
| if (engine) { | ||
|
|
@@ -942,7 +952,8 @@ void Shell::HandleEngineSkiaMessage(fml::RefPtr<PlatformMessage> message) { | |
| [rasterizer = rasterizer_->GetWeakPtr(), | ||
| max_bytes = args->value.GetInt()] { | ||
| if (rasterizer) { | ||
| rasterizer->SetResourceCacheMaxBytes(max_bytes); | ||
| rasterizer->SetResourceCacheMaxBytes(static_cast<size_t>(max_bytes), | ||
| true); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,22 @@ ShellTest::ShellTest() | |
|
|
||
| ShellTest::~ShellTest() = default; | ||
|
|
||
| void ShellTest::SendEnginePlatformMessage( | ||
| Shell* shell, | ||
| fml::RefPtr<PlatformMessage> message) { | ||
| fml::AutoResetWaitableEvent latch; | ||
| fml::TaskRunner::RunNowOrPostTask( | ||
| shell->GetTaskRunners().GetPlatformTaskRunner(), | ||
| [shell, &latch, message = std::move(message)]() { | ||
| if (!shell->weak_engine_) { | ||
| return; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You missed signaling the latch here. Consider instead:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh right, thanks. Fixed. |
||
| } | ||
| shell->weak_engine_->HandlePlatformMessage(std::move(message)); | ||
| latch.Signal(); | ||
| }); | ||
| latch.Wait(); | ||
| } | ||
|
|
||
| void ShellTest::SetSnapshotsAndAssets(Settings& settings) { | ||
| if (!assets_dir_.is_valid()) { | ||
| return; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,7 +65,8 @@ TEST_F(ShellTest, InitializeWithDifferentThreads) { | |
| ThreadHost thread_host("io.flutter.test." + GetCurrentTestName() + ".", | ||
| ThreadHost::Type::Platform | ThreadHost::Type::GPU | | ||
| ThreadHost::Type::IO | ThreadHost::Type::UI); | ||
| TaskRunners task_runners("test", thread_host.platform_thread->GetTaskRunner(), | ||
| TaskRunners task_runners("test", | ||
| thread_host.platform_thread->GetTaskRunner(), | ||
| thread_host.gpu_thread->GetTaskRunner(), | ||
| thread_host.ui_thread->GetTaskRunner(), | ||
| thread_host.io_thread->GetTaskRunner()); | ||
|
|
@@ -138,7 +139,8 @@ TEST_F(ShellTest, InitializeWithGPUAndPlatformThreadsTheSame) { | |
| Settings settings = CreateSettingsForFixture(); | ||
| ThreadHost thread_host( | ||
| "io.flutter.test." + GetCurrentTestName() + ".", | ||
| ThreadHost::Type::Platform | ThreadHost::Type::IO | ThreadHost::Type::UI); | ||
| ThreadHost::Type::Platform | ThreadHost::Type::IO | | ||
| ThreadHost::Type::UI); | ||
| TaskRunners task_runners( | ||
| "test", | ||
| thread_host.platform_thread->GetTaskRunner(), // platform | ||
|
|
@@ -166,7 +168,8 @@ TEST_F(ShellTest, FixturesAreFunctional) { | |
| fml::AutoResetWaitableEvent main_latch; | ||
| AddNativeCallback( | ||
| "SayHiFromFixturesAreFunctionalMain", | ||
| CREATE_NATIVE_ENTRY([&main_latch](auto args) { main_latch.Signal(); })); | ||
| CREATE_NATIVE_ENTRY([&main_latch](auto args) { main_latch.Signal(); | ||
| })); | ||
|
|
||
| RunEngine(shell.get(), std::move(configuration)); | ||
| main_latch.Wait(); | ||
|
|
@@ -201,7 +204,8 @@ TEST_F(ShellTest, SecondaryIsolateBindingsAreSetupViaShellSettings) { | |
|
|
||
| TEST(ShellTestNoFixture, EnableMirrorsIsWhitelisted) { | ||
| if (DartVM::IsRunningPrecompiledCode()) { | ||
| // This covers profile and release modes which use AOT (where this flag does | ||
| // This covers profile and release modes which use AOT (where this flag | ||
| does | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like there are some unintentional reformattings in this file
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah clang format reflowed some comments that I then uncommented. Will fix |
||
| // not make sense anyway). | ||
| GTEST_SKIP(); | ||
| return; | ||
|
|
@@ -210,7 +214,8 @@ TEST(ShellTestNoFixture, EnableMirrorsIsWhitelisted) { | |
| const std::vector<fml::CommandLine::Option> options = { | ||
| fml::CommandLine::Option("dart-flags", "--enable_mirrors")}; | ||
| fml::CommandLine command_line("", options, std::vector<std::string>()); | ||
| flutter::Settings settings = flutter::SettingsFromCommandLine(command_line); | ||
| flutter::Settings settings = | ||
| flutter::SettingsFromCommandLine(command_line); | ||
| EXPECT_EQ(settings.dart_flags.size(), 1u); | ||
| } | ||
|
|
||
|
|
@@ -228,7 +233,8 @@ TEST_F(ShellTest, BlacklistedDartVMFlag) { | |
| "Encountered blacklisted Dart VM flag: --verify_after_gc"; | ||
| ASSERT_DEATH(flutter::SettingsFromCommandLine(command_line), expected); | ||
| #else | ||
| flutter::Settings settings = flutter::SettingsFromCommandLine(command_line); | ||
| flutter::Settings settings = | ||
| flutter::SettingsFromCommandLine(command_line); | ||
| EXPECT_EQ(settings.dart_flags.size(), 0u); | ||
| #endif | ||
| } | ||
|
|
@@ -238,7 +244,8 @@ TEST_F(ShellTest, WhitelistedDartVMFlag) { | |
| fml::CommandLine::Option("dart-flags", | ||
| "--max_profile_depth 1,--random_seed 42")}; | ||
| fml::CommandLine command_line("", options, std::vector<std::string>()); | ||
| flutter::Settings settings = flutter::SettingsFromCommandLine(command_line); | ||
| flutter::Settings settings = | ||
| flutter::SettingsFromCommandLine(command_line); | ||
|
|
||
| #if FLUTTER_RUNTIME_MODE != FLUTTER_RUNTIME_MODE_RELEASE | ||
| EXPECT_EQ(settings.dart_flags.size(), 2u); | ||
|
|
@@ -263,15 +270,18 @@ TEST_F(ShellTest, NoNeedToReportTimingsByDefault) { | |
| PumpOneFrame(shell.get()); | ||
| ASSERT_FALSE(GetNeedsReportTimings(shell.get())); | ||
|
|
||
| // This assertion may or may not be the direct result of needs_report_timings_ | ||
| // being false. The count could be 0 simply because we just cleared unreported | ||
| // This assertion may or may not be the direct result of | ||
| needs_report_timings_ | ||
| // being false. The count could be 0 simply because we just cleared | ||
| unreported | ||
| // timings by reporting them. Hence this can't replace the | ||
| // ASSERT_FALSE(GetNeedsReportTimings(shell.get())) check. We added this | ||
| // assertion for an additional confidence that we're not pushing back to | ||
| // unreported timings unnecessarily. | ||
| // | ||
| // Conversely, do not assert UnreportedTimingsCount(shell.get()) to be | ||
| // positive in any tests. Otherwise those tests will be flaky as the clearing | ||
| // positive in any tests. Otherwise those tests will be flaky as the | ||
| clearing | ||
| // of unreported timings is unpredictive. | ||
| ASSERT_EQ(UnreportedTimingsCount(shell.get()), 0); | ||
| } | ||
|
|
@@ -409,8 +419,8 @@ TEST_F(ShellTest, FrameRasterizedCallbackIsCalled) { | |
| std::vector<FrameTiming> timings = {timing}; | ||
| CheckFrameTimings(timings, start, finish); | ||
|
|
||
| // Check that onBeginFrame has the same timestamp as FrameTiming's build start | ||
| int64_t build_start = | ||
| // Check that onBeginFrame has the same timestamp as FrameTiming's build | ||
| start int64_t build_start = | ||
| timing.Get(FrameTiming::kBuildStart).ToEpochDelta().ToMicroseconds(); | ||
| ASSERT_EQ(build_start, begin_frame); | ||
| } | ||
|
|
@@ -423,9 +433,8 @@ TEST(SettingsTest, FrameTimingSetsAndGetsProperly) { | |
| int lastPhaseIndex = -1; | ||
| FrameTiming timing; | ||
| for (auto phase : FrameTiming::kPhases) { | ||
| ASSERT_TRUE(phase > lastPhaseIndex); // Ensure that kPhases are in order. | ||
| lastPhaseIndex = phase; | ||
| auto fake_time = | ||
| ASSERT_TRUE(phase > lastPhaseIndex); // Ensure that kPhases are in | ||
| order. lastPhaseIndex = phase; auto fake_time = | ||
| fml::TimePoint::FromEpochDelta(fml::TimeDelta::FromMicroseconds(phase)); | ||
| timing.Set(phase, fake_time); | ||
| ASSERT_TRUE(timing.Get(phase) == fake_time); | ||
|
|
@@ -513,7 +522,8 @@ TEST_F(ShellTest, ReportTimingsIsCalledImmediatelyAfterTheFirstFrame) { | |
| reportLatch.Wait(); | ||
| shell.reset(); | ||
|
|
||
| // Check for the immediate callback of the first frame that doesn't wait for | ||
| // Check for the immediate callback of the first frame that doesn't wait | ||
| for | ||
| // the other 9 frames to be rasterized. | ||
| ASSERT_EQ(timestamps.size(), FrameTiming::kCount); | ||
| } | ||
|
|
@@ -600,5 +610,53 @@ TEST_F(ShellTest, WaitForFirstFrameInlined) { | |
| ASSERT_FALSE(event.WaitWithTimeout(fml::TimeDelta::FromMilliseconds(1000))); | ||
| } | ||
|
|
||
| TEST_F(ShellTest, SetResourceCacheSize) { | ||
| Settings settings = CreateSettingsForFixture(); | ||
| auto task_runner = GetThreadTaskRunner(); | ||
| TaskRunners task_runners("test", task_runner, task_runner, task_runner, | ||
| task_runner); | ||
| std::unique_ptr<Shell> shell = | ||
| CreateShell(std::move(settings), std::move(task_runners)); | ||
|
|
||
| // Create the surface needed by rasterizer | ||
| PlatformViewNotifyCreated(shell.get()); | ||
|
|
||
| auto configuration = RunConfiguration::InferFromSettings(settings); | ||
| configuration.SetEntrypoint("emptyMain"); | ||
|
|
||
| RunEngine(shell.get(), std::move(configuration)); | ||
| PumpOneFrame(shell.get()); | ||
|
|
||
| EXPECT_EQ(shell->GetRasterizer()->GetResourceCacheMaxBytes(), | ||
| static_cast<size_t>(24 * (1 << 20))); | ||
|
|
||
| fml::TaskRunner::RunNowOrPostTask( | ||
| shell->GetTaskRunners().GetPlatformTaskRunner(), [&shell]() { | ||
| shell->GetPlatformView()->SetViewportMetrics( | ||
| {1.0, 400, 200, 0, 0, 0, 0, 0, 0, 0, 0}); | ||
| }); | ||
| PumpOneFrame(shell.get()); | ||
|
|
||
| EXPECT_EQ(shell->GetRasterizer()->GetResourceCacheMaxBytes(), 3840000U); | ||
|
|
||
| std::string request_json = | ||
| "{\"method\": \"Skia.setResourceCacheMaxBytes\", \"args\": 10000}"; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Consider using raw string literals when constructing JSON (properly formatted) inline for readability. Or use RapidJSON. |
||
| std::vector<uint8_t> data(request_json.begin(), request_json.end()); | ||
| auto platform_message = fml::MakeRefCounted<PlatformMessage>( | ||
| "flutter/skia", std::move(data), nullptr); | ||
| SendEnginePlatformMessage(shell.get(), std::move(platform_message)); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you make sure a reply is received for this message? I think that's broken right now.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's no reply on this mesage. It's not implemented at all. What's the use case for it?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See b/137134910 for details. Without a reply applications can't await setting this, so have no visibility into when the change has taken effect. I think sending an empty message around here would fix it: https://github.com/flutter/engine/blob/d937b693f556db17cf80622e6fdffb7e27316f61/shell/common/shell.cc#L945
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've added an empty reply and a test for it.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in b50cab1 |
||
| PumpOneFrame(shell.get()); | ||
| EXPECT_EQ(shell->GetRasterizer()->GetResourceCacheMaxBytes(), 10000U); | ||
|
|
||
| fml::TaskRunner::RunNowOrPostTask( | ||
| shell->GetTaskRunners().GetPlatformTaskRunner(), [&shell]() { | ||
| shell->GetPlatformView()->SetViewportMetrics( | ||
| {1.0, 800, 400, 0, 0, 0, 0, 0, 0, 0, 0}); | ||
| }); | ||
| PumpOneFrame(shell.get()); | ||
|
|
||
| EXPECT_EQ(shell->GetRasterizer()->GetResourceCacheMaxBytes(), 10000U); | ||
| } | ||
|
|
||
| } // namespace testing | ||
| } // namespace flutter | ||
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 API is timing sensitive and depends on the surface already being acquired. It would be better if the cache size in bytes was instead stored in an
std::optional<size_t> on the rasterizer.. Then, here and inRasterizer::Setup, you could usevalue_or(some_default)to set the cache size of context of the surface. Similarly, the getter would then just the cache size from the optional ivar instead of depending on the surface being present. This scheme would also make the setting survive rasterizer teardown and re-setup.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.
done