Remove tech debt related to image disposal and layer GC #26870
Conversation
| ImageReleasedAfterFrame | ||
| #endif // defined(OS_FUCHSIA) | ||
| ) { | ||
| TEST_F(ImageDisposeTest, ImageReleasedAfterFrameAndDisposePictureAndLayer) { |
There was a problem hiding this comment.
Reviewers take note: this re-enables a test that was flaking on Fuchsia.
However, this test no longer relies on GC behavior and should be fully deterministic now.
There was a problem hiding this comment.
It also no longer relies on decoding a massive image, and is much faster now too.
There was a problem hiding this comment.
flutter/flutter#84116 can be closed when this lands.
|
This is ready for review, but I'm going to leave it as a draft until the framework PR that implements this behavior is merged. |
There was a problem hiding this comment.
Dart may wish to deprecate the Dart_HintFreed() API as well after this lands. /cc @rmacnak-google
| // Provide an approximation of the total memory impact of this object to the | ||
| // Dart GC. The ContainerLayer may hold references to a tree of other layers, | ||
| // which in turn may contain Skia objects. | ||
| return 3000; |
There was a problem hiding this comment.
Is the shallow size of the C++ object still tracked as an external allocation for Dart GC?
There was a problem hiding this comment.
That's handled automatically here: https://github.com/flutter/engine/blob/master/third_party/tonic/dart_wrappable.h#L89
| ImageReleasedAfterFrame | ||
| #endif // defined(OS_FUCHSIA) | ||
| ) { | ||
| TEST_F(ImageDisposeTest, ImageReleasedAfterFrameAndDisposePictureAndLayer) { |
There was a problem hiding this comment.
flutter/flutter#84116 can be closed when this lands.
|
The change in the framework landed. This is ready for review. |
| std::shared_ptr<DartIsolate> root_isolate = root_isolate_.lock(); | ||
| if (!root_isolate) { | ||
| return false; | ||
| } | ||
|
|
||
| tonic::DartState::Scope scope(root_isolate); | ||
|
|
||
| // Dart will use the freed hint at the next idle notification. Make sure to | ||
| // Update it with our latest value before calling NotifyIdle. | ||
| Dart_HintFreed(freed_hint); |
There was a problem hiding this comment.
Could you let me know if my understanding is correct here?
Not tracking/providing this hint for freed images won't cause the GC policy to behave poorly for us because:
- Dart references to expensive resources like images is now more tightly managed WRT inclusion in draw lists, and disposal of them is explicit on the framework side, and so...
- These expensive resources are now freed outside the confines of GC runs, removing our need to force the Dart GC gate to consider the size of freed images as they pile up.
There was a problem hiding this comment.
Correct. Running the GC for this is expensive and no longer needed
* remove tech debt * Fix test
* remove tech debt * Fix test
WIP - do not land until after flutter/flutter#84740
Fixes flutter/flutter#84116