Hint freed #19842

dnfield · 2020-07-17T15:32:24Z

Description

Uses the new Dart_HintFreed API to notify the VM when the engine is done with a layer tree, and that a collection could potentially free memory.

This assumes https://dart-review.googlesource.com/c/sdk/+/150503 will land.

/cc @a-siva @rmacnak-google @chinmaygarde

Related Issues

flutter/flutter#56482

Tests

I added the following tests:

Unit tests that PictureLayer reports what it should
Unit test that showing an image, showing a new frame, and then notifying idle (which normally would happen in a "regular app" as frames are rendered) releases the SkImage/SkPicture.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

No, no existing tests failed, so this is not a breaking change.

zanderso · 2020-08-14T15:53:36Z

lib/ui/fixtures/ui_test.dart

+}
+
+void _captureImageAndPicture(Image image, Picture picture) native 'CaptureImageAndPicture';
+Future<void> _done() native 'Done';
 void _encodeImage(Image i, int format, void Function(Uint8List result))


I'd leave these adjacent to the Dart code above that calls them.

(Even better would be to split these different fixtures into separate files, but this PR is probably big enough already)

Ah, bad merge. Fixed.

zanderso · 2020-08-14T15:54:29Z

lib/ui/painting/image_release_unittests.cc

+// Copyright 2013 The Flutter Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+// FLUTTER_NOLINT


When we add new files, we should have the lints enabled.

zanderso · 2020-08-14T15:56:40Z

lib/ui/painting/image_release_unittests.cc

+}
+
+TEST_F(ShellTest, ImageReleasedAfterFrame) {
+  fml::AutoResetWaitableEvent message_latch;


You might be getting a lint for the lifetime of this? If so you can make it a global or (I think) a field of a new subclass of ShellTest

You are very smart.

zanderso · 2020-08-14T16:01:31Z

flow/layers/layer_tree_unittests.cc

@@ -33,11 +37,22 @@ class LayerTreeTest : public CanvasTest {
  CompositorContext::ScopedFrame& frame() { return *scoped_frame_.get(); }
  const SkMatrix& root_transform() { return root_transform_; }

+  void OnCompositorEndFrame(size_t freed_hint) override {


// |CompositorContext::Delegate|

Done and done

zanderso · 2020-08-14T16:01:42Z

flow/layers/layer_tree_unittests.cc

+    last_freed_hint_ = freed_hint;
+  }
+
+  fml::Milliseconds GetFrameBudget() override {


zanderso · 2020-08-14T16:02:02Z

shell/common/shell.h

@@ -528,10 +529,14 @@ class Shell final : public PlatformView::Delegate,
  void OnFrameRasterized(const FrameTiming&) override;

  // |Rasterizer::Delegate|
-  fml::Milliseconds GetFrameBudget() override;
+  fml::TimePoint GetLatestFrameTargetTime() const override;

  // |Rasterizer::Delegate|


Is this stale?

I'm not sure which line you're referring to - I moved this member up from the Rasterizer delegate to the CompositorContext delegate (Which the rasterizer delegate is now a subclass of).

Ah, okay. I was just confused by the two // |...| comments, and thought one of them might be extra.

zanderso · 2020-08-14T16:03:02Z

runtime/runtime_controller.cc

  std::shared_ptr<DartIsolate> root_isolate = root_isolate_.lock();
  if (!root_isolate) {
    return false;
  }

  tonic::DartState::Scope scope(root_isolate);

+  Dart_HintFreed(freed_hint);


If the ordering of these two calls is important, then a comment here explaining that will probably be helpful.

zanderso · 2020-08-14T17:57:09Z

It looks like the new test is failing in the presubmit checks.

dnfield · 2020-08-14T19:02:34Z

I fixed up a merge cconflict, but I'm wondering if this test might be flaky. I trie drunning it 50 times locally and it did not fail, but it did fail on CI once.

zanderso · 2020-08-14T19:49:43Z

lib/ui/fixtures/ui_test.dart

+    final Scene scene = builder.build();
+    window.render(scene);
+    scene.dispose();
+    window.onBeginFrame = (Duration duration) => _done();


Should _done go on window.onDrawFrame. The docs say that that will flush the microtask queue before running it, which may reduce flakiness?

Looks like that worked - just had to put it in onDrawFrame on the next frame.

zanderso

lgtm but this could probably also use a review from @chinmaygarde

chinmaygarde

The access to the engine in the shell delegate callback is unsafe. Also, I am still trying to convince myself that there is no data race or race condition in test (I am not sure yet but will keep trying to convince myself)

chinmaygarde · 2020-08-18T22:50:53Z

flow/layers/layer_tree_unittests.cc

@@ -15,11 +15,15 @@
 namespace flutter {
 namespace testing {

-class LayerTreeTest : public CanvasTest {
+class CompositorDelegate : public CompositorContext::Delegate {


Did you mean to leave this in? It is not used and this class cannot be instantiated anyway since it is virtual. You may have wanted to use this in the fixture but instead opted to subclass it directly.

Nope, will delete.

chinmaygarde · 2020-08-18T22:54:00Z

lib/ui/painting/image_release_unittests.cc

+};
+
+TEST_F(ImageReleaseTest, ImageReleasedAfterFrame) {
+  auto nativeCaptureImageAndPicture = [&](Dart_NativeArguments args) {


Very minor nit: We don't use camelCase for locals.

Here and elsewhere.

chinmaygarde · 2020-08-18T23:03:55Z

lib/ui/painting/image_release_unittests.cc

+
+  AddNativeCallback("CaptureImageAndPicture",
+                    CREATE_NATIVE_ENTRY(nativeCaptureImageAndPicture));
+  AddNativeCallback("Done", CREATE_NATIVE_ENTRY(nativeDone));


Just "Done" is a bit too terse IMO. How about onBeginFrameDone since that is when you are making the call in the test.

chinmaygarde · 2020-08-18T23:06:54Z

shell/common/shell.cc

@@ -1205,6 +1205,12 @@ void Shell::OnFrameRasterized(const FrameTiming& timing) {
  }
 }

+void Shell::OnCompositorEndFrame(size_t freed_hint) {
+  if (engine_) {


Shell APIs are only safely accessible on the platform task runner. The engine OTOH can only be accessed on the UI task runner. I believe you are on the raster task runner here. So this this API is not resilient to death of the shell, accesses the engine in a thread unsafe manner and is a non-standard shell method to boot.

Its fine for some methods to have different threading restrictions. But when they veer from convention, please document the same and also add an FML_DCHECK. In this case though, it just not thread-safe so lets fix that first.

Hmm. I want the shell to be the delegate here. This was originally living on the raster task runner.

I should be ok as long as I just jump to the UI task runner for this block with a weak ptr to the engine right?

Do I need to get the weak ptr on the platform task runner?

chinmaygarde · 2020-08-18T23:07:43Z

flow/compositor_context.h

+   public:
+    /// Called at the end of a frame with approximately how many bytes mightbe
+    /// freed if a GC ran now.
+    virtual void OnCompositorEndFrame(size_t freed_hint) = 0;


Please document the threading restrictions that implementors must adhere to. I believe the shell is not thread-unsafe. Details below.

I'm not sure what thread restrictions to document here. This is just called when there's a hint available for bytes freed - shouldn't it be up to the implementer what threads it uses when processing that information?

I meant whoever implements the delegate protocol will receive events on the raster thread. This might be unexpected to the implementer.

chinmaygarde · 2020-08-18T23:13:11Z

lib/ui/painting/image_release_unittests.cc

+  message_latch.Wait();
+
+  ASSERT_TRUE(current_picture_);
+  ASSERT_TRUE(current_image_);


I am trying to convince myself this is not a data race. The latch will be signalled when the frame has begun. However, the current image will be set as the frame is constructed. Not sure how this is working.

My thought was that it's not because Dart gives us to us in the capture call (while we're still latched), and then we unlatch when Dart calls onBeginFrameDone (by which point it has already let go of its references).

Sorry, I was thinking this was onBeginFrame instead of onBeginFrameDone. I think that makes sense now. Thanks.

dnfield · 2020-08-19T18:45:53Z

I've updated the test to avoid racing the garbage collector - I'm not sure this is quite the race @chinmaygarde was concerned about, but it should be less flaky now.

chinmaygarde

I'm not sure this is quite the race ....

No that was not an actual issue but a misreading of the way the test was setup. I clarified it. Thanks and my bad.

chinmaygarde · 2020-08-19T19:01:10Z

flow/compositor_context.h

+   public:
+    /// Called at the end of a frame with approximately how many bytes mightbe
+    /// freed if a GC ran now.
+    virtual void OnCompositorEndFrame(size_t freed_hint) = 0;


I meant whoever implements the delegate protocol will receive events on the raster thread. This might be unexpected to the implementer.

chinmaygarde · 2020-08-19T19:02:22Z

lib/ui/painting/image_release_unittests.cc

+  message_latch.Wait();
+
+  ASSERT_TRUE(current_picture_);
+  ASSERT_TRUE(current_image_);


Sorry, I was thinking this was onBeginFrame instead of onBeginFrameDone. I think that makes sense now. Thanks.

dnfield · 2020-08-19T19:50:48Z

I'm removing the test - it's flaky, and efforts to make it not be flaky are making it pass without these changes. I don't think it's accurately representing the state of the framework, and it really wants to be a benchmark about memory usage and collection.

dnfield · 2020-08-19T19:51:12Z

We still have coverage that OnCompositorFrameEnd is called with the right value(s) in the layer tree unittests.

* Hint the VM when a layer or picture goes out of scope

liyuqian · 2020-08-21T22:10:19Z

flow/raster_cache.h

    std::unique_ptr<RasterCacheResult> image;
  };

  template <class Cache>
-  static void SweepOneCacheAfterFrame(Cache& cache) {
+  static size_t SweepOneCacheAfterFrame(Cache& cache) {


It would be nice to document what size is returned from this function. Currently, it seems to only return the layer external size, and one might ask why not also include the image size of the raster cache. Is it because we only want to report the memory size that's managed by Dart VM so the SkImage memory size is irrelevant?

The image size is included in the layer external size. The SkImage size is really of primary importance here.

However, we have to figure out how to get that out of what the Dart VM sees - @jason-simmons has a patch open to remove that, but we'd still want it here.

This reverts commit 3930ac1.

This caused over-aggressive GCs, which vastly increased CPU usage benchmarks. * Revert "fix build (#20644)" This reverts commit b59793e. * Revert "Hint freed (#19842)" This reverts commit 3930ac1.

dnfield added 3 commits July 14, 2020 22:40

Hint the VM when a layer or picture goes out of scope

bc9f524

Merge remote-tracking branch 'upstream/master' into hint_freed

babe728

tests

b351806

googlebot added the cla: yes label Jul 17, 2020

dnfield added 3 commits August 13, 2020 08:58

merge

0f5ff33

format

d921c88

no lint

b05fac3

dnfield mentioned this pull request Aug 13, 2020

Remove the dummy rasterizer delegate now that flutter_runner is in tree, and cleanup ctor params #20486

Merged

dnfield added 2 commits August 13, 2020 15:02

merge

d6c50a1

fix fuchsia, fix test for AOT

21a9cd9

dnfield force-pushed the hint_freed branch from 957bd3d to 21a9cd9 Compare August 14, 2020 02:04

dnfield requested review from rmacnak-google, zanderso and chinmaygarde August 14, 2020 02:12

dnfield marked this pull request as ready for review August 14, 2020 02:12

missing ctors

5bc0710

zanderso reviewed Aug 14, 2020

View reviewed changes

dnfield added 2 commits August 14, 2020 09:34

zra review

bd32627

Merge branch 'hint_freed' of github.com:dnfield/engine into hint_freed

cd42842

dnfield added 2 commits August 14, 2020 11:50

Merge remote-tracking branch 'upstream/master' into hint_freed

4268cfa

merge

3684db4

zanderso reviewed Aug 14, 2020

View reviewed changes

onDrawFrame

d506040

zanderso approved these changes Aug 14, 2020

View reviewed changes

chinmaygarde suggested changes Aug 18, 2020

View reviewed changes

dnfield added 3 commits August 18, 2020 17:39

chinmay review

3094415

merge

5234e69

format

6a1a628

dnfield added 5 commits August 18, 2020 21:14

fix ctor

d743620

more bad merge

13efdb9

fix thread issues

478d773

format!

0fb859c

deflake

e4be6ec

chinmaygarde approved these changes Aug 19, 2020

View reviewed changes

Remove flaky test

3207082

dnfield added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Aug 19, 2020

dnfield merged commit 3930ac1 into flutter:master Aug 19, 2020

dnfield deleted the hint_freed branch August 19, 2020 21:04

engine-flutter-autoroll mentioned this pull request Aug 19, 2020

Roll Engine from 74d32ecdbd46 to b59793ee20be (6 revisions) flutter/flutter#64196

Closed

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

3930ac1 Hint freed (flutter/engine#19842)

a5a51e6

engine-flutter-autoroll mentioned this pull request Aug 20, 2020

Roll Engine from 74d32ecdbd46 to 26fe9125b5bf (7 revisions) flutter/flutter#64206

Closed

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

3930ac1 Hint freed (flutter/engine#19842)

f8967d8

engine-flutter-autoroll mentioned this pull request Aug 20, 2020

Roll Engine from 74d32ecdbd46 to 89f506ed626b (10 revisions) flutter/flutter#64213

Merged

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

3930ac1 Hint freed (flutter/engine#19842)

aa954b1

gspencergoog pushed a commit to gspencergoog/engine that referenced this pull request Aug 20, 2020

Hint freed (flutter#19842)

483970e

* Hint the VM when a layer or picture goes out of scope

dnfield mentioned this pull request Aug 21, 2020

removing Image widgets does not reduce Graphics memory allocation flutter/flutter#56482

Closed

liyuqian reviewed Aug 21, 2020

View reviewed changes

zanderso mentioned this pull request Aug 24, 2020

average_cpu_usage simple_animation_perf_ios regression.. flutter/flutter#64515

Closed

dnfield added a commit to dnfield/engine that referenced this pull request Aug 25, 2020

Revert "Hint freed (flutter#19842)"

8dde5d0

This reverts commit 3930ac1.

This was referenced Aug 25, 2020

Image dispose flutter/flutter#64582

Closed

Use hint freed specifically for image disposal #20754

Merged

Hint freed #19842

Hint freed #19842

Conversation

dnfield commented Jul 17, 2020

Description

Related Issues

Tests

Breaking Change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanderso commented Aug 14, 2020

dnfield commented Aug 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanderso left a comment

Choose a reason for hiding this comment

chinmaygarde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnfield commented Aug 19, 2020

chinmaygarde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnfield commented Aug 19, 2020

dnfield commented Aug 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment