-
Notifications
You must be signed in to change notification settings - Fork 6k
Reland Skia Caching improvements #10434
Changes from all 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_); | ||
| } | ||
|
|
@@ -55,6 +56,10 @@ fml::WeakPtr<Rasterizer> Rasterizer::GetWeakPtr() const { | |
|
|
||
| void Rasterizer::Setup(std::unique_ptr<Surface> surface) { | ||
| surface_ = std::move(surface); | ||
| if (max_cache_bytes_.has_value()) { | ||
| SetResourceCacheMaxBytes(max_cache_bytes_.value(), | ||
| user_override_resource_cache_bytes_); | ||
| } | ||
| compositor_context_->OnGrContextCreated(); | ||
| } | ||
|
|
||
|
|
@@ -355,7 +360,20 @@ 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; | ||
| } | ||
|
|
||
| max_cache_bytes_ = max_bytes; | ||
| if (!surface_) { | ||
| 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 +382,19 @@ void Rasterizer::SetResourceCacheMaxBytes(int max_bytes) { | |
| } | ||
| } | ||
|
|
||
| std::optional<size_t> Rasterizer::GetResourceCacheMaxBytes() const { | ||
| if (!surface_) { | ||
|
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. The validity of this call still depends on the the presence of a surface. Why not just access the optional and return the size_t instead of an optional? Now that the value is maintained between teardowns and setups, the contexts cache size should always be consistent with that in the optional.
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. What happens if this is called before the first setup, and before anyone has attempted to set the metrics?
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. Return
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. But that's not really accurate, and that symbol is defined as an implementation detail of gl_surface_cpu.cc. It's different on Fuchsia for instance, where they override it in another place.
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. Ok then. Bad idea. Thanks for clarifying. |
||
| return std::nullopt; | ||
| } | ||
| GrContext* context = surface_->GetContext(); | ||
| if (context) { | ||
| size_t max_bytes; | ||
| context->getResourceCacheLimits(nullptr, &max_bytes); | ||
| return max_bytes; | ||
| } | ||
| return std::nullopt; | ||
| } | ||
|
|
||
| Rasterizer::Screenshot::Screenshot() {} | ||
|
|
||
| Rasterizer::Screenshot::Screenshot(sk_sp<SkData> p_data, SkISize p_size) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |
| #define SHELL_COMMON_RASTERIZER_H_ | ||
|
|
||
| #include <memory> | ||
| #include <optional> | ||
|
|
||
| #include "flutter/common/settings.h" | ||
| #include "flutter/common/task_runners.h" | ||
|
|
@@ -374,8 +375,25 @@ 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, if a surface | ||
| /// is present. | ||
| /// | ||
| /// @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, if available. | ||
| /// | ||
| std::optional<size_t> GetResourceCacheMaxBytes() const; | ||
|
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. Per discussion above, this can now be just |
||
|
|
||
| private: | ||
| Delegate& delegate_; | ||
|
|
@@ -384,6 +402,8 @@ 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_; | ||
| std::optional<size_t> max_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) { | ||
|
|
@@ -939,10 +949,14 @@ void Shell::HandleEngineSkiaMessage(fml::RefPtr<PlatformMessage> message) { | |
| return; | ||
|
|
||
| task_runners_.GetGPUTaskRunner()->PostTask( | ||
| [rasterizer = rasterizer_->GetWeakPtr(), | ||
| max_bytes = args->value.GetInt()] { | ||
| [rasterizer = rasterizer_->GetWeakPtr(), max_bytes = args->value.GetInt(), | ||
| response = std::move(message->response())] { | ||
| if (rasterizer) { | ||
| rasterizer->SetResourceCacheMaxBytes(max_bytes); | ||
| rasterizer->SetResourceCacheMaxBytes(static_cast<size_t>(max_bytes), | ||
| true); | ||
| } | ||
| if (response) { | ||
| response->CompleteEmpty(); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -264,11 +264,11 @@ TEST_F(ShellTest, NoNeedToReportTimingsByDefault) { | |||
| 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 | ||||
| // 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. | ||||
| // 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 | ||||
|
|
@@ -600,5 +600,124 @@ 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().value_or(0U), | ||||
| 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().value_or(0U), | ||||
| 3840000U); | ||||
|
|
||||
| std::string request_json = R"json({ | ||||
| "method": "Skia.setResourceCacheMaxBytes", | ||||
| "args": 10000 | ||||
| })json"; | ||||
| 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: Line 945 in d937b69
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().value_or(0U), | ||||
| 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().value_or(0U), | ||||
| 10000U); | ||||
| } | ||||
|
|
||||
| TEST_F(ShellTest, SetResourceCacheSizeEarly) { | ||||
| 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)); | ||||
|
|
||||
| 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()); | ||||
|
|
||||
| // 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().value_or(0), | ||||
| static_cast<size_t>(3840000U)); | ||||
| } | ||||
|
|
||||
| TEST_F(ShellTest, SetResourceCacheSizeNotifiesDart) { | ||||
| 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)); | ||||
|
|
||||
| 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()); | ||||
|
|
||||
| // Create the surface needed by rasterizer | ||||
| PlatformViewNotifyCreated(shell.get()); | ||||
|
|
||||
| auto configuration = RunConfiguration::InferFromSettings(settings); | ||||
| configuration.SetEntrypoint("testSkiaResourceCacheSendsResponse"); | ||||
|
|
||||
| EXPECT_EQ(shell->GetRasterizer()->GetResourceCacheMaxBytes().value_or(0), | ||||
| static_cast<size_t>(3840000U)); | ||||
|
|
||||
| fml::AutoResetWaitableEvent latch; | ||||
| AddNativeCallback("NotifyNative", CREATE_NATIVE_ENTRY([&latch](auto args) { | ||||
| latch.Signal(); | ||||
| })); | ||||
|
|
||||
| RunEngine(shell.get(), std::move(configuration)); | ||||
| PumpOneFrame(shell.get()); | ||||
|
|
||||
| latch.Wait(); | ||||
|
|
||||
| EXPECT_EQ(shell->GetRasterizer()->GetResourceCacheMaxBytes().value_or(0), | ||||
| static_cast<size_t>(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