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

Remove tech debt related to image disposal and layer GC #26870

Merged
merged 4 commits into from
Jun 25, 2021
Merged

Remove tech debt related to image disposal and layer GC #26870

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
65dd1f6
remove tech debt
dnfield Jun 17, 2021
2f37c27
Merge remote-tracking branch 'upstream/master' into img_dispose
dnfield Jun 18, 2021
b909099
Merge remote-tracking branch 'upstream/master' into img_dispose
dnfield Jun 22, 2021
c3d396e
Fix test
dnfield Jun 22, 2021
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
1 change: 0 additions & 1 deletion ci/licenses_golden/licenses_flutter
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,6 @@ FILE: ../../../flutter/lib/ui/fixtures/hello_loop_2.webp
FILE: ../../../flutter/lib/ui/fixtures/ui_test.dart
FILE: ../../../flutter/lib/ui/geometry.dart
FILE: ../../../flutter/lib/ui/hash_codes.dart
FILE: ../../../flutter/lib/ui/hint_freed_delegate.h
FILE: ../../../flutter/lib/ui/hooks.dart
FILE: ../../../flutter/lib/ui/hooks_unittests.cc
FILE: ../../../flutter/lib/ui/io_manager.h
Expand Down
38 changes: 22 additions & 16 deletions lib/ui/fixtures/ui_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ import 'dart:ui';

void main() {}

/// Mutiple tests use this to signal to the C++ side that they are ready for
/// validation.
void _finish() native 'Finish';

@pragma('vm:entry-point')
void validateSceneBuilderAndScene() {
final SceneBuilder builder = SceneBuilder();
Expand Down Expand Up @@ -60,7 +64,6 @@ Future<void> createSingleFrameCodec() async {
_finish();
}
void _validateCodec(Codec codec) native 'ValidateCodec';
void _finish() native 'Finish';

@pragma('vm:entry-point')
void createVertices() {
Expand Down Expand Up @@ -232,8 +235,8 @@ void _validateExternal(Uint8List result) native 'ValidateExternal';

@pragma('vm:entry-point')
Future<void> pumpImage() async {
const int width = 6000;
const int height = 6000;
const int width = 60;
const int height = 60;
final Completer<Image> completer = Completer<Image>();
decodeImageFromPixels(
Uint8List.fromList(List<int>.filled(width * height * 4, 0xFF)),
Expand All @@ -243,50 +246,53 @@ Future<void> pumpImage() async {
(Image image) => completer.complete(image),
);
final Image image = await completer.future;
late Picture picture;
late OffsetEngineLayer layer;

final FrameCallback renderBlank = (Duration duration) {
void renderBlank(Duration duration) {
image.dispose();
picture.dispose();
layer.dispose();

final PictureRecorder recorder = PictureRecorder();
final Canvas canvas = Canvas(recorder);
canvas.drawRect(Rect.largest, Paint());
final Picture picture = recorder.endRecording();

canvas.drawPaint(Paint());
picture = recorder.endRecording();
final SceneBuilder builder = SceneBuilder();
layer = builder.pushOffset(0, 0);
builder.addPicture(Offset.zero, picture);

final Scene scene = builder.build();
window.render(scene);
scene.dispose();
window.onBeginFrame = (Duration duration) {
window.onDrawFrame = _onBeginFrameDone;
};
window.scheduleFrame();
};

final FrameCallback renderImage = (Duration duration) {
_finish();
}

void renderImage(Duration duration) {
final PictureRecorder recorder = PictureRecorder();
final Canvas canvas = Canvas(recorder);
canvas.drawImage(image, Offset.zero, Paint());
final Picture picture = recorder.endRecording();
picture = recorder.endRecording();

final SceneBuilder builder = SceneBuilder();
layer = builder.pushOffset(0, 0);
builder.addPicture(Offset.zero, picture);

_captureImageAndPicture(image, picture);

final Scene scene = builder.build();
window.render(scene);
scene.dispose();

window.onBeginFrame = renderBlank;
window.scheduleFrame();
};
}

window.onBeginFrame = renderImage;
window.scheduleFrame();
}
void _captureImageAndPicture(Image image, Picture picture) native 'CaptureImageAndPicture';
Future<void> _onBeginFrameDone() native 'OnBeginFrameDone';

@pragma('vm:entry-point')
void hooksTests() {
Expand Down
24 changes: 0 additions & 24 deletions lib/ui/hint_freed_delegate.h
View file
Open in desktop

This file was deleted.

10 changes: 0 additions & 10 deletions lib/ui/painting/engine_layer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,6 @@ EngineLayer::EngineLayer(std::shared_ptr<flutter::ContainerLayer> layer)

EngineLayer::~EngineLayer() = default;

size_t EngineLayer::GetAllocationSize() const {
// TODO(dnfield): Remove this when scene disposal changes land in the
// framework. https://github.com/flutter/flutter/issues/81514

// 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;
Copy link
Member

Choose a reason for hiding this comment

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

Is the shallow size of the C++ object still tracked as an external allocation for Dart GC?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's handled automatically here: https://github.com/flutter/engine/blob/master/third_party/tonic/dart_wrappable.h#L89

};

void EngineLayer::dispose() {
layer_.reset();
ClearDartWrapper();
Expand Down
2 changes: 0 additions & 2 deletions lib/ui/painting/engine_layer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@ class EngineLayer : public RefCountedDartWrappable<EngineLayer> {
public:
~EngineLayer() override;

size_t GetAllocationSize() const override;

static void MakeRetained(Dart_Handle dart_handle,
std::shared_ptr<flutter::ContainerLayer> layer) {
auto engine_layer = fml::MakeRefCounted<EngineLayer>(layer);
Expand Down
6 changes: 0 additions & 6 deletions lib/ui/painting/image.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,6 @@ Dart_Handle CanvasImage::toByteData(int format, Dart_Handle callback) {
}

void CanvasImage::dispose() {
// TODO(dnfield): Remove the hint freed delegate once Picture disposal is in
// the framework https://github.com/flutter/flutter/issues/81514
auto hint_freed_delegate = UIDartState::Current()->GetHintFreedDelegate();
if (hint_freed_delegate) {
hint_freed_delegate->HintFreed(GetAllocationSize());
}
image_.reset();
ClearDartWrapper();
}
Expand Down
33 changes: 5 additions & 28 deletions lib/ui/painting/image_dispose_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,23 +30,11 @@ class ImageDisposeTest : public ShellTest {
// Used to wait on Dart callbacks or Shell task runner flushing
fml::AutoResetWaitableEvent message_latch_;

fml::AutoResetWaitableEvent picture_finalizer_latch_;
static void picture_finalizer(void* isolate_callback_data, void* peer) {
auto latch = reinterpret_cast<fml::AutoResetWaitableEvent*>(peer);
latch->Signal();
}

sk_sp<SkPicture> current_picture_;
sk_sp<SkImage> current_image_;
};

TEST_F(ImageDisposeTest,
#if defined(OS_FUCHSIA)
DISABLED_ImageReleasedAfterFrame
#else
ImageReleasedAfterFrame
#endif // defined(OS_FUCHSIA)
) {
TEST_F(ImageDisposeTest, ImageReleasedAfterFrameAndDisposePictureAndLayer) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It also no longer relies on decoding a massive image, and is much faster now too.

Copy link
Member

Choose a reason for hiding this comment

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

flutter/flutter#84116 can be closed when this lands.

auto native_capture_image_and_picture = [&](Dart_NativeArguments args) {
auto image_handle = Dart_GetNativeArgument(args, 0);
auto native_image_handle =
Expand All @@ -60,12 +48,9 @@ TEST_F(ImageDisposeTest,
ASSERT_FALSE(picture->picture()->unique());
current_image_ = image->image();
current_picture_ = picture->picture();

Dart_NewFinalizableHandle(Dart_GetNativeArgument(args, 1),
&picture_finalizer_latch_, 0, &picture_finalizer);
};

auto native_on_begin_frame_done = [&](Dart_NativeArguments args) {
auto native_finish = [&](Dart_NativeArguments args) {
message_latch_.Signal();
};

Expand All @@ -80,8 +65,7 @@ TEST_F(ImageDisposeTest,

AddNativeCallback("CaptureImageAndPicture",
CREATE_NATIVE_ENTRY(native_capture_image_and_picture));
AddNativeCallback("OnBeginFrameDone",
CREATE_NATIVE_ENTRY(native_on_begin_frame_done));
AddNativeCallback("Finish", CREATE_NATIVE_ENTRY(native_finish));

std::unique_ptr<Shell> shell = CreateShell(std::move(settings), task_runners);

Expand All @@ -103,15 +87,8 @@ TEST_F(ImageDisposeTest,
ASSERT_TRUE(current_picture_);
ASSERT_TRUE(current_image_);

// Simulate a large notify idle, as the animator would do
// when it has no frames left.
// On slower machines, this is especially important - we capture that
// this happens normally in devicelab bnechmarks like large_image_changer.
NotifyIdle(shell.get(), Dart_TimelineGetMicros() + 100000);

picture_finalizer_latch_.Wait();

// Force a drain the SkiaUnrefQueue.
// Force a drain the SkiaUnrefQueue. The engine does this normally as frames
// pump, but we force it here to make the test more deterministic.
message_latch_.Reset();
task_runner->PostTask([&, io_manager = shell->GetIOManager()]() {
io_manager->GetSkiaUnrefQueue()->Drain();
Expand Down
6 changes: 0 additions & 6 deletions lib/ui/ui_dart_state.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ UIDartState::Context::Context(const TaskRunners& task_runners)
UIDartState::Context::Context(
const TaskRunners& task_runners,
fml::WeakPtr<SnapshotDelegate> snapshot_delegate,
fml::WeakPtr<HintFreedDelegate> hint_freed_delegate,
fml::WeakPtr<IOManager> io_manager,
fml::RefPtr<SkiaUnrefQueue> unref_queue,
fml::WeakPtr<ImageDecoder> image_decoder,
Expand All @@ -40,7 +39,6 @@ UIDartState::Context::Context(
std::shared_ptr<VolatilePathTracker> volatile_path_tracker)
: task_runners(task_runners),
snapshot_delegate(snapshot_delegate),
hint_freed_delegate(hint_freed_delegate),
io_manager(io_manager),
unref_queue(unref_queue),
image_decoder(image_decoder),
Expand Down Expand Up @@ -169,10 +167,6 @@ fml::WeakPtr<SnapshotDelegate> UIDartState::GetSnapshotDelegate() const {
return context_.snapshot_delegate;
}

fml::WeakPtr<HintFreedDelegate> UIDartState::GetHintFreedDelegate() const {
return context_.hint_freed_delegate;
}

fml::WeakPtr<GrDirectContext> UIDartState::GetResourceContext() const {
if (!context_.io_manager) {
return {};
Expand Down
8 changes: 0 additions & 8 deletions lib/ui/ui_dart_state.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
#include "flutter/fml/build_config.h"
#include "flutter/fml/memory/weak_ptr.h"
#include "flutter/fml/synchronization/waitable_event.h"
#include "flutter/lib/ui/hint_freed_delegate.h"
#include "flutter/lib/ui/io_manager.h"
#include "flutter/lib/ui/isolate_name_server/isolate_name_server.h"
#include "flutter/lib/ui/painting/image_decoder.h"
Expand Down Expand Up @@ -46,7 +45,6 @@ class UIDartState : public tonic::DartState {

Context(const TaskRunners& task_runners,
fml::WeakPtr<SnapshotDelegate> snapshot_delegate,
fml::WeakPtr<HintFreedDelegate> hint_freed_delegate,
fml::WeakPtr<IOManager> io_manager,
fml::RefPtr<SkiaUnrefQueue> unref_queue,
fml::WeakPtr<ImageDecoder> image_decoder,
Expand All @@ -65,10 +63,6 @@ class UIDartState : public tonic::DartState {
/// of Flutter view hierarchies.
fml::WeakPtr<SnapshotDelegate> snapshot_delegate;

/// The delegate used by the isolate to hint the Dart VM when additional
/// memory may be freed if a GC ran at the next NotifyIdle.
fml::WeakPtr<HintFreedDelegate> hint_freed_delegate;

/// The IO manager used by the isolate for asynchronous texture uploads.
fml::WeakPtr<IOManager> io_manager;

Expand Down Expand Up @@ -126,8 +120,6 @@ class UIDartState : public tonic::DartState {

fml::WeakPtr<SnapshotDelegate> GetSnapshotDelegate() const;

fml::WeakPtr<HintFreedDelegate> GetHintFreedDelegate() const;

fml::WeakPtr<GrDirectContext> GetResourceContext() const;

fml::WeakPtr<ImageDecoder> GetImageDecoder() const;
Expand Down
2 changes: 0 additions & 2 deletions runtime/dart_isolate.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@ std::weak_ptr<DartIsolate> DartIsolate::SpawnIsolate(
const Settings& settings,
std::unique_ptr<PlatformConfiguration> platform_configuration,
fml::WeakPtr<SnapshotDelegate> snapshot_delegate,
fml::WeakPtr<HintFreedDelegate> hint_freed_delegate,
std::string advisory_script_uri,
std::string advisory_script_entrypoint,
Flags flags,
Expand All @@ -103,7 +102,6 @@ std::weak_ptr<DartIsolate> DartIsolate::SpawnIsolate(
std::move(isolate_configration), //
UIDartState::Context{GetTaskRunners(), //
snapshot_delegate, //
hint_freed_delegate, //
GetIOManager(), //
GetSkiaUnrefQueue(), //
GetImageDecoder(), //
Expand Down
2 changes: 0 additions & 2 deletions runtime/dart_isolate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
#include "flutter/fml/compiler_specific.h"
#include "flutter/fml/macros.h"
#include "flutter/fml/mapping.h"
#include "flutter/lib/ui/hint_freed_delegate.h"
#include "flutter/lib/ui/io_manager.h"
#include "flutter/lib/ui/snapshot_delegate.h"
#include "flutter/lib/ui/ui_dart_state.h"
Expand Down Expand Up @@ -230,7 +229,6 @@ class DartIsolate : public UIDartState {
const Settings& settings,
std::unique_ptr<PlatformConfiguration> platform_configuration,
fml::WeakPtr<SnapshotDelegate> snapshot_delegate,
fml::WeakPtr<HintFreedDelegate> hint_freed_delegate,
std::string advisory_script_uri,
std::string advisory_script_entrypoint,
Flags flags,
Expand Down
1 change: 0 additions & 1 deletion runtime/dart_isolate_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,6 @@ TEST_F(DartIsolateTest, SpawnIsolate) {
/*settings=*/vm_data->GetSettings(),
/*platform_configuration=*/nullptr,
/*snapshot_delegate=*/{},
/*hint_freed_delegate=*/{},
/*advisory_script_uri=*/"main.dart",
/*advisory_script_entrypoint=*/"main",
/*flags=*/DartIsolate::Flags{},
Expand Down
5 changes: 1 addition & 4 deletions runtime/runtime_controller.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,17 +195,14 @@ bool RuntimeController::ReportTimings(std::vector<int64_t> timings) {
return false;
}

bool RuntimeController::NotifyIdle(int64_t deadline, size_t freed_hint) {
bool RuntimeController::NotifyIdle(int64_t deadline) {
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);
Copy link
Member

@bdero bdero Jun 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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:

  1. 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...
  2. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. Running the GC for this is expensive and no longer needed

Dart_NotifyIdle(deadline);

// Idle notifications being in isolate scope are part of the contract.
Expand Down
4 changes: 1 addition & 3 deletions runtime/runtime_controller.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,12 +341,10 @@ class RuntimeController : public PlatformConfigurationClient {
/// @param[in] deadline The deadline measures in microseconds against the
/// system's monotonic time. The clock can be accessed via
/// `Dart_TimelineGetMicros`.
/// @param[in] freed_hint A hint of the number of bytes potentially freed
/// since the last call to NotifyIdle if a GC were run.
///
/// @return If the idle notification was forwarded to the running isolate.
///
virtual bool NotifyIdle(int64_t deadline, size_t freed_hint);
virtual bool NotifyIdle(int64_t deadline);

//----------------------------------------------------------------------------
/// @brief Returns if the root isolate is running. The isolate must be
Expand Down
Loading