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

Improve caching limits for Skia #9503

Merged
dnfield merged 6 commits into from
Jun 26, 2019
Merged

Improve caching limits for Skia #9503

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6e8e0c6
Improve caching limits for Skia
dnfield Jun 26, 2019
8ef6360
format
dnfield Jun 26, 2019
dd2be3b
review
dnfield Jun 26, 2019
abfd34e
fix types, remove temp log
dnfield Jun 26, 2019
23230e3
std::atomic<bool>
dnfield Jun 26, 2019
6318e94
Revert "std::atomic<bool>"
dnfield Jun 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion shell/common/rasterizer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_);
}
Expand Down Expand Up @@ -405,7 +406,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();
if (context) {
int max_resources;
Expand All @@ -414,6 +423,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)
Expand Down
9 changes: 8 additions & 1 deletion shell/common/rasterizer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,13 @@ class Rasterizer final : public SnapshotDelegate {
return compositor_context_.get();
}

void SetResourceCacheMaxBytes(int max_bytes);
// Sets the max size in bytes of the Skia resource cache. If this call is
// originating from the user, e.g. over the flutter/skia system channel,
// set from_user to true and the value will take precedence over system
// generated values, e.g. from a display resolution change.
void SetResourceCacheMaxBytes(size_t max_bytes, bool from_user);

size_t GetResourceCacheMaxBytes() const;

private:
Delegate& delegate_;
Expand All @@ -101,6 +107,7 @@ class Rasterizer final : public SnapshotDelegate {
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_;
Copy link
Contributor

@liyuqian liyuqian Jun 26, 2019

Choose a reason for hiding this comment

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

It may just be more future-proof to make this std::atomic<bool> in case that someone mistakenly modified it on other threads (than the GPU thread) in a future PR. Considering the low frequency of reading/writing this bit, the overhead of that atomic bool should be negligible.

Copy link
Member

@jason-simmons jason-simmons Jun 26, 2019

Choose a reason for hiding this comment

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

The rasterizer is only accessed on the GPU thread. I'd prefer not to imply that any part of the rasterizer is safe to access from other threads.

Copy link
Contributor

@liyuqian liyuqian Jun 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, this is in Rasterizer, not Shell. Ignore my earlier comment :)

Copy link
Contributor Author

@dnfield dnfield Jun 26, 2019

Choose a reason for hiding this comment

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

How about if I check that the method using it runs on the GPU thread?

Copy link
Contributor

@liyuqian liyuqian Jun 26, 2019

Choose a reason for hiding this comment

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

Don't worry about it @dnfield . All fields of Rasterizer are only safe inside the GPU thread. So there's little reason to just guard this single field by making your change more complex.

Copy link
Contributor Author

@dnfield dnfield Jun 26, 2019

Choose a reason for hiding this comment

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

Nevermind, I see what you're saying - just switched it to std::atomic

Copy link
Contributor Author

@dnfield dnfield Jun 26, 2019

Choose a reason for hiding this comment

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

Haha - Github did not refresh on me. Ok, reverted back.

fml::WeakPtrFactory<Rasterizer> weak_factory_;

// |SnapshotDelegate|
Expand Down
13 changes: 12 additions & 1 deletion shell/common/shell.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -610,6 +610,16 @@ void Shell::OnPlatformViewSetViewportMetrics(const ViewportMetrics& metrics) {
FML_DCHECK(is_setup_);
FML_DCHECK(task_runners_.GetPlatformTaskRunner()->RunsTasksOnCurrentThread());

// This is the formula Android uses.
Copy link
Contributor

@abhishekamit abhishekamit Jul 9, 2019

Choose a reason for hiding this comment

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

Where is this method called from?

On android it looks like the engine method is called directly:

engine/shell/platform/android/android_shell_holder.cc

Line 184 in 0a2e28d

engine->SetViewportMetrics(metrics);

iOS looks like it does the same:

engine/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm

Line 132 in 1dcd5f5

engine->SetViewportMetrics(std::move(metrics));

I'm not seeing this code getting run unless I add platform_view_->SetViewportMetrics(metrics); to

engine/shell/platform/android/android_shell_holder.cc

Line 175 in 2284210

void AndroidShellHolder::SetViewportMetrics(

Copy link
Contributor Author

@dnfield dnfield Jul 9, 2019

Choose a reason for hiding this comment

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

This is called by the embedders when the tell the PlatformView to update the viewport metrics. On iOS, this happens in response to a few things in FlutterViewController's lifecycle, and is funnled through here: https://github.com/flutter/engine/blob/master/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm#L674

On Android, that call results from similar things in FlutterView.java which all funnel through https://github.com/flutter/engine/blob/master/shell/platform/android/io/flutter/view/FlutterView.java#L614.

Unless you're writing your own embedding, you should never call this method. The reason I put the change here is because this is the method where we learn about screen resolution changes, and we want the cache size to be based on a multiple of screen resolution.

It's possible to override that entirely by using the Skia platform channel.

Copy link
Contributor

@abhishekamit abhishekamit Jul 9, 2019

Choose a reason for hiding this comment

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

If you go down the callstack from FlutterView.java you don't get here, you get to the engine call.

engine/shell/platform/android/io/flutter/view/FlutterView.java

Line 617 in 2284210

mNativeView.getFlutterJNI().setViewportMetrics(mMetrics.devicePixelRatio, mMetrics.physicalWidth,

engine/shell/platform/android/platform_view_android_jni.cc

Line 279 in 2284210

ANDROID_SHELL_HOLDER->SetViewportMetrics(metrics);

engine/shell/platform/android/android_shell_holder.cc

Line 184 in 2284210

engine->SetViewportMetrics(metrics);

Before this change calling the engine SetViewportMetrics was equivalent to calling the platform view wrapper around it. Its not after this change.

Copy link
Contributor Author

@dnfield dnfield Jul 9, 2019

Choose a reason for hiding this comment

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

Ahh that's unexpected. The other similar methods in the JNI platform view just forward to platform view. I think it'd be reasonable to change this one to do the same.

Woudl that unstick you?

Copy link
Contributor

@abhishekamit abhishekamit Jul 9, 2019

Choose a reason for hiding this comment

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

When I add that line the engine crashes in release mode because surface_ seems to be null here:

engine/shell/common/rasterizer.cc

Line 424 in 2284210

GrContext* context = surface_->GetContext();

I'm not sure why surface_ is null in release mode but not debug mode.

Copy link
Contributor Author

@dnfield dnfield Jul 9, 2019

Choose a reason for hiding this comment

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

It depends on where you're calling this - the surface may not have been set up yet and perhaps there's some race condition...

Copy link
Contributor

@abhishekamit abhishekamit Jul 9, 2019

Choose a reason for hiding this comment

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

I mentioned this on #9729, but since the effect of this change seems to have been to reduce the default cache size from 512mb to 24mb should it be rolled back?

// https://android.googlesource.com/platform/frameworks/base/+/master/libs/hwui/renderthread/CacheManager.cpp#41
int max_bytes = metrics.physical_width * metrics.physical_height * 12 * 4;
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) {
Expand Down Expand Up @@ -863,7 +873,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);
}
});
}
Expand Down
5 changes: 4 additions & 1 deletion shell/gpu/gpu_surface_gl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,10 @@ static const int kGrCacheMaxCount = 8192;

// Default maximum number of bytes of GPU memory of budgeted resources in the
// cache.
static const size_t kGrCacheMaxByteSize = 512 * (1 << 20);
// The shell will dynamically increase or decrease this cache based on the
// viewport size, unless a user has specifically requested a size on the Skia
// system channel.
static const size_t kGrCacheMaxByteSize = 24 * (1 << 20);

GPUSurfaceGL::GPUSurfaceGL(GPUSurfaceGLDelegate* delegate)
: delegate_(delegate), weak_factory_(this) {
Expand Down