Reland Skia Caching improvements #10434

dnfield · 2019-08-01T23:50:24Z

This relands #9503, which was reverted because it didn't work. The underlying problem of letting embedders get the engine directly to set the viewport metrics has been resolved by #9747

This additionally adds a test to make sure these values get set correctly on the Rasterizer.

This patch is important because our current default (512MB) is simply too large for lower end devices with small amounts of RAM. There have been numerous reports of memory usage related crashes when this cache is allowed to grow to its current limit, e.g. on an iPhone with 1GB of RAM.

/cc @abhishekamit - Please let me know if you have concerns with this patch.

jason-simmons · 2019-08-02T00:25:29Z

shell/common/shell_unittests.cc

  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


Looks like there are some unintentional reformattings in this file

Yeah clang format reflowed some comments that I then uncommented. Will fix

chinmaygarde · 2019-08-02T00:31:36Z

shell/common/shell_test.cc

+      shell->GetTaskRunners().GetPlatformTaskRunner(),
+      [shell, &latch, message = std::move(message)]() {
+        if (!shell->weak_engine_) {
+          return;


You missed signaling the latch here. Consider instead:

if (auto engine = shell->weak_engine_) { engine->Handle(...); } latch.Signal();

Ahh right, thanks. Fixed.

chinmaygarde · 2019-08-02T00:38:16Z

shell/common/rasterizer.cc

 }

-void Rasterizer::SetResourceCacheMaxBytes(int max_bytes) {
+void Rasterizer::SetResourceCacheMaxBytes(size_t max_bytes, bool from_user) {


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 in Rasterizer::Setup, you could use value_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.

chinmaygarde · 2019-08-02T00:39:06Z

shell/common/rasterizer.cc

+    return;
+  }
+
  GrContext* context = surface_->GetContext();


Making this call after Rasterizer::Teardown but before the next Setup will dereference a null pointer.

chinmaygarde · 2019-08-02T00:39:38Z

shell/common/shell.cc


+  // 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;


You can move this to a constant in rasterizer and use std::optionals default as discussed above.

I'm not sure I'm clear on which part should be the constant. Just the 12 * 4 part?

maybe a static method on rasterizer that takes width and height?

chinmaygarde · 2019-08-02T20:36:43Z

shell/common/rasterizer.h

+  /// @brief      The current value of Skia's resource cache size, if a surface
+  ///             is present.
+  ///
+  /// @attention  This cache setting will be invalidated when the surface is


This attention block is not true anymore. You just fixed it :)

chinmaygarde · 2019-08-02T20:39:30Z

shell/common/rasterizer.cc

 }

+std::optional<size_t> Rasterizer::GetResourceCacheMaxBytes() const {
+  if (!surface_) {


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.

What happens if this is called before the first setup, and before anyone has attempted to set the metrics?

Return kGrCacheMaxByteSize maybe?

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.

Ok then. Bad idea. Thanks for clarifying.

chinmaygarde · 2019-08-02T20:40:03Z

shell/common/rasterizer.h

+  ///
+  /// @return     The size of Skia's resource cache, if available.
+  ///
+  std::optional<size_t> GetResourceCacheMaxBytes() const;


Per discussion above, this can now be just size_t GetResourceCacheMaxBytes() const;

chinmaygarde · 2019-08-02T20:42:09Z

shell/common/shell_unittests.cc

  FrameTiming timing;
  for (auto phase : FrameTiming::kPhases) {
-    ASSERT_TRUE(phase > lastPhaseIndex);  // Ensure that kPhases are in order.
+    ASSERT_TRUE(phase > lastPhaseIndex);  // Ensure that kPhases are in ƒorder.


I don't think you meant to add the function symbol here.

chinmaygarde · 2019-08-02T20:43:58Z

shell/common/shell_unittests.cc

+  EXPECT_EQ(shell->GetRasterizer()->GetResourceCacheMaxBytes(), 3840000U);
+
+  std::string request_json =
+      "{\"method\": \"Skia.setResourceCacheMaxBytes\", \"args\": 10000}";


Nit: Consider using raw string literals when constructing JSON (properly formatted) inline for readability. Or use RapidJSON.

abhishekamit · 2019-08-02T20:56:39Z

shell/common/shell_unittests.cc

+  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));


Can you make sure a reply is received for this message? I think that's broken right now.

There's no reply on this mesage. It's not implemented at all. What's the use case for it?

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:

engine/shell/common/shell.cc

Line 945 in d937b69

rasterizer->SetResourceCacheMaxBytes(max_bytes);

message->response->CompleteEmpty() should be enough.

I've added an empty reply and a test for it.

in b50cab1

[email protected]:flutter/engine.git/compare/bd988f6cc3d0...9605017 git log bd988f6..9605017 --no-merges --oneline 2019-08-03 [email protected] Reland Skia Caching improvements (flutter/engine#10434) 2019-08-02 [email protected] Delete unused create_macos_gen_snapshot.py script (flutter/engine#10479) 2019-08-02 [email protected] Add #else, #endif condition comments (flutter/engine#10477) 2019-08-02 [email protected] Roll fuchsia/sdk/core/mac-amd64 from eAnGy... to Mzwf3... (flutter/engine#10476) 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.

dnfield added 2 commits August 1, 2019 14:56

Reland Skia cache size reduction

c82f8d0

test

871ead7

dnfield requested review from chinmaygarde, jason-simmons and liyuqian August 1, 2019 23:50

googlebot added the cla: yes label Aug 1, 2019

uncomment tests

a831de6

chinmaygarde mentioned this pull request Aug 2, 2019

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

Closed

jason-simmons reviewed Aug 2, 2019

View reviewed changes

chinmaygarde reviewed Aug 2, 2019

View reviewed changes

dnfield added 4 commits August 1, 2019 20:02

fix bad format

612c38f

test

9456798

fix test

43038a2

optional for get bytes

e130727

chinmaygarde approved these changes Aug 2, 2019

View reviewed changes

doc fix, test fix

9105082

abhishekamit reviewed Aug 2, 2019

View reviewed changes

dnfield added 4 commits August 2, 2019 14:21

fix buildfile

f3892af

fix tests

cd687f8

add empty callback and test

b50cab1

include optional

aa93b16

dnfield merged commit 9605017 into flutter:master Aug 3, 2019

dnfield deleted the skia_cache branch August 3, 2019 02:32

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

9605017 Reland Skia Caching improvements (flutter/engine#10434)

21b5698

engine-flutter-autoroll mentioned this pull request Aug 3, 2019

Roll engine bd988f6cc3d0..960501721b23 (4 commits) flutter/flutter#37531

Merged

cfontas pushed a commit to cfontas/engine that referenced this pull request Aug 8, 2019

Reland Skia Caching improvements (flutter#10434)

f423e64

dnfield mentioned this pull request Aug 9, 2019

Setting skia cache size causes crash in release mode on android flutter/flutter#35890

Closed

abhishekamit mentioned this pull request Aug 27, 2019

Return a non-empty response from setResourceCacheMaxBytes flutter/flutter#39337

Closed

dnfield mentioned this pull request Nov 26, 2019

Android memory leaks until it is killed by the system flutter/flutter#31303

Closed

Reland Skia Caching improvements #10434

Reland Skia Caching improvements #10434

Uh oh!

Conversation

dnfield commented Aug 1, 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!

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dnfield commented Aug 1, 2019 •

edited

Loading