From fa3cce82866d7853625712df4c28803b739bd347 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Tue, 25 Aug 2020 13:12:41 -0700 Subject: [PATCH 01/12] Use hint freed specifically for image disposal --- lib/ui/hint_freed_delegate.h | 24 ++++++++++++++++++++++++ lib/ui/painting/canvas.cc | 6 ------ lib/ui/painting/canvas.h | 3 --- lib/ui/painting/image.cc | 4 ++++ lib/ui/painting/picture.cc | 18 +++++++----------- lib/ui/painting/picture.h | 9 ++------- lib/ui/painting/picture_recorder.cc | 8 +++----- lib/ui/ui_dart_state.cc | 6 ++++++ lib/ui/ui_dart_state.h | 5 +++++ runtime/dart_isolate.cc | 25 ++++++++++++++++--------- runtime/dart_isolate.h | 3 +++ runtime/dart_isolate_unittests.cc | 2 ++ runtime/dart_lifecycle_unittests.cc | 1 + runtime/runtime_controller.cc | 9 ++++++++- runtime/runtime_controller.h | 11 ++++++++++- shell/common/engine.cc | 18 ++++++++++++------ shell/common/engine.h | 9 ++++++++- testing/dart_isolate_runner.cc | 1 + 18 files changed, 112 insertions(+), 50 deletions(-) create mode 100644 lib/ui/hint_freed_delegate.h diff --git a/lib/ui/hint_freed_delegate.h b/lib/ui/hint_freed_delegate.h new file mode 100644 index 0000000000000..0e4a38b41454c --- /dev/null +++ b/lib/ui/hint_freed_delegate.h @@ -0,0 +1,24 @@ +// 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. + +#ifndef FLUTTER_LIB_UI_HINT_FREED_DELEGATE_H_ +#define FLUTTER_LIB_UI_HINT_FREED_DELEGATE_H_ + +namespace flutter { + +class HintFreedDelegate { + public: + //---------------------------------------------------------------------------- + /// @brief Notifies the engine that native bytes might be freed if a + /// garbage collection ran at the next NotifyIdle period. + /// + /// @param[in] size The number of bytes freed. This size adds to any + /// previously supplied value, rather than replacing. + /// + virtual void HintFreed(size_t size) = 0; +}; + +} // namespace flutter + +#endif // FLUTTER_LIB_UI_HINT_FREED_DELEGATE_H_ diff --git a/lib/ui/painting/canvas.cc b/lib/ui/painting/canvas.cc index c68f0d6001ad6..913730da65d4d 100644 --- a/lib/ui/painting/canvas.cc +++ b/lib/ui/painting/canvas.cc @@ -198,7 +198,6 @@ void Canvas::clipPath(const CanvasPath* path, bool doAntiAlias) { ToDart("Canvas.clipPath called with non-genuine Path.")); return; } - external_allocation_size_ += path->path().approximateBytesUsed(); canvas_->clipPath(path->path(), doAntiAlias); } @@ -310,7 +309,6 @@ void Canvas::drawPath(const CanvasPath* path, ToDart("Canvas.drawPath called with non-genuine Path.")); return; } - external_allocation_size_ += path->path().approximateBytesUsed(); canvas_->drawPath(path->path(), *paint.paint()); } @@ -391,7 +389,6 @@ void Canvas::drawPicture(Picture* picture) { ToDart("Canvas.drawPicture called with non-genuine Picture.")); return; } - external_allocation_size_ += picture->GetAllocationSize(); canvas_->drawPicture(picture->picture().get()); } @@ -424,7 +421,6 @@ void Canvas::drawVertices(const Vertices* vertices, ToDart("Canvas.drawVertices called with non-genuine Vertices.")); return; } - external_allocation_size_ += vertices->GetAllocationSize(); canvas_->drawVertices(vertices->vertices(), blend_mode, *paint.paint()); } @@ -453,7 +449,6 @@ void Canvas::drawAtlas(const Paint& paint, static_assert(sizeof(SkRect) == sizeof(float) * 4, "SkRect doesn't use floats."); - external_allocation_size_ += atlas->GetAllocationSize(); canvas_->drawAtlas( skImage.get(), reinterpret_cast(transforms.data()), reinterpret_cast(rects.data()), @@ -477,7 +472,6 @@ void Canvas::drawShadow(const CanvasPath* path, ->window() ->viewport_metrics() .device_pixel_ratio; - external_allocation_size_ += path->path().approximateBytesUsed(); flutter::PhysicalShapeLayer::DrawShadow(canvas_, path->path(), color, elevation, transparentOccluder, dpr); } diff --git a/lib/ui/painting/canvas.h b/lib/ui/painting/canvas.h index 53492b9667875..3c58bad4e7895 100644 --- a/lib/ui/painting/canvas.h +++ b/lib/ui/painting/canvas.h @@ -169,8 +169,6 @@ class Canvas : public RefCountedDartWrappable { static void RegisterNatives(tonic::DartLibraryNatives* natives); - size_t external_allocation_size() const { return external_allocation_size_; } - private: explicit Canvas(SkCanvas* canvas); @@ -178,7 +176,6 @@ class Canvas : public RefCountedDartWrappable { // which does not transfer ownership. For this reason, we hold a raw // pointer and manually set to null in Clear. SkCanvas* canvas_; - size_t external_allocation_size_ = 0; }; } // namespace flutter diff --git a/lib/ui/painting/image.cc b/lib/ui/painting/image.cc index 7da7c0ad029ec..64474227404a9 100644 --- a/lib/ui/painting/image.cc +++ b/lib/ui/painting/image.cc @@ -37,6 +37,10 @@ Dart_Handle CanvasImage::toByteData(int format, Dart_Handle callback) { } void CanvasImage::dispose() { + auto hint_freed_delegate = UIDartState::Current()->GetHintFreedDelegate(); + if (hint_freed_delegate) { + hint_freed_delegate->HintFreed(GetAllocationSize()); + } image_.reset(); ClearDartWrapper(); } diff --git a/lib/ui/painting/picture.cc b/lib/ui/painting/picture.cc index 1285a6b0921cb..5225d993ba4fb 100644 --- a/lib/ui/painting/picture.cc +++ b/lib/ui/painting/picture.cc @@ -28,20 +28,17 @@ IMPLEMENT_WRAPPERTYPEINFO(ui, Picture); DART_BIND_ALL(Picture, FOR_EACH_BINDING) -fml::RefPtr Picture::Create(Dart_Handle dart_handle, - flutter::SkiaGPUObject picture, - size_t external_allocation_size) { - auto canvas_picture = fml::MakeRefCounted(std::move(picture), - external_allocation_size); +fml::RefPtr Picture::Create( + Dart_Handle dart_handle, + flutter::SkiaGPUObject picture) { + auto canvas_picture = fml::MakeRefCounted(std::move(picture)); canvas_picture->AssociateWithDartWrapper(dart_handle); return canvas_picture; } -Picture::Picture(flutter::SkiaGPUObject picture, - size_t external_allocation_size) - : picture_(std::move(picture)), - external_allocation_size_(external_allocation_size) {} +Picture::Picture(flutter::SkiaGPUObject picture) + : picture_(std::move(picture)) {} Picture::~Picture() = default; @@ -62,8 +59,7 @@ void Picture::dispose() { size_t Picture::GetAllocationSize() const { if (auto picture = picture_.get()) { - return picture->approximateBytesUsed() + sizeof(Picture) + - external_allocation_size_; + return picture->approximateBytesUsed() + sizeof(Picture); } else { return sizeof(Picture); } diff --git a/lib/ui/painting/picture.h b/lib/ui/painting/picture.h index 8f1879e1af7fa..e0158d400ff5e 100644 --- a/lib/ui/painting/picture.h +++ b/lib/ui/painting/picture.h @@ -25,8 +25,7 @@ class Picture : public RefCountedDartWrappable { public: ~Picture() override; static fml::RefPtr Create(Dart_Handle dart_handle, - flutter::SkiaGPUObject picture, - size_t external_allocation_size); + flutter::SkiaGPUObject picture); sk_sp picture() const { return picture_.get(); } @@ -45,14 +44,10 @@ class Picture : public RefCountedDartWrappable { uint32_t height, Dart_Handle raw_image_callback); - size_t external_allocation_size() const { return external_allocation_size_; } - private: - Picture(flutter::SkiaGPUObject picture, - size_t external_allocation_size_); + Picture(flutter::SkiaGPUObject picture); flutter::SkiaGPUObject picture_; - size_t external_allocation_size_; }; } // namespace flutter diff --git a/lib/ui/painting/picture_recorder.cc b/lib/ui/painting/picture_recorder.cc index 5916faa9df3ae..5084f30d7812f 100644 --- a/lib/ui/painting/picture_recorder.cc +++ b/lib/ui/painting/picture_recorder.cc @@ -47,11 +47,9 @@ fml::RefPtr PictureRecorder::endRecording(Dart_Handle dart_picture) { return nullptr; } - fml::RefPtr picture = - Picture::Create(dart_picture, - UIDartState::CreateGPUObject( - picture_recorder_.finishRecordingAsPicture()), - canvas_->external_allocation_size()); + fml::RefPtr picture = Picture::Create( + dart_picture, UIDartState::CreateGPUObject( + picture_recorder_.finishRecordingAsPicture())); canvas_->Invalidate(); canvas_ = nullptr; diff --git a/lib/ui/ui_dart_state.cc b/lib/ui/ui_dart_state.cc index b43a442f849e0..eac218548cfbf 100644 --- a/lib/ui/ui_dart_state.cc +++ b/lib/ui/ui_dart_state.cc @@ -18,6 +18,7 @@ UIDartState::UIDartState( TaskObserverAdd add_callback, TaskObserverRemove remove_callback, fml::WeakPtr snapshot_delegate, + fml::WeakPtr hint_freed_delegate, fml::WeakPtr io_manager, fml::RefPtr skia_unref_queue, fml::WeakPtr image_decoder, @@ -31,6 +32,7 @@ UIDartState::UIDartState( add_callback_(std::move(add_callback)), remove_callback_(std::move(remove_callback)), snapshot_delegate_(std::move(snapshot_delegate)), + hint_freed_delegate_(std::move(hint_freed_delegate)), io_manager_(std::move(io_manager)), skia_unref_queue_(std::move(skia_unref_queue)), image_decoder_(std::move(image_decoder)), @@ -136,6 +138,10 @@ fml::WeakPtr UIDartState::GetSnapshotDelegate() const { return snapshot_delegate_; } +fml::WeakPtr UIDartState::GetHintFreedDelegate() const { + return hint_freed_delegate_; +} + fml::WeakPtr UIDartState::GetResourceContext() const { if (!io_manager_) { return {}; diff --git a/lib/ui/ui_dart_state.h b/lib/ui/ui_dart_state.h index 71755931a30d1..d5c22ffa3ff70 100644 --- a/lib/ui/ui_dart_state.h +++ b/lib/ui/ui_dart_state.h @@ -15,6 +15,7 @@ #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" @@ -60,6 +61,8 @@ class UIDartState : public tonic::DartState { fml::WeakPtr GetSnapshotDelegate() const; + fml::WeakPtr GetHintFreedDelegate() const; + fml::WeakPtr GetResourceContext() const; fml::WeakPtr GetImageDecoder() const; @@ -87,6 +90,7 @@ class UIDartState : public tonic::DartState { TaskObserverAdd add_callback, TaskObserverRemove remove_callback, fml::WeakPtr snapshot_delegate, + fml::WeakPtr hint_freed_delegate, fml::WeakPtr io_manager, fml::RefPtr skia_unref_queue, fml::WeakPtr image_decoder, @@ -113,6 +117,7 @@ class UIDartState : public tonic::DartState { const TaskObserverAdd add_callback_; const TaskObserverRemove remove_callback_; fml::WeakPtr snapshot_delegate_; + fml::WeakPtr hint_freed_delegate_; fml::WeakPtr io_manager_; fml::RefPtr skia_unref_queue_; fml::WeakPtr image_decoder_; diff --git a/runtime/dart_isolate.cc b/runtime/dart_isolate.cc index 06ef693b4b46b..919a9a532c17a 100644 --- a/runtime/dart_isolate.cc +++ b/runtime/dart_isolate.cc @@ -58,6 +58,7 @@ std::weak_ptr DartIsolate::CreateRootIsolate( TaskRunners task_runners, std::unique_ptr platform_configuration, fml::WeakPtr snapshot_delegate, + fml::WeakPtr hint_freed_delegate, fml::WeakPtr io_manager, fml::RefPtr unref_queue, fml::WeakPtr image_decoder, @@ -84,15 +85,16 @@ std::weak_ptr DartIsolate::CreateRootIsolate( auto isolate_data = std::make_unique>( std::shared_ptr(new DartIsolate( - settings, // settings - task_runners, // task runners - std::move(snapshot_delegate), // snapshot delegate - std::move(io_manager), // IO manager - std::move(unref_queue), // Skia unref queue - std::move(image_decoder), // Image Decoder - advisory_script_uri, // advisory URI - advisory_script_entrypoint, // advisory entrypoint - true // is_root_isolate + settings, // settings + task_runners, // task runners + std::move(snapshot_delegate), // snapshot delegate + std::move(hint_freed_delegate), // hint freed delegate + std::move(io_manager), // IO manager + std::move(unref_queue), // Skia unref queue + std::move(image_decoder), // Image Decoder + advisory_script_uri, // advisory URI + advisory_script_entrypoint, // advisory entrypoint + true // is_root_isolate ))); DartErrorString error; @@ -120,6 +122,7 @@ std::weak_ptr DartIsolate::CreateRootIsolate( DartIsolate::DartIsolate(const Settings& settings, TaskRunners task_runners, fml::WeakPtr snapshot_delegate, + fml::WeakPtr hint_freed_delegate, fml::WeakPtr io_manager, fml::RefPtr unref_queue, fml::WeakPtr image_decoder, @@ -130,6 +133,7 @@ DartIsolate::DartIsolate(const Settings& settings, settings.task_observer_add, settings.task_observer_remove, std::move(snapshot_delegate), + std::move(hint_freed_delegate), std::move(io_manager), std::move(unref_queue), std::move(image_decoder), @@ -603,6 +607,7 @@ Dart_Isolate DartIsolate::DartCreateAndStartServiceIsolate( null_task_runners, // task runners nullptr, // platform_configuration {}, // snapshot delegate + {}, // Hint freed delegate {}, // IO Manager {}, // Skia unref queue {}, // Image Decoder @@ -706,6 +711,7 @@ Dart_Isolate DartIsolate::DartIsolateGroupCreateCallback( (*isolate_group_data)->GetSettings(), // settings null_task_runners, // task_runners fml::WeakPtr{}, // snapshot_delegate + fml::WeakPtr{}, // hint_freed_delegate fml::WeakPtr{}, // io_manager fml::RefPtr{}, // unref_queue fml::WeakPtr{}, // image_decoder @@ -749,6 +755,7 @@ bool DartIsolate::DartIsolateInitializeCallback(void** child_callback_data, (*isolate_group_data)->GetSettings(), // settings null_task_runners, // task_runners fml::WeakPtr{}, // snapshot_delegate + fml::WeakPtr{}, // hint_freed_delegate fml::WeakPtr{}, // io_manager fml::RefPtr{}, // unref_queue fml::WeakPtr{}, // image_decoder diff --git a/runtime/dart_isolate.h b/runtime/dart_isolate.h index d3862d577ba73..d1f741518ec21 100644 --- a/runtime/dart_isolate.h +++ b/runtime/dart_isolate.h @@ -13,6 +13,7 @@ #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" @@ -194,6 +195,7 @@ class DartIsolate : public UIDartState { TaskRunners task_runners, std::unique_ptr platform_configuration, fml::WeakPtr snapshot_delegate, + fml::WeakPtr hint_freed_delegate, fml::WeakPtr io_manager, fml::RefPtr skia_unref_queue, fml::WeakPtr image_decoder, @@ -404,6 +406,7 @@ class DartIsolate : public UIDartState { DartIsolate(const Settings& settings, TaskRunners task_runners, fml::WeakPtr snapshot_delegate, + fml::WeakPtr hint_freed_delegate, fml::WeakPtr io_manager, fml::RefPtr unref_queue, fml::WeakPtr image_decoder, diff --git a/runtime/dart_isolate_unittests.cc b/runtime/dart_isolate_unittests.cc index 0e9e4cf59521c..68be0efb8eeaa 100644 --- a/runtime/dart_isolate_unittests.cc +++ b/runtime/dart_isolate_unittests.cc @@ -51,6 +51,7 @@ TEST_F(DartIsolateTest, RootIsolateCreationAndShutdown) { std::move(task_runners), // task runners nullptr, // window {}, // snapshot delegate + {}, // hint freed delegate {}, // io manager {}, // unref queue {}, // image decoder @@ -85,6 +86,7 @@ TEST_F(DartIsolateTest, IsolateShutdownCallbackIsInIsolateScope) { std::move(task_runners), // task runners nullptr, // window {}, // snapshot delegate + {}, // hint freed delegate {}, // io manager {}, // unref queue {}, // image decoder diff --git a/runtime/dart_lifecycle_unittests.cc b/runtime/dart_lifecycle_unittests.cc index 94e3598038d4d..3c45c702a80fe 100644 --- a/runtime/dart_lifecycle_unittests.cc +++ b/runtime/dart_lifecycle_unittests.cc @@ -56,6 +56,7 @@ static std::shared_ptr CreateAndRunRootIsolate( runners, // task_runners {}, // window {}, // snapshot_delegate + {}, // hint_freed_delegate {}, // io_manager {}, // unref_queue {}, // image_decoder diff --git a/runtime/runtime_controller.cc b/runtime/runtime_controller.cc index f66cfed778660..7761bee4ea7ea 100644 --- a/runtime/runtime_controller.cc +++ b/runtime/runtime_controller.cc @@ -26,6 +26,7 @@ RuntimeController::RuntimeController( fml::RefPtr p_isolate_snapshot, TaskRunners p_task_runners, fml::WeakPtr p_snapshot_delegate, + fml::WeakPtr p_hint_freed_delegate, fml::WeakPtr p_io_manager, fml::RefPtr p_unref_queue, fml::WeakPtr p_image_decoder, @@ -41,6 +42,7 @@ RuntimeController::RuntimeController( isolate_snapshot_(std::move(p_isolate_snapshot)), task_runners_(p_task_runners), snapshot_delegate_(p_snapshot_delegate), + hint_freed_delegate_(p_hint_freed_delegate), io_manager_(p_io_manager), unref_queue_(p_unref_queue), image_decoder_(p_image_decoder), @@ -61,6 +63,7 @@ RuntimeController::RuntimeController( task_runners_, // std::make_unique(this), // snapshot_delegate_, // + hint_freed_delegate_, // io_manager_, // unref_queue_, // image_decoder_, // @@ -122,6 +125,7 @@ std::unique_ptr RuntimeController::Clone() const { isolate_snapshot_, // task_runners_, // snapshot_delegate_, // + hint_freed_delegate_, // io_manager_, // unref_queue_, // image_decoder_, // @@ -233,7 +237,7 @@ bool RuntimeController::ReportTimings(std::vector timings) { return false; } -bool RuntimeController::NotifyIdle(int64_t deadline) { +bool RuntimeController::NotifyIdle(int64_t deadline, size_t freed_hint) { std::shared_ptr root_isolate = root_isolate_.lock(); if (!root_isolate) { return false; @@ -241,6 +245,9 @@ bool RuntimeController::NotifyIdle(int64_t deadline) { 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); Dart_NotifyIdle(deadline); // Idle notifications being in isolate scope are part of the contract. diff --git a/runtime/runtime_controller.h b/runtime/runtime_controller.h index 18adbc2c1d720..4767d01fbcb13 100644 --- a/runtime/runtime_controller.h +++ b/runtime/runtime_controller.h @@ -67,6 +67,11 @@ class RuntimeController : public PlatformConfigurationClient { /// @param[in] snapshot_delegate The snapshot delegate used by the /// isolate to gather raster snapshots /// of Flutter view hierarchies. + /// @param[in] hint_freed_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. /// @param[in] io_manager The IO manager used by the isolate /// for asynchronous texture uploads. /// @param[in] unref_queue The unref queue used by the @@ -111,6 +116,7 @@ class RuntimeController : public PlatformConfigurationClient { fml::RefPtr isolate_snapshot, TaskRunners task_runners, fml::WeakPtr snapshot_delegate, + fml::WeakPtr hint_freed_delegate, fml::WeakPtr io_manager, fml::RefPtr unref_queue, fml::WeakPtr image_decoder, @@ -328,10 +334,12 @@ 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. /// - bool NotifyIdle(int64_t deadline); + bool NotifyIdle(int64_t deadline, size_t freed_hint); //---------------------------------------------------------------------------- /// @brief Returns if the root isolate is running. The isolate must be @@ -464,6 +472,7 @@ class RuntimeController : public PlatformConfigurationClient { fml::RefPtr isolate_snapshot_; TaskRunners task_runners_; fml::WeakPtr snapshot_delegate_; + fml::WeakPtr hint_freed_delegate_; fml::WeakPtr io_manager_; fml::RefPtr unref_queue_; fml::WeakPtr image_decoder_; diff --git a/shell/common/engine.cc b/shell/common/engine.cc index 315780f23346b..f7171b2fe6c1f 100644 --- a/shell/common/engine.cc +++ b/shell/common/engine.cc @@ -76,11 +76,12 @@ Engine::Engine(Delegate& delegate, io_manager, nullptr) { runtime_controller_ = std::make_unique( - *this, // runtime delegate - &vm, // VM - std::move(isolate_snapshot), // isolate snapshot - task_runners_, // task runners - std::move(snapshot_delegate), + *this, // runtime delegate + &vm, // VM + std::move(isolate_snapshot), // isolate snapshot + task_runners_, // task runners + std::move(snapshot_delegate), // snapshot delegate + GetWeakPtr(), // hint freed delegate std::move(io_manager), // io manager std::move(unref_queue), // Skia unref queue image_decoder_.GetWeakPtr(), // image decoder @@ -248,11 +249,16 @@ void Engine::ReportTimings(std::vector timings) { runtime_controller_->ReportTimings(std::move(timings)); } +void Engine::HintFreed(size_t size) { + hint_freed_bytes_since_last_idle_ += size; +} + void Engine::NotifyIdle(int64_t deadline) { auto trace_event = std::to_string(deadline - Dart_TimelineGetMicros()); TRACE_EVENT1("flutter", "Engine::NotifyIdle", "deadline_now_delta", trace_event.c_str()); - runtime_controller_->NotifyIdle(deadline); + runtime_controller_->NotifyIdle(deadline, hint_freed_bytes_since_last_idle_); + hint_freed_bytes_since_last_idle_ = 0; } std::pair Engine::GetUIIsolateReturnCode() { diff --git a/shell/common/engine.h b/shell/common/engine.h index d16ae0ffed2c7..e4de94846e62f 100644 --- a/shell/common/engine.h +++ b/shell/common/engine.h @@ -12,6 +12,7 @@ #include "flutter/common/task_runners.h" #include "flutter/fml/macros.h" #include "flutter/fml/memory/weak_ptr.h" +#include "flutter/lib/ui/hint_freed_delegate.h" #include "flutter/lib/ui/painting/image_decoder.h" #include "flutter/lib/ui/semantics/custom_accessibility_action.h" #include "flutter/lib/ui/semantics/semantics_node.h" @@ -68,7 +69,9 @@ namespace flutter { /// name and it does happen to be one of the older classes in the /// repository. /// -class Engine final : public RuntimeDelegate, PointerDataDispatcher::Delegate { +class Engine final : public RuntimeDelegate, + public HintFreedDelegate, + PointerDataDispatcher::Delegate { public: //---------------------------------------------------------------------------- /// @brief Indicates the result of the call to `Engine::Run`. @@ -465,6 +468,9 @@ class Engine final : public RuntimeDelegate, PointerDataDispatcher::Delegate { /// void BeginFrame(fml::TimePoint frame_time); + // |HintFreedDelegate| + void HintFreed(size_t size) override; + //---------------------------------------------------------------------------- /// @brief Notifies the engine that the UI task runner is not expected to /// undertake a new frame workload till a specified timepoint. The @@ -797,6 +803,7 @@ class Engine final : public RuntimeDelegate, PointerDataDispatcher::Delegate { FontCollection font_collection_; ImageDecoder image_decoder_; TaskRunners task_runners_; + size_t hint_freed_bytes_since_last_idle_ = 0; fml::WeakPtrFactory weak_factory_; // |RuntimeDelegate| diff --git a/testing/dart_isolate_runner.cc b/testing/dart_isolate_runner.cc index 8fe1d1f5ee09b..1515f2e5afb15 100644 --- a/testing/dart_isolate_runner.cc +++ b/testing/dart_isolate_runner.cc @@ -73,6 +73,7 @@ void RunDartCodeInIsolate(DartVMRef& vm_ref, std::move(task_runners), // task runners nullptr, // window {}, // snapshot delegate + {}, // hint freed delegate io_manager, // io manager {}, // unref queue {}, // image decoder From 7b35fb351a37ac601ff4cf310ecbd5e76cece250 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Wed, 26 Aug 2020 11:29:18 -0700 Subject: [PATCH 02/12] update tests, license file --- ci/licenses_golden/licenses_flutter | 1 + testing/dart/canvas_test.dart | 60 ----------------------------- 2 files changed, 1 insertion(+), 60 deletions(-) diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index 99124c9effdbb..d83137983e8d0 100755 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -308,6 +308,7 @@ 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/io_manager.h FILE: ../../../flutter/lib/ui/isolate_name_server.dart diff --git a/testing/dart/canvas_test.dart b/testing/dart/canvas_test.dart index 8640cc86bf63d..a43be44622fd7 100644 --- a/testing/dart/canvas_test.dart +++ b/testing/dart/canvas_test.dart @@ -296,64 +296,4 @@ void main() { expectArgumentError(() => canvas.drawRawAtlas(image, Float32List(0), Float32List(4), null, null, rect, paint)); expectArgumentError(() => canvas.drawRawAtlas(image, Float32List(4), Float32List(4), Int32List(2), BlendMode.src, rect, paint)); }); - - test('Vertex buffer size reflected in picture size for drawVertices', () async { - final PictureRecorder recorder = PictureRecorder(); - final Canvas canvas = Canvas(recorder); - - const int uint16max = 65535; - - final Int32List colors = Int32List(uint16max); - final Float32List coords = Float32List(uint16max * 2); - final Uint16List indices = Uint16List(uint16max); - final Float32List positions = Float32List(uint16max * 2); - colors[0] = const Color(0xFFFF0000).value; - colors[1] = const Color(0xFF00FF00).value; - colors[2] = const Color(0xFF0000FF).value; - colors[3] = const Color(0xFF00FFFF).value; - indices[1] = indices[3] = 1; - indices[2] = indices[5] = 3; - indices[4] = 2; - positions[2] = positions[4] = positions[5] = positions[7] = 250.0; - - final Vertices vertices = Vertices.raw( - VertexMode.triangles, - positions, - textureCoordinates: coords, - colors: colors, - indices: indices, - ); - canvas.drawVertices(vertices, BlendMode.src, Paint()); - final Picture picture = recorder.endRecording(); - - - const int minimumExpected = uint16max * 4; - expect(picture.approximateBytesUsed, greaterThan(minimumExpected)); - - final PictureRecorder recorder2 = PictureRecorder(); - final Canvas canvas2 = Canvas(recorder2); - canvas2.drawPicture(picture); - final Picture picture2 = recorder2.endRecording(); - - expect(picture2.approximateBytesUsed, greaterThan(minimumExpected)); - }); - - test('Path reflected in picture size for drawPath, clipPath, and drawShadow', () async { - final PictureRecorder recorder = PictureRecorder(); - final Canvas canvas = Canvas(recorder); - final Path path = Path(); - for (int i = 0; i < 10000; i++) { - path.lineTo(5, 9); - path.lineTo(i.toDouble(), i.toDouble()); - } - path.close(); - canvas.drawPath(path, Paint()); - canvas.drawShadow(path, const Color(0xFF000000), 5.0, false); - canvas.clipPath(path); - final Picture picture = recorder.endRecording(); - - // Slightly fuzzy here to allow for platform specific differences - // Measurement on macOS: 541078 - expect(picture.approximateBytesUsed, greaterThan(530000)); - }); } From eee1b3308914e16183fad4d2594ea8b72776fd2b Mon Sep 17 00:00:00 2001 From: Dan Field Date: Wed, 26 Aug 2020 12:00:31 -0700 Subject: [PATCH 03/12] test that is hopefully not flaky! --- ci/licenses_golden/licenses_flutter | 1 + lib/ui/BUILD.gn | 1 + lib/ui/fixtures/ui_test.dart | 59 +++++++++++++ lib/ui/painting/image_dispose_unittests.cc | 97 ++++++++++++++++++++++ shell/common/shell_test.cc | 34 ++++++++ shell/common/shell_test.h | 2 + 6 files changed, 194 insertions(+) create mode 100644 lib/ui/painting/image_dispose_unittests.cc diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index d83137983e8d0..d087801e40c87 100755 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -338,6 +338,7 @@ FILE: ../../../flutter/lib/ui/painting/image_decoder.h FILE: ../../../flutter/lib/ui/painting/image_decoder_unittests.cc FILE: ../../../flutter/lib/ui/painting/image_descriptor.cc FILE: ../../../flutter/lib/ui/painting/image_descriptor.h +FILE: ../../../flutter/lib/ui/painting/image_dispose_unittests.cc FILE: ../../../flutter/lib/ui/painting/image_encoding.cc FILE: ../../../flutter/lib/ui/painting/image_encoding.h FILE: ../../../flutter/lib/ui/painting/image_encoding_unittests.cc diff --git a/lib/ui/BUILD.gn b/lib/ui/BUILD.gn index decca25d81f90..6c72102c2d9b8 100644 --- a/lib/ui/BUILD.gn +++ b/lib/ui/BUILD.gn @@ -181,6 +181,7 @@ if (enable_unittests) { public_configs = [ "//flutter:export_dynamic_symbols" ] sources = [ + "painting/image_dispose_unittests.cc", "painting/image_encoding_unittests.cc", "painting/vertices_unittests.cc", "window/platform_configuration_unittests.cc", diff --git a/lib/ui/fixtures/ui_test.dart b/lib/ui/fixtures/ui_test.dart index e2fcdaa7c3e8b..3017d29e2ca7d 100644 --- a/lib/ui/fixtures/ui_test.dart +++ b/lib/ui/fixtures/ui_test.dart @@ -3,6 +3,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +import 'dart:async'; import 'dart:typed_data'; import 'dart:ui'; @@ -73,3 +74,61 @@ Future encodeImageProducesExternalUint8List() async { void _encodeImage(Image i, int format, void Function(Uint8List result)) native 'EncodeImage'; void _validateExternal(Uint8List result) native 'ValidateExternal'; + +@pragma('vm:entry-point') +Future pumpImage() async { + const int width = 8000; + const int height = 8000; + final Completer completer = Completer(); + decodeImageFromPixels( + Uint8List.fromList(List.filled(width * height * 4, 0xFF)), + width, + height, + PixelFormat.rgba8888, + (Image image) => completer.complete(image), + ); + final Image image = await completer.future; + + final FrameCallback renderBlank = (Duration duration) { + image.dispose(); + + final PictureRecorder recorder = PictureRecorder(); + final Canvas canvas = Canvas(recorder); + canvas.drawRect(Rect.largest, Paint()); + final Picture picture = recorder.endRecording(); + + final SceneBuilder builder = SceneBuilder(); + 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) { + final PictureRecorder recorder = PictureRecorder(); + final Canvas canvas = Canvas(recorder); + canvas.drawImage(image, Offset.zero, Paint()); + final Picture picture = recorder.endRecording(); + + final SceneBuilder builder = SceneBuilder(); + 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 _onBeginFrameDone() native 'OnBeginFrameDone'; diff --git a/lib/ui/painting/image_dispose_unittests.cc b/lib/ui/painting/image_dispose_unittests.cc new file mode 100644 index 0000000000000..aa31fd74b3ca7 --- /dev/null +++ b/lib/ui/painting/image_dispose_unittests.cc @@ -0,0 +1,97 @@ + +// 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. + +#define FML_USED_ON_EMBEDDER + +#include "flutter/common/task_runners.h" +#include "flutter/fml/synchronization/waitable_event.h" +#include "flutter/lib/ui/painting/image.h" +#include "flutter/lib/ui/painting/picture.h" +#include "flutter/runtime/dart_vm.h" +#include "flutter/shell/common/shell_test.h" +#include "flutter/shell/common/thread_host.h" +#include "flutter/testing/testing.h" + +namespace flutter { +namespace testing { + +class ImageDisposeTest : public ShellTest { + public: + template + T* GetNativePeer(Dart_NativeArguments args, int index) { + auto handle = Dart_GetNativeArgument(args, index); + intptr_t peer = 0; + EXPECT_FALSE(Dart_IsError(Dart_GetNativeInstanceField( + handle, tonic::DartWrappable::kPeerIndex, &peer))); + return reinterpret_cast(peer); + } + + // Used to wait on Dart callbacks or Shell task runner flushing + fml::AutoResetWaitableEvent message_latch_; + + sk_sp current_picture_; + sk_sp current_image_; +}; + +TEST_F(ImageDisposeTest, ImageReleasedAfterFrame) { + auto native_capture_image_and_picture = [&](Dart_NativeArguments args) { + CanvasImage* image = GetNativePeer(args, 0); + Picture* picture = GetNativePeer(args, 1); + ASSERT_FALSE(image->image()->unique()); + ASSERT_FALSE(picture->picture()->unique()); + current_image_ = image->image(); + current_picture_ = picture->picture(); + }; + + auto native_on_begin_frame_done = [&](Dart_NativeArguments args) { + message_latch_.Signal(); + }; + + Settings settings = CreateSettingsForFixture(); + auto task_runner = CreateNewThread(); + TaskRunners task_runners("test", // label + GetCurrentTaskRunner(), // platform + task_runner, // raster + task_runner, // ui + task_runner // io + ); + + AddNativeCallback("CaptureImageAndPicture", + CREATE_NATIVE_ENTRY(native_capture_image_and_picture)); + AddNativeCallback("OnBeginFrameDone", + CREATE_NATIVE_ENTRY(native_on_begin_frame_done)); + + std::unique_ptr shell = CreateShell(std::move(settings), task_runners); + + ASSERT_TRUE(shell->IsSetup()); + + SetViewportMetrics(shell.get(), 800, 600); + + shell->GetPlatformView()->NotifyCreated(); + + auto configuration = RunConfiguration::InferFromSettings(settings); + configuration.SetEntrypoint("pumpImage"); + + shell->RunEngine(std::move(configuration), [&](auto result) { + ASSERT_EQ(result, Engine::RunStatus::Success); + }); + + message_latch_.Wait(); + + ASSERT_TRUE(current_picture_); + ASSERT_TRUE(current_image_); + + EXPECT_TRUE(current_picture_->unique()); + current_picture_.reset(); + + EXPECT_TRUE(current_image_->unique()); + current_image_.reset(); + + shell->GetPlatformView()->NotifyDestroyed(); + DestroyShell(std::move(shell), std::move(task_runners)); +} + +} // namespace testing +} // namespace flutter diff --git a/shell/common/shell_test.cc b/shell/common/shell_test.cc index 8144d3accaacd..e80652e33c059 100644 --- a/shell/common/shell_test.cc +++ b/shell/common/shell_test.cc @@ -106,6 +106,40 @@ void ShellTest::VSyncFlush(Shell* shell, bool& will_draw_new_frame) { latch.Wait(); } +void ShellTest::SetViewportMetrics(Shell* shell, double width, double height) { + flutter::ViewportMetrics viewport_metrics = { + 1, // device pixel ratio + width, // physical width + height, // physical height + 0, // padding top + 0, // padding right + 0, // padding bottom + 0, // padding left + 0, // view inset top + 0, // view inset right + 0, // view inset bottom + 0, // view inset left + 0, // gesture inset top + 0, // gesture inset right + 0, // gesture inset bottom + 0 // gesture inset left + }; + // Set viewport to nonempty, and call Animator::BeginFrame to make the layer + // tree pipeline nonempty. Without either of this, the layer tree below + // won't be rasterized. + fml::AutoResetWaitableEvent latch; + shell->GetTaskRunners().GetUITaskRunner()->PostTask( + [&latch, engine = shell->weak_engine_, viewport_metrics]() { + engine->SetViewportMetrics(std::move(viewport_metrics)); + const auto frame_begin_time = fml::TimePoint::Now(); + const auto frame_end_time = + frame_begin_time + fml::TimeDelta::FromSecondsF(1.0 / 60.0); + engine->animator_->BeginFrame(frame_begin_time, frame_end_time); + latch.Signal(); + }); + latch.Wait(); +} + void ShellTest::PumpOneFrame(Shell* shell, double width, double height, diff --git a/shell/common/shell_test.h b/shell/common/shell_test.h index c61aba37c7647..9d7d2ddab01be 100644 --- a/shell/common/shell_test.h +++ b/shell/common/shell_test.h @@ -63,6 +63,8 @@ class ShellTest : public FixtureTest { /// in PumpOneFrame. using LayerTreeBuilder = std::function root)>; + + static void SetViewportMetrics(Shell* shell, double width, double height); static void PumpOneFrame(Shell* shell, double width = 1, double height = 1, From 3666025be56a71d69631bb0d629b779a4a6d0098 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Wed, 26 Aug 2020 12:41:05 -0700 Subject: [PATCH 04/12] Update image_dispose_unittests.cc --- lib/ui/painting/image_dispose_unittests.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/ui/painting/image_dispose_unittests.cc b/lib/ui/painting/image_dispose_unittests.cc index aa31fd74b3ca7..eb2c95df12fd1 100644 --- a/lib/ui/painting/image_dispose_unittests.cc +++ b/lib/ui/painting/image_dispose_unittests.cc @@ -1,4 +1,3 @@ - // 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. From d9a6a8d42cd6ab2304ec83b7e8388070627e2df6 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Wed, 26 Aug 2020 16:21:13 -0700 Subject: [PATCH 05/12] Deflake? --- lib/ui/painting/image_dispose_unittests.cc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/ui/painting/image_dispose_unittests.cc b/lib/ui/painting/image_dispose_unittests.cc index aa31fd74b3ca7..732f06d92ab1a 100644 --- a/lib/ui/painting/image_dispose_unittests.cc +++ b/lib/ui/painting/image_dispose_unittests.cc @@ -83,6 +83,12 @@ TEST_F(ImageDisposeTest, ImageReleasedAfterFrame) { ASSERT_TRUE(current_picture_); ASSERT_TRUE(current_image_); + message_latch_.Reset(); + + // flush the UI Task runner + task_runner->PostTask([&]() { message_latch_.Signal(); }); + message_latch_.Wait(); + EXPECT_TRUE(current_picture_->unique()); current_picture_.reset(); From 8b19c6cd5003315fe5ef5ca8281b4224df601fb0 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Thu, 27 Aug 2020 12:24:07 -0700 Subject: [PATCH 06/12] deflaked --- lib/ui/fixtures/ui_test.dart | 4 ++-- lib/ui/painting/image_dispose_unittests.cc | 25 +++++++++++++++++++--- shell/common/shell.cc | 5 +++++ shell/common/shell.h | 7 ++++++ shell/common/shell_test.cc | 3 ++- 5 files changed, 38 insertions(+), 6 deletions(-) diff --git a/lib/ui/fixtures/ui_test.dart b/lib/ui/fixtures/ui_test.dart index 3017d29e2ca7d..39ad57d73d603 100644 --- a/lib/ui/fixtures/ui_test.dart +++ b/lib/ui/fixtures/ui_test.dart @@ -77,8 +77,8 @@ void _validateExternal(Uint8List result) native 'ValidateExternal'; @pragma('vm:entry-point') Future pumpImage() async { - const int width = 8000; - const int height = 8000; + const int width = 4000; + const int height = 4000; final Completer completer = Completer(); decodeImageFromPixels( Uint8List.fromList(List.filled(width * height * 4, 0xFF)), diff --git a/lib/ui/painting/image_dispose_unittests.cc b/lib/ui/painting/image_dispose_unittests.cc index bd258236c3a9e..137aa070f0dca 100644 --- a/lib/ui/painting/image_dispose_unittests.cc +++ b/lib/ui/painting/image_dispose_unittests.cc @@ -30,6 +30,12 @@ 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(peer); + latch->Signal(); + } + sk_sp current_picture_; sk_sp current_image_; }; @@ -42,6 +48,10 @@ TEST_F(ImageDisposeTest, ImageReleasedAfterFrame) { ASSERT_FALSE(picture->picture()->unique()); current_image_ = image->image(); current_picture_ = picture->picture(); + + Dart_NewFinalizableHandle( + Dart_HandleFromWeakPersistent(picture->dart_wrapper()), + &picture_finalizer_latch_, 0, &picture_finalizer); }; auto native_on_begin_frame_done = [&](Dart_NativeArguments args) { @@ -82,10 +92,19 @@ TEST_F(ImageDisposeTest, ImageReleasedAfterFrame) { ASSERT_TRUE(current_picture_); ASSERT_TRUE(current_image_); - message_latch_.Reset(); + // Make sure we hit at least one VSync flush, in case we're on a slower + // machine that effectively missed its VSync drawing this image. + bool will_draw_new_frame; + VSyncFlush(shell.get(), will_draw_new_frame); - // flush the UI Task runner - task_runner->PostTask([&]() { message_latch_.Signal(); }); + picture_finalizer_latch_.Wait(); + + // Force a drain the SkiaUnrefQueue. + message_latch_.Reset(); + task_runner->PostTask([&, io_manager = shell->GetIOManager()]() { + io_manager->GetSkiaUnrefQueue()->Drain(); + message_latch_.Signal(); + }); message_latch_.Wait(); EXPECT_TRUE(current_picture_->unique()); diff --git a/shell/common/shell.cc b/shell/common/shell.cc index 97c4cde047d27..c2efc96bfb60b 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -609,6 +609,11 @@ fml::WeakPtr Shell::GetPlatformView() { return weak_platform_view_; } +fml::WeakPtr Shell::GetIOManager() { + FML_DCHECK(is_setup_); + return io_manager_->GetWeakPtr(); +} + DartVM* Shell::GetDartVM() { return &vm_; } diff --git a/shell/common/shell.h b/shell/common/shell.h index dd09fad2ae7a6..f7070f72cda8e 100644 --- a/shell/common/shell.h +++ b/shell/common/shell.h @@ -274,6 +274,13 @@ class Shell final : public PlatformView::Delegate, /// fml::WeakPtr GetPlatformView(); + //---------------------------------------------------------------------------- + /// @brief The IO Manager may only be accessed on the IO task runner. + /// + /// @return A weak pointer to the IO manager. + /// + fml::WeakPtr GetIOManager(); + // Embedders should call this under low memory conditions to free up // internal caches used. // diff --git a/shell/common/shell_test.cc b/shell/common/shell_test.cc index e80652e33c059..fbd1f7afe9dfb 100644 --- a/shell/common/shell_test.cc +++ b/shell/common/shell_test.cc @@ -85,7 +85,8 @@ void ShellTest::RestartEngine(Shell* shell, RunConfiguration configuration) { void ShellTest::VSyncFlush(Shell* shell, bool& will_draw_new_frame) { fml::AutoResetWaitableEvent latch; - shell->GetTaskRunners().GetPlatformTaskRunner()->PostTask( + fml::TaskRunner::RunNowOrPostTask( + shell->GetTaskRunners().GetPlatformTaskRunner(), [shell, &will_draw_new_frame, &latch] { // The following UI task ensures that all previous UI tasks are flushed. fml::AutoResetWaitableEvent ui_latch; From c154c464fc231bbd9ed013842877f111c29cd128 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Tue, 1 Sep 2020 14:48:20 -0700 Subject: [PATCH 07/12] Fix windows? --- lib/ui/painting/image_dispose_unittests.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/ui/painting/image_dispose_unittests.cc b/lib/ui/painting/image_dispose_unittests.cc index 137aa070f0dca..641ef5e657dcb 100644 --- a/lib/ui/painting/image_dispose_unittests.cc +++ b/lib/ui/painting/image_dispose_unittests.cc @@ -49,9 +49,8 @@ TEST_F(ImageDisposeTest, ImageReleasedAfterFrame) { current_image_ = image->image(); current_picture_ = picture->picture(); - Dart_NewFinalizableHandle( - Dart_HandleFromWeakPersistent(picture->dart_wrapper()), - &picture_finalizer_latch_, 0, &picture_finalizer); + Dart_NewFinalizableHandle(Dart_GetNativeArgument(args, 1), + &picture_finalizer_latch_, 0, &picture_finalizer); }; auto native_on_begin_frame_done = [&](Dart_NativeArguments args) { From 68b31513e9c9577ff79e3d2aa04765d092b48672 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Tue, 1 Sep 2020 16:41:01 -0700 Subject: [PATCH 08/12] Deflake by adding .. a timeout :( --- lib/ui/painting/image_dispose_unittests.cc | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/ui/painting/image_dispose_unittests.cc b/lib/ui/painting/image_dispose_unittests.cc index 641ef5e657dcb..72142cc4f4d15 100644 --- a/lib/ui/painting/image_dispose_unittests.cc +++ b/lib/ui/painting/image_dispose_unittests.cc @@ -91,11 +91,13 @@ TEST_F(ImageDisposeTest, ImageReleasedAfterFrame) { ASSERT_TRUE(current_picture_); ASSERT_TRUE(current_image_); - // Make sure we hit at least one VSync flush, in case we're on a slower - // machine that effectively missed its VSync drawing this image. - bool will_draw_new_frame; - VSyncFlush(shell.get(), will_draw_new_frame); - + // Make sure we wait longer than the animator's notify idle. + // which is 51ms in animator.cc. + message_latch_.Reset(); + task_runner->PostDelayedTask([&]() { + message_latch_.Signal(); + }, fml::TimeDelta::FromMilliseconds(60)); + message_latch_.Wait(); picture_finalizer_latch_.Wait(); // Force a drain the SkiaUnrefQueue. From b82af7b075dbbcb234eeae87280088b4a49997b0 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Tue, 1 Sep 2020 16:41:17 -0700 Subject: [PATCH 09/12] Deflake by adding .. a timeout :( --- lib/ui/painting/image_dispose_unittests.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/ui/painting/image_dispose_unittests.cc b/lib/ui/painting/image_dispose_unittests.cc index 72142cc4f4d15..3b706beee8117 100644 --- a/lib/ui/painting/image_dispose_unittests.cc +++ b/lib/ui/painting/image_dispose_unittests.cc @@ -94,9 +94,8 @@ TEST_F(ImageDisposeTest, ImageReleasedAfterFrame) { // Make sure we wait longer than the animator's notify idle. // which is 51ms in animator.cc. message_latch_.Reset(); - task_runner->PostDelayedTask([&]() { - message_latch_.Signal(); - }, fml::TimeDelta::FromMilliseconds(60)); + task_runner->PostDelayedTask([&]() { message_latch_.Signal(); }, + fml::TimeDelta::FromMilliseconds(60)); message_latch_.Wait(); picture_finalizer_latch_.Wait(); From d09f46b84344c88c82dfaf4dc4d73d2f24a4147d Mon Sep 17 00:00:00 2001 From: Dan Field Date: Tue, 1 Sep 2020 17:27:25 -0700 Subject: [PATCH 10/12] make it safer --- lib/ui/fixtures/ui_test.dart | 4 ++-- lib/ui/painting/image_dispose_unittests.cc | 12 +++++------ shell/common/shell_test.cc | 24 +++++++++++++++++----- shell/common/shell_test.h | 2 ++ testing/run_tests.py | 3 ++- 5 files changed, 31 insertions(+), 14 deletions(-) diff --git a/lib/ui/fixtures/ui_test.dart b/lib/ui/fixtures/ui_test.dart index 39ad57d73d603..c01986f25b6cc 100644 --- a/lib/ui/fixtures/ui_test.dart +++ b/lib/ui/fixtures/ui_test.dart @@ -77,8 +77,8 @@ void _validateExternal(Uint8List result) native 'ValidateExternal'; @pragma('vm:entry-point') Future pumpImage() async { - const int width = 4000; - const int height = 4000; + const int width = 1000; + const int height = 1000; final Completer completer = Completer(); decodeImageFromPixels( Uint8List.fromList(List.filled(width * height * 4, 0xFF)), diff --git a/lib/ui/painting/image_dispose_unittests.cc b/lib/ui/painting/image_dispose_unittests.cc index 3b706beee8117..3763d70539f95 100644 --- a/lib/ui/painting/image_dispose_unittests.cc +++ b/lib/ui/painting/image_dispose_unittests.cc @@ -91,12 +91,12 @@ TEST_F(ImageDisposeTest, ImageReleasedAfterFrame) { ASSERT_TRUE(current_picture_); ASSERT_TRUE(current_image_); - // Make sure we wait longer than the animator's notify idle. - // which is 51ms in animator.cc. - message_latch_.Reset(); - task_runner->PostDelayedTask([&]() { message_latch_.Signal(); }, - fml::TimeDelta::FromMilliseconds(60)); - message_latch_.Wait(); + // 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. diff --git a/shell/common/shell_test.cc b/shell/common/shell_test.cc index fbd1f7afe9dfb..b43aaa0c06ca1 100644 --- a/shell/common/shell_test.cc +++ b/shell/common/shell_test.cc @@ -131,11 +131,25 @@ void ShellTest::SetViewportMetrics(Shell* shell, double width, double height) { fml::AutoResetWaitableEvent latch; shell->GetTaskRunners().GetUITaskRunner()->PostTask( [&latch, engine = shell->weak_engine_, viewport_metrics]() { - engine->SetViewportMetrics(std::move(viewport_metrics)); - const auto frame_begin_time = fml::TimePoint::Now(); - const auto frame_end_time = - frame_begin_time + fml::TimeDelta::FromSecondsF(1.0 / 60.0); - engine->animator_->BeginFrame(frame_begin_time, frame_end_time); + if (engine) { + engine->SetViewportMetrics(std::move(viewport_metrics)); + const auto frame_begin_time = fml::TimePoint::Now(); + const auto frame_end_time = + frame_begin_time + fml::TimeDelta::FromSecondsF(1.0 / 60.0); + engine->animator_->BeginFrame(frame_begin_time, frame_end_time); + } + latch.Signal(); + }); + latch.Wait(); +} + +void ShellTest::NotifyIdle(Shell* shell, int64_t deadline) { + fml::AutoResetWaitableEvent latch; + shell->GetTaskRunners().GetUITaskRunner()->PostTask( + [&latch, engine = shell->weak_engine_, deadline]() { + if (engine) { + engine->NotifyIdle(deadline); + } latch.Signal(); }); latch.Wait(); diff --git a/shell/common/shell_test.h b/shell/common/shell_test.h index 9d7d2ddab01be..5c8fd9d9b574a 100644 --- a/shell/common/shell_test.h +++ b/shell/common/shell_test.h @@ -65,6 +65,8 @@ class ShellTest : public FixtureTest { std::function root)>; static void SetViewportMetrics(Shell* shell, double width, double height); + static void NotifyIdle(Shell* shell, int64_t deadline); + static void PumpOneFrame(Shell* shell, double width = 1, double height = 1, diff --git a/testing/run_tests.py b/testing/run_tests.py index 3330cb985db15..a11f91c47b1e8 100755 --- a/testing/run_tests.py +++ b/testing/run_tests.py @@ -139,7 +139,8 @@ def RunCCTests(build_dir, filter): RunEngineExecutable(build_dir, 'jni_unittests', filter, shuffle_flags) RunEngineExecutable(build_dir, 'platform_view_android_delegate_unittests', filter, shuffle_flags) - RunEngineExecutable(build_dir, 'ui_unittests', filter, shuffle_flags) + # The image release unit test can take a while on slow machines. + RunEngineExecutable(build_dir, 'ui_unittests', filter, shuffle_flags + ['--timeout=90']) RunEngineExecutable(build_dir, 'testing_unittests', filter, shuffle_flags) From 35e376ba512efb68d2df517fa2323c14b68638ba Mon Sep 17 00:00:00 2001 From: Dan Field Date: Tue, 1 Sep 2020 21:07:04 -0700 Subject: [PATCH 11/12] .. --- lib/ui/fixtures/ui_test.dart | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/ui/fixtures/ui_test.dart b/lib/ui/fixtures/ui_test.dart index c01986f25b6cc..87effaad4f5c7 100644 --- a/lib/ui/fixtures/ui_test.dart +++ b/lib/ui/fixtures/ui_test.dart @@ -77,8 +77,8 @@ void _validateExternal(Uint8List result) native 'ValidateExternal'; @pragma('vm:entry-point') Future pumpImage() async { - const int width = 1000; - const int height = 1000; + const int width = 6000; + const int height = 6000; final Completer completer = Completer(); decodeImageFromPixels( Uint8List.fromList(List.filled(width * height * 4, 0xFF)), From fcf221796be422cc125f85821cf97157fac37251 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Wed, 2 Sep 2020 10:34:17 -0700 Subject: [PATCH 12/12] split changees to new pr --- lib/ui/painting/canvas.cc | 6 +++ lib/ui/painting/canvas.h | 3 ++ lib/ui/painting/picture.cc | 18 +++++---- lib/ui/painting/picture.h | 9 ++++- lib/ui/painting/picture_recorder.cc | 8 ++-- testing/dart/canvas_test.dart | 60 +++++++++++++++++++++++++++++ 6 files changed, 92 insertions(+), 12 deletions(-) diff --git a/lib/ui/painting/canvas.cc b/lib/ui/painting/canvas.cc index 913730da65d4d..c68f0d6001ad6 100644 --- a/lib/ui/painting/canvas.cc +++ b/lib/ui/painting/canvas.cc @@ -198,6 +198,7 @@ void Canvas::clipPath(const CanvasPath* path, bool doAntiAlias) { ToDart("Canvas.clipPath called with non-genuine Path.")); return; } + external_allocation_size_ += path->path().approximateBytesUsed(); canvas_->clipPath(path->path(), doAntiAlias); } @@ -309,6 +310,7 @@ void Canvas::drawPath(const CanvasPath* path, ToDart("Canvas.drawPath called with non-genuine Path.")); return; } + external_allocation_size_ += path->path().approximateBytesUsed(); canvas_->drawPath(path->path(), *paint.paint()); } @@ -389,6 +391,7 @@ void Canvas::drawPicture(Picture* picture) { ToDart("Canvas.drawPicture called with non-genuine Picture.")); return; } + external_allocation_size_ += picture->GetAllocationSize(); canvas_->drawPicture(picture->picture().get()); } @@ -421,6 +424,7 @@ void Canvas::drawVertices(const Vertices* vertices, ToDart("Canvas.drawVertices called with non-genuine Vertices.")); return; } + external_allocation_size_ += vertices->GetAllocationSize(); canvas_->drawVertices(vertices->vertices(), blend_mode, *paint.paint()); } @@ -449,6 +453,7 @@ void Canvas::drawAtlas(const Paint& paint, static_assert(sizeof(SkRect) == sizeof(float) * 4, "SkRect doesn't use floats."); + external_allocation_size_ += atlas->GetAllocationSize(); canvas_->drawAtlas( skImage.get(), reinterpret_cast(transforms.data()), reinterpret_cast(rects.data()), @@ -472,6 +477,7 @@ void Canvas::drawShadow(const CanvasPath* path, ->window() ->viewport_metrics() .device_pixel_ratio; + external_allocation_size_ += path->path().approximateBytesUsed(); flutter::PhysicalShapeLayer::DrawShadow(canvas_, path->path(), color, elevation, transparentOccluder, dpr); } diff --git a/lib/ui/painting/canvas.h b/lib/ui/painting/canvas.h index 3c58bad4e7895..53492b9667875 100644 --- a/lib/ui/painting/canvas.h +++ b/lib/ui/painting/canvas.h @@ -169,6 +169,8 @@ class Canvas : public RefCountedDartWrappable { static void RegisterNatives(tonic::DartLibraryNatives* natives); + size_t external_allocation_size() const { return external_allocation_size_; } + private: explicit Canvas(SkCanvas* canvas); @@ -176,6 +178,7 @@ class Canvas : public RefCountedDartWrappable { // which does not transfer ownership. For this reason, we hold a raw // pointer and manually set to null in Clear. SkCanvas* canvas_; + size_t external_allocation_size_ = 0; }; } // namespace flutter diff --git a/lib/ui/painting/picture.cc b/lib/ui/painting/picture.cc index 5225d993ba4fb..1285a6b0921cb 100644 --- a/lib/ui/painting/picture.cc +++ b/lib/ui/painting/picture.cc @@ -28,17 +28,20 @@ IMPLEMENT_WRAPPERTYPEINFO(ui, Picture); DART_BIND_ALL(Picture, FOR_EACH_BINDING) -fml::RefPtr Picture::Create( - Dart_Handle dart_handle, - flutter::SkiaGPUObject picture) { - auto canvas_picture = fml::MakeRefCounted(std::move(picture)); +fml::RefPtr Picture::Create(Dart_Handle dart_handle, + flutter::SkiaGPUObject picture, + size_t external_allocation_size) { + auto canvas_picture = fml::MakeRefCounted(std::move(picture), + external_allocation_size); canvas_picture->AssociateWithDartWrapper(dart_handle); return canvas_picture; } -Picture::Picture(flutter::SkiaGPUObject picture) - : picture_(std::move(picture)) {} +Picture::Picture(flutter::SkiaGPUObject picture, + size_t external_allocation_size) + : picture_(std::move(picture)), + external_allocation_size_(external_allocation_size) {} Picture::~Picture() = default; @@ -59,7 +62,8 @@ void Picture::dispose() { size_t Picture::GetAllocationSize() const { if (auto picture = picture_.get()) { - return picture->approximateBytesUsed() + sizeof(Picture); + return picture->approximateBytesUsed() + sizeof(Picture) + + external_allocation_size_; } else { return sizeof(Picture); } diff --git a/lib/ui/painting/picture.h b/lib/ui/painting/picture.h index e0158d400ff5e..8f1879e1af7fa 100644 --- a/lib/ui/painting/picture.h +++ b/lib/ui/painting/picture.h @@ -25,7 +25,8 @@ class Picture : public RefCountedDartWrappable { public: ~Picture() override; static fml::RefPtr Create(Dart_Handle dart_handle, - flutter::SkiaGPUObject picture); + flutter::SkiaGPUObject picture, + size_t external_allocation_size); sk_sp picture() const { return picture_.get(); } @@ -44,10 +45,14 @@ class Picture : public RefCountedDartWrappable { uint32_t height, Dart_Handle raw_image_callback); + size_t external_allocation_size() const { return external_allocation_size_; } + private: - Picture(flutter::SkiaGPUObject picture); + Picture(flutter::SkiaGPUObject picture, + size_t external_allocation_size_); flutter::SkiaGPUObject picture_; + size_t external_allocation_size_; }; } // namespace flutter diff --git a/lib/ui/painting/picture_recorder.cc b/lib/ui/painting/picture_recorder.cc index 5084f30d7812f..5916faa9df3ae 100644 --- a/lib/ui/painting/picture_recorder.cc +++ b/lib/ui/painting/picture_recorder.cc @@ -47,9 +47,11 @@ fml::RefPtr PictureRecorder::endRecording(Dart_Handle dart_picture) { return nullptr; } - fml::RefPtr picture = Picture::Create( - dart_picture, UIDartState::CreateGPUObject( - picture_recorder_.finishRecordingAsPicture())); + fml::RefPtr picture = + Picture::Create(dart_picture, + UIDartState::CreateGPUObject( + picture_recorder_.finishRecordingAsPicture()), + canvas_->external_allocation_size()); canvas_->Invalidate(); canvas_ = nullptr; diff --git a/testing/dart/canvas_test.dart b/testing/dart/canvas_test.dart index dd8143a39cc4b..39a21a540f678 100644 --- a/testing/dart/canvas_test.dart +++ b/testing/dart/canvas_test.dart @@ -269,4 +269,64 @@ void main() { expectArgumentError(() => canvas.drawRawAtlas(image, Float32List(0), Float32List(4), null, null, rect, paint)); expectArgumentError(() => canvas.drawRawAtlas(image, Float32List(4), Float32List(4), Int32List(2), BlendMode.src, rect, paint)); }); + + test('Vertex buffer size reflected in picture size for drawVertices', () async { + final PictureRecorder recorder = PictureRecorder(); + final Canvas canvas = Canvas(recorder); + + const int uint16max = 65535; + + final Int32List colors = Int32List(uint16max); + final Float32List coords = Float32List(uint16max * 2); + final Uint16List indices = Uint16List(uint16max); + final Float32List positions = Float32List(uint16max * 2); + colors[0] = const Color(0xFFFF0000).value; + colors[1] = const Color(0xFF00FF00).value; + colors[2] = const Color(0xFF0000FF).value; + colors[3] = const Color(0xFF00FFFF).value; + indices[1] = indices[3] = 1; + indices[2] = indices[5] = 3; + indices[4] = 2; + positions[2] = positions[4] = positions[5] = positions[7] = 250.0; + + final Vertices vertices = Vertices.raw( + VertexMode.triangles, + positions, + textureCoordinates: coords, + colors: colors, + indices: indices, + ); + canvas.drawVertices(vertices, BlendMode.src, Paint()); + final Picture picture = recorder.endRecording(); + + + const int minimumExpected = uint16max * 4; + expect(picture.approximateBytesUsed, greaterThan(minimumExpected)); + + final PictureRecorder recorder2 = PictureRecorder(); + final Canvas canvas2 = Canvas(recorder2); + canvas2.drawPicture(picture); + final Picture picture2 = recorder2.endRecording(); + + expect(picture2.approximateBytesUsed, greaterThan(minimumExpected)); + }); + + test('Path reflected in picture size for drawPath, clipPath, and drawShadow', () async { + final PictureRecorder recorder = PictureRecorder(); + final Canvas canvas = Canvas(recorder); + final Path path = Path(); + for (int i = 0; i < 10000; i++) { + path.lineTo(5, 9); + path.lineTo(i.toDouble(), i.toDouble()); + } + path.close(); + canvas.drawPath(path, Paint()); + canvas.drawShadow(path, const Color(0xFF000000), 5.0, false); + canvas.clipPath(path); + final Picture picture = recorder.endRecording(); + + // Slightly fuzzy here to allow for platform specific differences + // Measurement on macOS: 541078 + expect(picture.approximateBytesUsed, greaterThan(530000)); + }); }