Improve caching limits for Skia #9503

dnfield · 2019-06-26T17:31:49Z

Our current Skia cache limit for the on screen GrContext is 512mb. We already know this is too big for some lower end devices, and so we've implemented a custom channel to allow applications or embedders to easily set a lower value.

The value is just too high for lower end devices with little ram. Android P+ uses the formula added here - I've also set the initial value substantially lower so we don't end up allocating lots of memory while waiting for the screen metrics to get pumped through.

Fixes flutter/flutter#35091
Fixes flutter/flutter#35038

/cc @ened

/cc @nathanrogersgoogle

dnfield · 2019-06-26T17:34:34Z

For some examples - on an iPhone 5, this would set the cache to roughly 48mb. On a Pixel 3, it would set it to about 111mb. On an iPhone X, about 131mb.

All of these values are substantially lower than our current default of 512mb (a 4k display is still below it, a 5k is above that). We should watch this for performance regressions, but I suspect our current value is just too high.

On a machine with the new Apple display, it would be about 977mb 😆

liyuqian · 2019-06-26T18:05:53Z

shell/common/shell.cc

+  // Only set this if the user has _not_ requested a specific value.
+  if (raster_cache_max_bytes_user_value_ == -1) {
+    // This is the formula Android uses.
+    int max_bytes = metrics.physical_width * metrics.physical_height * 12 * 4;


Add shell unit tests for this? You can go over the 4K, 5K, and new mac display settings there :)

Will stop by - I'm struggling a bit on the test for this since things fizzle if there's no GrContext.

liyuqian · 2019-06-26T18:13:18Z

shell/common/shell.cc

@@ -610,6 +611,18 @@ void Shell::OnPlatformViewSetViewportMetrics(const ViewportMetrics& metrics) {
  FML_DCHECK(is_setup_);
  FML_DCHECK(task_runners_.GetPlatformTaskRunner()->RunsTasksOnCurrentThread());

+  // Only set this if the user has _not_ requested a specific value.
+  if (raster_cache_max_bytes_user_value_ == -1) {
+    // This is the formula Android uses.


Can we add a link to the Android formula? I'm curious how it gets the magic number 12.

See https://android.googlesource.com/platform/frameworks/base/+/master/libs/hwui/renderthread/CacheManager.cpp#41

Added link to comment

jason-simmons · 2019-06-26T18:04:56Z

shell/common/shell.cc

@@ -859,9 +872,10 @@ void Shell::HandleEngineSkiaMessage(fml::RefPtr<PlatformMessage> message) {
  if (args == root.MemberEnd() || !args->value.IsInt())
    return;

+  raster_cache_max_bytes_user_value_ = args->value.GetInt();


This is running on the UI thread, but Shell::OnPlatformViewSetViewportMetrics runs on the platform thread. Use a std::atomic type for raster_cache_max_bytes_user_value_.

Or perhaps a better way to go would be holding raster_cache_max_bytes_user_value_ in a std::optional member within the rasterizer, which will only be accessed on the GPU thread. SetResourceCacheMaxBytes can take a flag indicating whether it should set the raster_cache_max_bytes_user_value_.

chinmaygarde

Please add a unit test for this in shell_unittests. You can make the call to update the viewport metrics directly. Then you will have to post a task to the GPU thread to read the resource cache size for asserting.

chinmaygarde · 2019-06-26T18:12:09Z

shell/common/shell.h

@@ -106,7 +106,10 @@ class Shell final : public PlatformView::Delegate,
  fml::WeakPtr<Engine> weak_engine_;          // to be shared across threads
  fml::WeakPtr<Rasterizer> weak_rasterizer_;  // to be shared across threads
  fml::WeakPtr<PlatformView>
-      weak_platform_view_;  // to be shared across threads
+      weak_platform_view_;                 // to be shared across threads
+  int raster_cache_max_bytes_user_value_;  // The user-requested value for the


std::optional<size_t> instead of magic values please.

Going to move this to rasterizer as a bool flag per @jason-simmons suggestion instead

chinmaygarde · 2019-06-26T18:39:05Z

shell/common/shell.cc

+  // Only set this if the user has _not_ requested a specific value.
+  if (raster_cache_max_bytes_user_value_ == -1) {
+    // This is the formula Android uses.
+    int max_bytes = metrics.physical_width * metrics.physical_height * 12 * 4;


How did we figure 12 times screen size was a good metric? Seems really large. But I am not sure what it should be. Maybe 3-4?

The android dev who pointed this out to me suggested that it should be lower (I think his suggestion was 5).

I think it's probably fine to make it lower, but this is what Android has.

dnfield · 2019-06-26T20:52:24Z

Tests may have to wait for https://github.com/flutter/engine/pull/9486/files to land, which has some necessary reworking of GL testing harnesses to make testing this possible.

chinmaygarde

LGTM. We are going to pair on images & caching tomorrow and author a GL based shell_unittest and add a test for this then.

liyuqian · 2019-06-26T21:29:22Z

shell/common/rasterizer.h

@@ -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_;


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.

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.

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

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

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.

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

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

This reverts commit 23230e3.

nathanrogersgoogle · 2019-06-26T23:39:32Z

How is this change going to interact with the https://fuchsia.googlesource.com/topaz/+/1b6341661b0bcf57449bf648b1b4f83fbb83ac8e/runtime/flutter_runner/vulkan_surface_producer.cc#127 we do on Fuchsia?

dnfield · 2019-06-26T23:43:11Z

Hmm. This would end up overriding it. I was under the impression you were using the system channel that Jason implemented. If you send a system channel, we won't override it.

dnfield · 2019-06-27T00:06:32Z

Should we revert this in the mean time? It seems likely to me that this patch would end up increasing the cache size on Fuchsia. I can try to verify that.

flutter/engine@8591bd3...f3761ba git log 8591bd3..f3761ba --no-merges --oneline f3761ba Roll fuchsia/sdk/core/mac-amd64 from pWygawI3vBzP9dYloEvKka8r1p0NpLLZzZQ-yMYI1UIC to ZZsO1TTl-976ngr5N8h6rRvXTbMO_3qyKDpbeOhE0dwC (flutter/engine#9510) a83fe3e Roll Fuchsia SDK to latest (flutter/engine#9509) 63c2c33 Improve caching limits for Skia (flutter/engine#9503) 8876f6e Revert Skia version to d8f79a27b06b5bce7a27f89ce2d43d39f8c058dc (flutter/engine#9507) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff ([email protected]), and stop the roller if necessary.

nathanrogersgoogle · 2019-06-27T03:38:49Z

I'm not familiar with the "system channel that Jason implemented" thing? Can you provide a github/googlesource/codesearch link?

We probably don't need to full on revert this change, but we should be mindful of the effect that is has on Fuchsia.

dnfield · 2019-06-27T06:07:33Z

Sorry for being vague there, I assumed you were the one it was implemented for (I think it was actually requested by someone else).

Flutter provides a system channel to control this value. A Flutter app/mod, from Dart code, can tell the engine to use X number of bytes for the resource cache. This would override whatever was picked in the vulkan_surface_producer.cc anyway. The interface is here: https://github.com/flutter/flutter/blob/v1.5.4-hotfix.2/packages/flutter/lib/src/services/system_channels.dart#L232

That gets pumped through to the same SetResourceCacheMaxBytes call eventually, and can be called at any point in the mod's lifecycle.

abhishekamit · 2019-07-09T22:05:16Z

shell/common/shell.cc

@@ -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.


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(

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.

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.

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?

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.

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

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?

This reverts commit 63c2c33.

flutter/engine@8591bd3...f3761ba git log 8591bd3..f3761ba --no-merges --oneline f3761ba Roll fuchsia/sdk/core/mac-amd64 from pWygawI3vBzP9dYloEvKka8r1p0NpLLZzZQ-yMYI1UIC to ZZsO1TTl-976ngr5N8h6rRvXTbMO_3qyKDpbeOhE0dwC (flutter/engine#9510) a83fe3e Roll Fuchsia SDK to latest (flutter/engine#9509) 63c2c33 Improve caching limits for Skia (flutter/engine#9503) 8876f6e Revert Skia version to d8f79a27b06b5bce7a27f89ce2d43d39f8c058dc (flutter/engine#9507) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff ([email protected]), and stop the roller if necessary.

Improve caching limits for Skia

6e8e0c6

dnfield requested review from chinmaygarde, jason-simmons and liyuqian June 26, 2019 17:31

googlebot added the cla: yes label Jun 26, 2019

format

8ef6360

liyuqian reviewed Jun 26, 2019

View reviewed changes

jason-simmons suggested changes Jun 26, 2019

View reviewed changes

chinmaygarde reviewed Jun 26, 2019

View reviewed changes

dnfield added 2 commits June 26, 2019 13:25

review

dd2be3b

fix types, remove temp log

abfd34e

chinmaygarde approved these changes Jun 26, 2019

View reviewed changes

liyuqian reviewed Jun 26, 2019

View reviewed changes

dnfield added 2 commits June 26, 2019 15:21

std::atomic<bool>

23230e3

Revert "std::atomic<bool>"

6318e94

This reverts commit 23230e3.

dnfield requested a review from jason-simmons June 26, 2019 22:23

jason-simmons approved these changes Jun 26, 2019

View reviewed changes

dnfield merged commit 63c2c33 into flutter:master Jun 26, 2019

dnfield deleted the resource_cache branch June 26, 2019 23:21

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 27, 2019

63c2c33 Improve caching limits for Skia (flutter/engine#9503)

7297295

This was referenced Jun 27, 2019

Roll engine 8591bd32e6b6..a83fe3e56204 (3 commits) flutter/flutter#35151

Closed

Roll engine 8591bd32e6b6..f3761ba8d01f (4 commits) flutter/flutter#35154

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 27, 2019

63c2c33 Improve caching limits for Skia (flutter/engine#9503)

14140f2

liyuqian mentioned this pull request Jun 28, 2019

flutter_gallery__transition_perf regressed flutter/flutter#35296

Closed

abhishekamit reviewed Jul 9, 2019

View reviewed changes

dnfield mentioned this pull request Jul 9, 2019

Make platform_view_android_jni delegate to platform_view_android #9729

Closed

dnfield added a commit that referenced this pull request Jul 10, 2019

Revert "Improve caching limits for Skia (#9503)"

29bd2f4

This reverts commit 63c2c33.

dnfield mentioned this pull request Jul 10, 2019

Revert "Improve caching limits for Skia" #9740

Merged

dnfield added a commit that referenced this pull request Jul 10, 2019

Revert "Improve caching limits for Skia (#9503)" (#9740)

564f53f

This reverts commit 63c2c33.

p30arena mentioned this pull request Jul 29, 2019

App crashes when displaying specific widgets flutter/flutter#36631

Closed

dnfield mentioned this pull request Aug 1, 2019

Reland Skia Caching improvements #10434

Merged

chinmaygarde mentioned this pull request Aug 2, 2019

Memory Leak flutter framework iOS - gridview, listview - it IS a bug... flutter/flutter#32143

Closed

pragmatrix mentioned this pull request Apr 2, 2020

Canvas.draw_path memory usage high with Metal rust-skia/rust-skia#308

Closed

Improve caching limits for Skia #9503

Improve caching limits for Skia #9503

Uh oh!

Conversation

dnfield commented Jun 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfield commented Jun 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield commented Jun 26, 2019

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liyuqian Jun 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nathanrogersgoogle commented Jun 26, 2019

Uh oh!

dnfield commented Jun 26, 2019

Uh oh!

dnfield commented Jun 27, 2019

Uh oh!

nathanrogersgoogle commented Jun 27, 2019

Uh oh!

dnfield commented Jun 27, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield commented Jun 26, 2019 •

edited

Loading

dnfield commented Jun 26, 2019 •

edited

Loading

liyuqian Jun 26, 2019 •

edited

Loading