From 720dcb2905c956069bbf6275163e69819e60efea Mon Sep 17 00:00:00 2001 From: Dan Field Date: Mon, 21 Dec 2020 13:56:01 -0800 Subject: [PATCH 1/4] Reland path volatility tracker (#23063)" (#23220) This reverts commit fceef3aaa9d156e8ec3f4a079c142921882f70d8. --- ci/licenses_golden/licenses_flutter | 3 + fml/trace_event.h | 13 ++ lib/ui/BUILD.gn | 3 + lib/ui/fixtures/ui_test.dart | 13 +- lib/ui/painting/path.cc | 130 ++++++++++++------ lib/ui/painting/path.h | 15 +- lib/ui/painting/path_unittests.cc | 128 +++++++++++++++++ lib/ui/ui_benchmarks.cc | 51 ++++++- lib/ui/ui_dart_state.cc | 9 +- lib/ui/ui_dart_state.h | 7 +- lib/ui/volatile_path_tracker.cc | 74 ++++++++++ lib/ui/volatile_path_tracker.h | 73 ++++++++++ runtime/dart_isolate.cc | 64 +++++---- runtime/dart_isolate.h | 10 +- runtime/dart_isolate_unittests.cc | 12 +- runtime/dart_lifecycle_unittests.cc | 3 +- runtime/fixtures/runtime_test.dart | 2 + runtime/fixtures/split_lib_test.dart | 2 + runtime/runtime_controller.cc | 12 +- runtime/runtime_controller.h | 7 +- shell/common/engine.cc | 6 +- shell/common/engine.h | 4 +- shell/common/shell.cc | 36 +++-- shell/common/shell.h | 7 +- testing/dart/path_test.dart | 8 ++ testing/dart_isolate_runner.cc | 11 +- testing/dart_isolate_runner.h | 21 +-- .../tonic/tests/dart_state_unittest.cc | 3 +- 28 files changed, 606 insertions(+), 121 deletions(-) create mode 100644 lib/ui/painting/path_unittests.cc create mode 100644 lib/ui/volatile_path_tracker.cc create mode 100644 lib/ui/volatile_path_tracker.h diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index 750c9f164db59..177517471df03 100755 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -1131,6 +1131,7 @@ FILE: ../../../flutter/lib/ui/painting/path.cc FILE: ../../../flutter/lib/ui/painting/path.h FILE: ../../../flutter/lib/ui/painting/path_measure.cc FILE: ../../../flutter/lib/ui/painting/path_measure.h +FILE: ../../../flutter/lib/ui/painting/path_unittests.cc FILE: ../../../flutter/lib/ui/painting/picture.cc FILE: ../../../flutter/lib/ui/painting/picture.h FILE: ../../../flutter/lib/ui/painting/picture_recorder.cc @@ -1174,6 +1175,8 @@ FILE: ../../../flutter/lib/ui/ui.dart FILE: ../../../flutter/lib/ui/ui_benchmarks.cc FILE: ../../../flutter/lib/ui/ui_dart_state.cc FILE: ../../../flutter/lib/ui/ui_dart_state.h +FILE: ../../../flutter/lib/ui/volatile_path_tracker.cc +FILE: ../../../flutter/lib/ui/volatile_path_tracker.h FILE: ../../../flutter/lib/ui/window.dart FILE: ../../../flutter/lib/ui/window/platform_configuration.cc FILE: ../../../flutter/lib/ui/window/platform_configuration.h diff --git a/fml/trace_event.h b/fml/trace_event.h index ae80298cfd699..3928d68c7d24c 100644 --- a/fml/trace_event.h +++ b/fml/trace_event.h @@ -68,6 +68,19 @@ ::fml::tracing::TraceCounter((category_group), (name), (counter_id), (arg1), \ __VA_ARGS__); +// Avoid using the same `name` and `argX_name` for nested traces, which can +// lead to double free errors. E.g. the following code should be avoided: +// +// ```cpp +// { +// TRACE_EVENT1("flutter", "Foo::Bar", "count", "initial_count_value"); +// ... +// TRACE_EVENT_INSTANT1("flutter", "Foo::Bar", +// "count", "updated_count_value"); +// } +// ``` +// +// Instead, either use different `name` or `arg1` parameter names. #define FML_TRACE_EVENT(category_group, name, ...) \ ::fml::tracing::TraceEvent((category_group), (name), __VA_ARGS__); \ __FML__AUTO_TRACE_END(name) diff --git a/lib/ui/BUILD.gn b/lib/ui/BUILD.gn index 6ce9238b2738d..fa31f9fe5575e 100644 --- a/lib/ui/BUILD.gn +++ b/lib/ui/BUILD.gn @@ -91,6 +91,8 @@ source_set("ui") { "text/text_box.h", "ui_dart_state.cc", "ui_dart_state.h", + "volatile_path_tracker.cc", + "volatile_path_tracker.h", "window/platform_configuration.cc", "window/platform_configuration.h", "window/platform_message.cc", @@ -188,6 +190,7 @@ if (enable_unittests) { sources = [ "painting/image_dispose_unittests.cc", "painting/image_encoding_unittests.cc", + "painting/path_unittests.cc", "painting/vertices_unittests.cc", "window/platform_configuration_unittests.cc", "window/pointer_data_packet_converter_unittests.cc", diff --git a/lib/ui/fixtures/ui_test.dart b/lib/ui/fixtures/ui_test.dart index 87effaad4f5c7..9e274e94eb70e 100644 --- a/lib/ui/fixtures/ui_test.dart +++ b/lib/ui/fixtures/ui_test.dart @@ -35,9 +35,20 @@ void createVertices() { ); _validateVertices(vertices); } - void _validateVertices(Vertices vertices) native 'ValidateVertices'; +@pragma('vm:entry-point') +void createPath() { + final Path path = Path()..lineTo(10, 10); + _validatePath(path); + // Arbitrarily hold a reference to the path to make sure it does not get + // garbage collected. + Future.delayed(const Duration(days: 100)).then((_) { + path.lineTo(100, 100); + }); +} +void _validatePath(Path path) native 'ValidatePath'; + @pragma('vm:entry-point') void frameCallback(FrameInfo info) { print('called back'); diff --git a/lib/ui/painting/path.cc b/lib/ui/painting/path.cc index d0898a4ca283c..6cc2b0529d0b1 100644 --- a/lib/ui/painting/path.cc +++ b/lib/ui/painting/path.cc @@ -67,43 +67,69 @@ void CanvasPath::RegisterNatives(tonic::DartLibraryNatives* natives) { FOR_EACH_BINDING(DART_REGISTER_NATIVE)}); } -CanvasPath::CanvasPath() {} +CanvasPath::CanvasPath() + : path_tracker_(UIDartState::Current()->GetVolatilePathTracker()), + tracked_path_(std::make_shared()) { + FML_DCHECK(path_tracker_); + resetVolatility(); +} + +CanvasPath::~CanvasPath() = default; + +void CanvasPath::resetVolatility() { + if (!tracked_path_->tracking_volatility) { + mutable_path().setIsVolatile(true); + tracked_path_->frame_count = 0; + tracked_path_->tracking_volatility = true; + path_tracker_->Insert(tracked_path_); + } +} -CanvasPath::~CanvasPath() {} +void CanvasPath::ReleaseDartWrappableReference() const { + FML_DCHECK(path_tracker_); + path_tracker_->Erase(tracked_path_); +} int CanvasPath::getFillType() { - return static_cast(path_.getFillType()); + return static_cast(path().getFillType()); } void CanvasPath::setFillType(int fill_type) { - path_.setFillType(static_cast(fill_type)); + mutable_path().setFillType(static_cast(fill_type)); + resetVolatility(); } void CanvasPath::moveTo(float x, float y) { - path_.moveTo(x, y); + mutable_path().moveTo(x, y); + resetVolatility(); } void CanvasPath::relativeMoveTo(float x, float y) { - path_.rMoveTo(x, y); + mutable_path().rMoveTo(x, y); + resetVolatility(); } void CanvasPath::lineTo(float x, float y) { - path_.lineTo(x, y); + mutable_path().lineTo(x, y); + resetVolatility(); } void CanvasPath::relativeLineTo(float x, float y) { - path_.rLineTo(x, y); + mutable_path().rLineTo(x, y); + resetVolatility(); } void CanvasPath::quadraticBezierTo(float x1, float y1, float x2, float y2) { - path_.quadTo(x1, y1, x2, y2); + mutable_path().quadTo(x1, y1, x2, y2); + resetVolatility(); } void CanvasPath::relativeQuadraticBezierTo(float x1, float y1, float x2, float y2) { - path_.rQuadTo(x1, y1, x2, y2); + mutable_path().rQuadTo(x1, y1, x2, y2); + resetVolatility(); } void CanvasPath::cubicTo(float x1, @@ -112,7 +138,8 @@ void CanvasPath::cubicTo(float x1, float y2, float x3, float y3) { - path_.cubicTo(x1, y1, x2, y2, x3, y3); + mutable_path().cubicTo(x1, y1, x2, y2, x3, y3); + resetVolatility(); } void CanvasPath::relativeCubicTo(float x1, @@ -121,11 +148,13 @@ void CanvasPath::relativeCubicTo(float x1, float y2, float x3, float y3) { - path_.rCubicTo(x1, y1, x2, y2, x3, y3); + mutable_path().rCubicTo(x1, y1, x2, y2, x3, y3); + resetVolatility(); } void CanvasPath::conicTo(float x1, float y1, float x2, float y2, float w) { - path_.conicTo(x1, y1, x2, y2, w); + mutable_path().conicTo(x1, y1, x2, y2, w); + resetVolatility(); } void CanvasPath::relativeConicTo(float x1, @@ -133,7 +162,8 @@ void CanvasPath::relativeConicTo(float x1, float x2, float y2, float w) { - path_.rConicTo(x1, y1, x2, y2, w); + mutable_path().rConicTo(x1, y1, x2, y2, w); + resetVolatility(); } void CanvasPath::arcTo(float left, @@ -143,9 +173,10 @@ void CanvasPath::arcTo(float left, float startAngle, float sweepAngle, bool forceMoveTo) { - path_.arcTo(SkRect::MakeLTRB(left, top, right, bottom), - startAngle * 180.0 / M_PI, sweepAngle * 180.0 / M_PI, - forceMoveTo); + mutable_path().arcTo(SkRect::MakeLTRB(left, top, right, bottom), + startAngle * 180.0 / M_PI, sweepAngle * 180.0 / M_PI, + forceMoveTo); + resetVolatility(); } void CanvasPath::arcToPoint(float arcEndX, @@ -160,8 +191,9 @@ void CanvasPath::arcToPoint(float arcEndX, const auto direction = isClockwiseDirection ? SkPathDirection::kCW : SkPathDirection::kCCW; - path_.arcTo(radiusX, radiusY, xAxisRotation, arcSize, direction, arcEndX, - arcEndY); + mutable_path().arcTo(radiusX, radiusY, xAxisRotation, arcSize, direction, + arcEndX, arcEndY); + resetVolatility(); } void CanvasPath::relativeArcToPoint(float arcEndDeltaX, @@ -175,16 +207,19 @@ void CanvasPath::relativeArcToPoint(float arcEndDeltaX, : SkPath::ArcSize::kSmall_ArcSize; const auto direction = isClockwiseDirection ? SkPathDirection::kCW : SkPathDirection::kCCW; - path_.rArcTo(radiusX, radiusY, xAxisRotation, arcSize, direction, - arcEndDeltaX, arcEndDeltaY); + mutable_path().rArcTo(radiusX, radiusY, xAxisRotation, arcSize, direction, + arcEndDeltaX, arcEndDeltaY); + resetVolatility(); } void CanvasPath::addRect(float left, float top, float right, float bottom) { - path_.addRect(SkRect::MakeLTRB(left, top, right, bottom)); + mutable_path().addRect(SkRect::MakeLTRB(left, top, right, bottom)); + resetVolatility(); } void CanvasPath::addOval(float left, float top, float right, float bottom) { - path_.addOval(SkRect::MakeLTRB(left, top, right, bottom)); + mutable_path().addOval(SkRect::MakeLTRB(left, top, right, bottom)); + resetVolatility(); } void CanvasPath::addArc(float left, @@ -193,17 +228,20 @@ void CanvasPath::addArc(float left, float bottom, float startAngle, float sweepAngle) { - path_.addArc(SkRect::MakeLTRB(left, top, right, bottom), - startAngle * 180.0 / M_PI, sweepAngle * 180.0 / M_PI); + mutable_path().addArc(SkRect::MakeLTRB(left, top, right, bottom), + startAngle * 180.0 / M_PI, sweepAngle * 180.0 / M_PI); + resetVolatility(); } void CanvasPath::addPolygon(const tonic::Float32List& points, bool close) { - path_.addPoly(reinterpret_cast(points.data()), - points.num_elements() / 2, close); + mutable_path().addPoly(reinterpret_cast(points.data()), + points.num_elements() / 2, close); + resetVolatility(); } void CanvasPath::addRRect(const RRect& rrect) { - path_.addRRect(rrect.sk_rrect); + mutable_path().addRRect(rrect.sk_rrect); + resetVolatility(); } void CanvasPath::addPath(CanvasPath* path, double dx, double dy) { @@ -211,7 +249,8 @@ void CanvasPath::addPath(CanvasPath* path, double dx, double dy) { Dart_ThrowException(ToDart("Path.addPath called with non-genuine Path.")); return; } - path_.addPath(path->path(), dx, dy, SkPath::kAppend_AddPathMode); + mutable_path().addPath(path->path(), dx, dy, SkPath::kAppend_AddPathMode); + resetVolatility(); } void CanvasPath::addPathWithMatrix(CanvasPath* path, @@ -227,8 +266,9 @@ void CanvasPath::addPathWithMatrix(CanvasPath* path, SkMatrix matrix = ToSkMatrix(matrix4); matrix.setTranslateX(matrix.getTranslateX() + dx); matrix.setTranslateY(matrix.getTranslateY() + dy); - path_.addPath(path->path(), matrix, SkPath::kAppend_AddPathMode); + mutable_path().addPath(path->path(), matrix, SkPath::kAppend_AddPathMode); matrix4.Release(); + resetVolatility(); } void CanvasPath::extendWithPath(CanvasPath* path, double dx, double dy) { @@ -237,7 +277,8 @@ void CanvasPath::extendWithPath(CanvasPath* path, double dx, double dy) { ToDart("Path.extendWithPath called with non-genuine Path.")); return; } - path_.addPath(path->path(), dx, dy, SkPath::kExtend_AddPathMode); + mutable_path().addPath(path->path(), dx, dy, SkPath::kExtend_AddPathMode); + resetVolatility(); } void CanvasPath::extendWithPathAndMatrix(CanvasPath* path, @@ -253,37 +294,43 @@ void CanvasPath::extendWithPathAndMatrix(CanvasPath* path, SkMatrix matrix = ToSkMatrix(matrix4); matrix.setTranslateX(matrix.getTranslateX() + dx); matrix.setTranslateY(matrix.getTranslateY() + dy); - path_.addPath(path->path(), matrix, SkPath::kExtend_AddPathMode); + mutable_path().addPath(path->path(), matrix, SkPath::kExtend_AddPathMode); matrix4.Release(); + resetVolatility(); } void CanvasPath::close() { - path_.close(); + mutable_path().close(); + resetVolatility(); } void CanvasPath::reset() { - path_.reset(); + mutable_path().reset(); + resetVolatility(); } bool CanvasPath::contains(double x, double y) { - return path_.contains(x, y); + return path().contains(x, y); } void CanvasPath::shift(Dart_Handle path_handle, double dx, double dy) { fml::RefPtr path = CanvasPath::Create(path_handle); - path_.offset(dx, dy, &path->path_); + auto& other_mutable_path = path->mutable_path(); + mutable_path().offset(dx, dy, &other_mutable_path); + resetVolatility(); } void CanvasPath::transform(Dart_Handle path_handle, tonic::Float64List& matrix4) { fml::RefPtr path = CanvasPath::Create(path_handle); - path_.transform(ToSkMatrix(matrix4), &path->path_); + auto& other_mutable_path = path->mutable_path(); + mutable_path().transform(ToSkMatrix(matrix4), &other_mutable_path); matrix4.Release(); } tonic::Float32List CanvasPath::getBounds() { tonic::Float32List rect(Dart_NewTypedData(Dart_TypedData_kFloat32, 4)); - const SkRect& bounds = path_.getBounds(); + const SkRect& bounds = path().getBounds(); rect[0] = bounds.left(); rect[1] = bounds.top(); rect[2] = bounds.right(); @@ -293,21 +340,22 @@ tonic::Float32List CanvasPath::getBounds() { bool CanvasPath::op(CanvasPath* path1, CanvasPath* path2, int operation) { return Op(path1->path(), path2->path(), static_cast(operation), - &path_); + &tracked_path_->path); + resetVolatility(); } void CanvasPath::clone(Dart_Handle path_handle) { fml::RefPtr path = CanvasPath::Create(path_handle); // per Skia docs, this will create a fast copy // data is shared until the source path or dest path are mutated - path->path_ = path_; + path->mutable_path() = this->path(); } // This is doomed to be called too early, since Paths are mutable. // However, it can help for some of the clone/shift/transform type methods // where the resultant path will initially have a meaningful size. size_t CanvasPath::GetAllocationSize() const { - return sizeof(CanvasPath) + path_.approximateBytesUsed(); + return sizeof(CanvasPath) + path().approximateBytesUsed(); } } // namespace flutter diff --git a/lib/ui/painting/path.h b/lib/ui/painting/path.h index dbd6d7a0d1cc2..3c05cdd140837 100644 --- a/lib/ui/painting/path.h +++ b/lib/ui/painting/path.h @@ -7,6 +7,7 @@ #include "flutter/lib/ui/dart_wrapper.h" #include "flutter/lib/ui/painting/rrect.h" +#include "flutter/lib/ui/volatile_path_tracker.h" #include "third_party/skia/include/core/SkPath.h" #include "third_party/skia/include/pathops/SkPathOps.h" #include "third_party/tonic/typed_data/typed_list.h" @@ -36,7 +37,7 @@ class CanvasPath : public RefCountedDartWrappable { static fml::RefPtr CreateFrom(Dart_Handle path_handle, const SkPath& src) { fml::RefPtr path = CanvasPath::Create(path_handle); - path->path_ = src; + path->tracked_path_->path = src; return path; } @@ -108,16 +109,24 @@ class CanvasPath : public RefCountedDartWrappable { bool op(CanvasPath* path1, CanvasPath* path2, int operation); void clone(Dart_Handle path_handle); - const SkPath& path() const { return path_; } + const SkPath& path() const { return tracked_path_->path; } size_t GetAllocationSize() const override; static void RegisterNatives(tonic::DartLibraryNatives* natives); + virtual void ReleaseDartWrappableReference() const override; + private: CanvasPath(); - SkPath path_; + std::shared_ptr path_tracker_; + std::shared_ptr tracked_path_; + + // Must be called whenever the path is created or mutated. + void resetVolatility(); + + SkPath& mutable_path() { return tracked_path_->path; } }; } // namespace flutter diff --git a/lib/ui/painting/path_unittests.cc b/lib/ui/painting/path_unittests.cc new file mode 100644 index 0000000000000..ed5a52500e1a2 --- /dev/null +++ b/lib/ui/painting/path_unittests.cc @@ -0,0 +1,128 @@ +// 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. + +#include "flutter/lib/ui/painting/path.h" + +#include + +#include "flutter/common/task_runners.h" +#include "flutter/fml/synchronization/waitable_event.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 { + +TEST_F(ShellTest, PathVolatilityOldPathsBecomeNonVolatile) { + auto message_latch = std::make_shared(); + + auto native_validate_path = [message_latch](Dart_NativeArguments args) { + auto handle = Dart_GetNativeArgument(args, 0); + intptr_t peer = 0; + Dart_Handle result = Dart_GetNativeInstanceField( + handle, tonic::DartWrappable::kPeerIndex, &peer); + EXPECT_FALSE(Dart_IsError(result)); + CanvasPath* path = reinterpret_cast(peer); + EXPECT_TRUE(path); + EXPECT_TRUE(path->path().isVolatile()); + std::shared_ptr tracker = + UIDartState::Current()->GetVolatilePathTracker(); + EXPECT_TRUE(tracker); + + for (int i = 0; i < VolatilePathTracker::kFramesOfVolatility; i++) { + EXPECT_TRUE(path->path().isVolatile()); + tracker->OnFrame(); + } + EXPECT_FALSE(path->path().isVolatile()); + message_latch->Signal(); + }; + + Settings settings = CreateSettingsForFixture(); + TaskRunners task_runners("test", // label + GetCurrentTaskRunner(), // platform + CreateNewThread(), // raster + CreateNewThread(), // ui + CreateNewThread() // io + ); + + AddNativeCallback("ValidatePath", CREATE_NATIVE_ENTRY(native_validate_path)); + + std::unique_ptr shell = + CreateShell(std::move(settings), std::move(task_runners)); + + ASSERT_TRUE(shell->IsSetup()); + auto configuration = RunConfiguration::InferFromSettings(settings); + configuration.SetEntrypoint("createPath"); + + shell->RunEngine(std::move(configuration), [](auto result) { + ASSERT_EQ(result, Engine::RunStatus::Success); + }); + + message_latch->Wait(); + + DestroyShell(std::move(shell), std::move(task_runners)); +} + +TEST_F(ShellTest, PathVolatilityGCRemovesPathFromTracker) { + auto message_latch = std::make_shared(); + + auto native_validate_path = [message_latch](Dart_NativeArguments args) { + auto handle = Dart_GetNativeArgument(args, 0); + intptr_t peer = 0; + Dart_Handle result = Dart_GetNativeInstanceField( + handle, tonic::DartWrappable::kPeerIndex, &peer); + EXPECT_FALSE(Dart_IsError(result)); + CanvasPath* path = reinterpret_cast(peer); + EXPECT_TRUE(path); + EXPECT_TRUE(path->path().isVolatile()); + std::shared_ptr tracker = + UIDartState::Current()->GetVolatilePathTracker(); + EXPECT_TRUE(tracker); + + static_assert(VolatilePathTracker::kFramesOfVolatility > 1); + EXPECT_TRUE(path->path().isVolatile()); + tracker->OnFrame(); + EXPECT_TRUE(path->path().isVolatile()); + + // simulate GC + path->ReleaseDartWrappableReference(); + + tracker->OnFrame(); + // Because the path got GC'd, it was removed from the cache and we're the + // only one holding it. + EXPECT_TRUE(path->path().isVolatile()); + + message_latch->Signal(); + }; + + Settings settings = CreateSettingsForFixture(); + TaskRunners task_runners("test", // label + GetCurrentTaskRunner(), // platform + CreateNewThread(), // raster + CreateNewThread(), // ui + CreateNewThread() // io + ); + + AddNativeCallback("ValidatePath", CREATE_NATIVE_ENTRY(native_validate_path)); + + std::unique_ptr shell = + CreateShell(std::move(settings), std::move(task_runners)); + + ASSERT_TRUE(shell->IsSetup()); + auto configuration = RunConfiguration::InferFromSettings(settings); + configuration.SetEntrypoint("createPath"); + + shell->RunEngine(std::move(configuration), [](auto result) { + ASSERT_EQ(result, Engine::RunStatus::Success); + }); + + message_latch->Wait(); + + DestroyShell(std::move(shell), std::move(task_runners)); +} + +} // namespace testing +} // namespace flutter diff --git a/lib/ui/ui_benchmarks.cc b/lib/ui/ui_benchmarks.cc index 9c0a189f31531..16a1ae2a0c158 100644 --- a/lib/ui/ui_benchmarks.cc +++ b/lib/ui/ui_benchmarks.cc @@ -4,6 +4,7 @@ #include "flutter/benchmarking/benchmarking.h" #include "flutter/common/settings.h" +#include "flutter/lib/ui/volatile_path_tracker.h" #include "flutter/lib/ui/window/platform_message_response_dart.h" #include "flutter/runtime/dart_vm_lifecycle.h" #include "flutter/shell/common/thread_host.h" @@ -18,8 +19,7 @@ class Fixture : public testing::FixtureTest { void TestBody() override{}; }; -static void BM_PlatformMessageResponseDartComplete( - benchmark::State& state) { // NOLINT +static void BM_PlatformMessageResponseDartComplete(benchmark::State& state) { ThreadHost thread_host("test", ThreadHost::Type::Platform | ThreadHost::Type::RASTER | ThreadHost::Type::IO | ThreadHost::Type::UI); @@ -67,7 +67,54 @@ static void BM_PlatformMessageResponseDartComplete( } } +static void BM_PathVolatilityTracker(benchmark::State& state) { + ThreadHost thread_host("test", + ThreadHost::Type::Platform | ThreadHost::Type::RASTER | + ThreadHost::Type::IO | ThreadHost::Type::UI); + TaskRunners task_runners("test", thread_host.platform_thread->GetTaskRunner(), + thread_host.raster_thread->GetTaskRunner(), + thread_host.ui_thread->GetTaskRunner(), + thread_host.io_thread->GetTaskRunner()); + + VolatilePathTracker tracker(task_runners.GetUITaskRunner()); + + while (state.KeepRunning()) { + std::vector> paths; + constexpr int path_count = 1000; + for (int i = 0; i < path_count; i++) { + auto path = std::make_shared(); + path->path = SkPath(); + path->path.setIsVolatile(true); + paths.push_back(std::move(path)); + } + + fml::AutoResetWaitableEvent latch; + task_runners.GetUITaskRunner()->PostTask([&]() { + for (auto path : paths) { + tracker.Insert(path); + } + latch.Signal(); + }); + + latch.Wait(); + + task_runners.GetUITaskRunner()->PostTask([&]() { tracker.OnFrame(); }); + + for (int i = 0; i < path_count - 10; ++i) { + tracker.Erase(paths[i]); + } + + task_runners.GetUITaskRunner()->PostTask([&]() { tracker.OnFrame(); }); + + latch.Reset(); + task_runners.GetUITaskRunner()->PostTask([&]() { latch.Signal(); }); + latch.Wait(); + } +} + BENCHMARK(BM_PlatformMessageResponseDartComplete) ->Unit(benchmark::kMicrosecond); +BENCHMARK(BM_PathVolatilityTracker)->Unit(benchmark::kMillisecond); + } // namespace flutter diff --git a/lib/ui/ui_dart_state.cc b/lib/ui/ui_dart_state.cc index eac218548cfbf..3268e50c52bf9 100644 --- a/lib/ui/ui_dart_state.cc +++ b/lib/ui/ui_dart_state.cc @@ -27,7 +27,8 @@ UIDartState::UIDartState( std::string logger_prefix, UnhandledExceptionCallback unhandled_exception_callback, std::shared_ptr isolate_name_server, - bool is_root_isolate) + bool is_root_isolate, + std::shared_ptr volatile_path_tracker) : task_runners_(std::move(task_runners)), add_callback_(std::move(add_callback)), remove_callback_(std::move(remove_callback)), @@ -36,6 +37,7 @@ UIDartState::UIDartState( io_manager_(std::move(io_manager)), skia_unref_queue_(std::move(skia_unref_queue)), image_decoder_(std::move(image_decoder)), + volatile_path_tracker_(std::move(volatile_path_tracker)), advisory_script_uri_(std::move(advisory_script_uri)), advisory_script_entrypoint_(std::move(advisory_script_entrypoint)), logger_prefix_(std::move(logger_prefix)), @@ -106,6 +108,11 @@ fml::RefPtr UIDartState::GetSkiaUnrefQueue() const { return skia_unref_queue_; } +std::shared_ptr UIDartState::GetVolatilePathTracker() + const { + return volatile_path_tracker_; +} + void UIDartState::ScheduleMicrotask(Dart_Handle closure) { if (tonic::LogIfError(closure) || !Dart_IsClosure(closure)) { return; diff --git a/lib/ui/ui_dart_state.h b/lib/ui/ui_dart_state.h index d5c22ffa3ff70..d1f66baeee048 100644 --- a/lib/ui/ui_dart_state.h +++ b/lib/ui/ui_dart_state.h @@ -20,6 +20,7 @@ #include "flutter/lib/ui/isolate_name_server/isolate_name_server.h" #include "flutter/lib/ui/painting/image_decoder.h" #include "flutter/lib/ui/snapshot_delegate.h" +#include "flutter/lib/ui/volatile_path_tracker.h" #include "third_party/dart/runtime/include/dart_api.h" #include "third_party/skia/include/gpu/GrDirectContext.h" #include "third_party/tonic/dart_microtask_queue.h" @@ -59,6 +60,8 @@ class UIDartState : public tonic::DartState { fml::RefPtr GetSkiaUnrefQueue() const; + std::shared_ptr GetVolatilePathTracker() const; + fml::WeakPtr GetSnapshotDelegate() const; fml::WeakPtr GetHintFreedDelegate() const; @@ -99,7 +102,8 @@ class UIDartState : public tonic::DartState { std::string logger_prefix, UnhandledExceptionCallback unhandled_exception_callback, std::shared_ptr isolate_name_server, - bool is_root_isolate_); + bool is_root_isolate_, + std::shared_ptr volatile_path_tracker); ~UIDartState() override; @@ -121,6 +125,7 @@ class UIDartState : public tonic::DartState { fml::WeakPtr io_manager_; fml::RefPtr skia_unref_queue_; fml::WeakPtr image_decoder_; + std::shared_ptr volatile_path_tracker_; const std::string advisory_script_uri_; const std::string advisory_script_entrypoint_; const std::string logger_prefix_; diff --git a/lib/ui/volatile_path_tracker.cc b/lib/ui/volatile_path_tracker.cc new file mode 100644 index 0000000000000..2922a1de769fc --- /dev/null +++ b/lib/ui/volatile_path_tracker.cc @@ -0,0 +1,74 @@ +// 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. + +#include "flutter/lib/ui/volatile_path_tracker.h" + +namespace flutter { + +VolatilePathTracker::VolatilePathTracker( + fml::RefPtr ui_task_runner) + : ui_task_runner_(ui_task_runner) {} + +void VolatilePathTracker::Insert(std::shared_ptr path) { + FML_DCHECK(ui_task_runner_->RunsTasksOnCurrentThread()); + FML_DCHECK(path); + FML_DCHECK(path->path.isVolatile()); + paths_.insert(path); +} + +void VolatilePathTracker::Erase(std::shared_ptr path) { + FML_DCHECK(path); + if (ui_task_runner_->RunsTasksOnCurrentThread()) { + paths_.erase(path); + return; + } + + std::scoped_lock lock(paths_to_remove_mutex_); + needs_drain_ = true; + paths_to_remove_.push_back(path); +} + +void VolatilePathTracker::OnFrame() { + FML_DCHECK(ui_task_runner_->RunsTasksOnCurrentThread()); + std::string total_count = std::to_string(paths_.size()); + TRACE_EVENT1("flutter", "VolatilePathTracker::OnFrame", "total_count", + total_count.c_str()); + + Drain(); + + std::set> surviving_paths_; + for (const std::shared_ptr path : paths_) { + path->frame_count++; + if (path->frame_count >= kFramesOfVolatility) { + path->path.setIsVolatile(false); + path->tracking_volatility = false; + } else { + surviving_paths_.insert(path); + } + } + paths_.swap(surviving_paths_); + std::string post_removal_count = std::to_string(paths_.size()); + TRACE_EVENT_INSTANT1("flutter", "VolatilePathTracker::OnFrame", + "remaining_count", post_removal_count.c_str()); +} + +void VolatilePathTracker::Drain() { + if (needs_drain_) { + TRACE_EVENT0("flutter", "VolatilePathTracker::Drain"); + std::deque> paths_to_remove; + { + std::scoped_lock lock(paths_to_remove_mutex_); + paths_to_remove.swap(paths_to_remove_); + needs_drain_ = false; + } + std::string count = std::to_string(paths_to_remove.size()); + TRACE_EVENT_INSTANT1("flutter", "VolatilePathTracker::Drain", "count", + count.c_str()); + for (auto path : paths_to_remove) { + paths_.erase(path); + } + } +} + +} // namespace flutter diff --git a/lib/ui/volatile_path_tracker.h b/lib/ui/volatile_path_tracker.h new file mode 100644 index 0000000000000..a0ca69983acf1 --- /dev/null +++ b/lib/ui/volatile_path_tracker.h @@ -0,0 +1,73 @@ +// 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_VOLATILE_PATH_TRACKER_H_ +#define FLUTTER_LIB_VOLATILE_PATH_TRACKER_H_ + +#include +#include +#include + +#include "flutter/fml/macros.h" +#include "flutter/fml/task_runner.h" +#include "flutter/fml/trace_event.h" +#include "third_party/skia/include/core/SkPath.h" + +namespace flutter { + +/// A cache for paths drawn from dart:ui. +/// +/// Whenever a flutter::CanvasPath is created, it must Insert an entry into +/// this cache. Whenever a frame is drawn, the shell must call OnFrame. The +/// cache will flip the volatility bit on the SkPath and remove it from the +/// cache. If the Dart object is released, Erase must be called to avoid +/// tracking a path that is no longer referenced in Dart code. +class VolatilePathTracker { + public: + /// The fields of this struct must only accessed on the UI task runner. + struct TrackedPath { + bool tracking_volatility = false; + int frame_count = 0; + SkPath path; + }; + + explicit VolatilePathTracker(fml::RefPtr ui_task_runner); + + static constexpr int kFramesOfVolatility = 2; + + // Starts tracking a path. + // Must be called from the UI task runner. + // + // Callers should only insert paths that are currently volatile. + void Insert(std::shared_ptr path); + + // Removes a path from tracking. + // + // May be called from any thread. + void Erase(std::shared_ptr path); + + // Called by the shell at the end of a frame after notifying Dart about idle + // time. + // + // This method will flip the volatility bit to false for any paths that have + // survived the |kFramesOfVolatility|. + // + // Must be called from the UI task runner. + void OnFrame(); + + private: + fml::RefPtr ui_task_runner_; + std::atomic_bool needs_drain_ = false; + std::mutex paths_to_remove_mutex_; + std::deque> paths_to_remove_; + std::set> paths_; + + void Drain(); + + FML_DISALLOW_COPY_AND_ASSIGN(VolatilePathTracker); +}; + +} // namespace flutter + +#endif // FLUTTER_LIB_VOLATILE_PATH_TRACKER_H_ diff --git a/runtime/dart_isolate.cc b/runtime/dart_isolate.cc index 095c4965bc482..15fe61f30ce01 100644 --- a/runtime/dart_isolate.cc +++ b/runtime/dart_isolate.cc @@ -90,7 +90,8 @@ std::weak_ptr DartIsolate::CreateRunningRootIsolate( const fml::closure& isolate_shutdown_callback, std::optional dart_entrypoint, std::optional dart_entrypoint_library, - std::unique_ptr isolate_configration) { + std::unique_ptr isolate_configration, + std::shared_ptr volatile_path_tracker) { if (!isolate_snapshot) { FML_LOG(ERROR) << "Invalid isolate snapshot."; return {}; @@ -117,7 +118,8 @@ std::weak_ptr DartIsolate::CreateRunningRootIsolate( advisory_script_entrypoint, // isolate_flags, // isolate_create_callback, // - isolate_shutdown_callback // + isolate_shutdown_callback, // + std::move(volatile_path_tracker) // ) .lock(); @@ -187,7 +189,8 @@ std::weak_ptr DartIsolate::CreateRootIsolate( std::string advisory_script_entrypoint, Flags flags, const fml::closure& isolate_create_callback, - const fml::closure& isolate_shutdown_callback) { + const fml::closure& isolate_shutdown_callback, + std::shared_ptr volatile_path_tracker) { TRACE_EVENT0("flutter", "DartIsolate::CreateRootIsolate"); // The child isolate preparer is null but will be set when the isolate is @@ -206,16 +209,17 @@ 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(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 + 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 + std::move(volatile_path_tracker) // volatile path tracker ))); DartErrorString error; @@ -241,16 +245,18 @@ std::weak_ptr DartIsolate::CreateRootIsolate( return (*root_isolate_data)->GetWeakIsolatePtr(); } -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, - std::string advisory_script_uri, - std::string advisory_script_entrypoint, - bool is_root_isolate) +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, + std::string advisory_script_uri, + std::string advisory_script_entrypoint, + bool is_root_isolate, + std::shared_ptr volatile_path_tracker) : UIDartState(std::move(task_runners), settings.task_observer_add, settings.task_observer_remove, @@ -264,7 +270,8 @@ DartIsolate::DartIsolate(const Settings& settings, settings.log_tag, settings.unhandled_exception_callback, DartVMRef::GetIsolateNameServer(), - is_root_isolate), + is_root_isolate, + std::move(volatile_path_tracker)), may_insecurely_connect_to_all_domains_( settings.may_insecurely_connect_to_all_domains), domain_network_policy_(settings.domain_network_policy) { @@ -752,7 +759,8 @@ Dart_Isolate DartIsolate::DartCreateAndStartServiceIsolate( DART_VM_SERVICE_ISOLATE_NAME, // script entrypoint DartIsolate::Flags{flags}, // flags nullptr, // isolate create callback - nullptr // isolate shutdown callback + nullptr, // isolate shutdown callback + nullptr // volatile path tracker ); std::shared_ptr service_isolate = weak_service_isolate.lock(); @@ -858,7 +866,8 @@ Dart_Isolate DartIsolate::DartIsolateGroupCreateCallback( fml::WeakPtr{}, // image_decoder advisory_script_uri, // advisory_script_uri advisory_script_entrypoint, // advisory_script_entrypoint - false))); // is_root_isolate + false, // is_root_isolate + nullptr))); // volatile path tracker Dart_Isolate vm_isolate = CreateDartIsolateGroup( std::move(isolate_group_data), std::move(isolate_data), flags, error); @@ -903,7 +912,8 @@ bool DartIsolate::DartIsolateInitializeCallback(void** child_callback_data, (*isolate_group_data)->GetAdvisoryScriptURI(), // advisory_script_uri (*isolate_group_data) ->GetAdvisoryScriptEntrypoint(), // advisory_script_entrypoint - false))); // is_root_isolate + false, // is_root_isolate + nullptr))); // volatile path tracker // root isolate should have been created via CreateRootIsolate if (!InitializeIsolate(*embedder_isolate, isolate, error)) { diff --git a/runtime/dart_isolate.h b/runtime/dart_isolate.h index 457d7edc1e128..81a4c8b1ee6a6 100644 --- a/runtime/dart_isolate.h +++ b/runtime/dart_isolate.h @@ -19,6 +19,7 @@ #include "flutter/lib/ui/io_manager.h" #include "flutter/lib/ui/snapshot_delegate.h" #include "flutter/lib/ui/ui_dart_state.h" +#include "flutter/lib/ui/volatile_path_tracker.h" #include "flutter/lib/ui/window/platform_configuration.h" #include "flutter/runtime/dart_snapshot.h" #include "third_party/dart/runtime/include/dart_api.h" @@ -230,7 +231,8 @@ class DartIsolate : public UIDartState { const fml::closure& isolate_shutdown_callback, std::optional dart_entrypoint, std::optional dart_entrypoint_library, - std::unique_ptr isolate_configration); + std::unique_ptr isolate_configration, + std::shared_ptr volatile_path_tracker); // |UIDartState| ~DartIsolate() override; @@ -430,7 +432,8 @@ class DartIsolate : public UIDartState { std::string advisory_script_entrypoint, Flags flags, const fml::closure& isolate_create_callback, - const fml::closure& isolate_shutdown_callback); + const fml::closure& isolate_shutdown_callback, + std::shared_ptr volatile_path_tracker); DartIsolate(const Settings& settings, TaskRunners task_runners, @@ -441,7 +444,8 @@ class DartIsolate : public UIDartState { fml::WeakPtr image_decoder, std::string advisory_script_uri, std::string advisory_script_entrypoint, - bool is_root_isolate); + bool is_root_isolate, + std::shared_ptr volatile_path_tracker); [[nodiscard]] bool Initialize(Dart_Isolate isolate); diff --git a/runtime/dart_isolate_unittests.cc b/runtime/dart_isolate_unittests.cc index 8c0116e1b2e0b..1b59e70c81621 100644 --- a/runtime/dart_isolate_unittests.cc +++ b/runtime/dart_isolate_unittests.cc @@ -68,7 +68,8 @@ TEST_F(DartIsolateTest, RootIsolateCreationAndShutdown) { settings.isolate_shutdown_callback, // isolate shutdown callback "main", // dart entrypoint std::nullopt, // dart entrypoint library - std::move(isolate_configuration) // isolate configuration + std::move(isolate_configuration), // isolate configuration + nullptr // Volatile path tracker ); auto root_isolate = weak_isolate.lock(); ASSERT_TRUE(root_isolate); @@ -108,7 +109,8 @@ TEST_F(DartIsolateTest, IsolateShutdownCallbackIsInIsolateScope) { settings.isolate_shutdown_callback, // isolate shutdown callback "main", // dart entrypoint std::nullopt, // dart entrypoint library - std::move(isolate_configuration) // isolate configuration + std::move(isolate_configuration), // isolate configuration + nullptr // Volatile path tracker ); auto root_isolate = weak_isolate.lock(); ASSERT_TRUE(root_isolate); @@ -366,7 +368,8 @@ TEST_F(DartIsolateTest, CanCreateServiceIsolate) { settings.isolate_shutdown_callback, // isolate shutdown callback "main", // dart entrypoint std::nullopt, // dart entrypoint library - std::move(isolate_configuration) // isolate configuration + std::move(isolate_configuration), // isolate configuration + nullptr // Volatile path tracker ); auto root_isolate = weak_isolate.lock(); ASSERT_TRUE(root_isolate); @@ -466,7 +469,8 @@ TEST_F(DartIsolateTest, InvalidLoadingUnitFails) { settings.isolate_shutdown_callback, // isolate shutdown callback "main", // dart entrypoint std::nullopt, // dart entrypoint library - std::move(isolate_configuration) // isolate configuration + std::move(isolate_configuration), // isolate configuration + nullptr // volatile path tracker ); auto root_isolate = weak_isolate.lock(); ASSERT_TRUE(root_isolate); diff --git a/runtime/dart_lifecycle_unittests.cc b/runtime/dart_lifecycle_unittests.cc index 8576b3fb11292..09de19f6e1687 100644 --- a/runtime/dart_lifecycle_unittests.cc +++ b/runtime/dart_lifecycle_unittests.cc @@ -73,7 +73,8 @@ static std::shared_ptr CreateAndRunRootIsolate( settings.isolate_shutdown_callback, // isolate shutdown callback, entrypoint, // dart entrypoint std::nullopt, // dart entrypoint library - std::move(isolate_configuration) // isolate configuration + std::move(isolate_configuration), // isolate configuration + nullptr // Volatile path tracker ) .lock(); diff --git a/runtime/fixtures/runtime_test.dart b/runtime/fixtures/runtime_test.dart index d559e6ec3d02b..9e137e1fb6acb 100644 --- a/runtime/fixtures/runtime_test.dart +++ b/runtime/fixtures/runtime_test.dart @@ -2,6 +2,8 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +// @dart=2.10 + import 'dart:async'; import 'dart:isolate'; import 'dart:ui'; diff --git a/runtime/fixtures/split_lib_test.dart b/runtime/fixtures/split_lib_test.dart index 5d811fc14c374..1f4822552a395 100644 --- a/runtime/fixtures/split_lib_test.dart +++ b/runtime/fixtures/split_lib_test.dart @@ -2,6 +2,8 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +// @dart=2.10 + library splitlib; int splitAdd(int i, int j) { diff --git a/runtime/runtime_controller.cc b/runtime/runtime_controller.cc index 6e512f17f648a..4998a9aeb3f50 100644 --- a/runtime/runtime_controller.cc +++ b/runtime/runtime_controller.cc @@ -37,7 +37,8 @@ RuntimeController::RuntimeController( const PlatformData& p_platform_data, const fml::closure& p_isolate_create_callback, const fml::closure& p_isolate_shutdown_callback, - std::shared_ptr p_persistent_isolate_data) + std::shared_ptr p_persistent_isolate_data, + std::shared_ptr p_volatile_path_tracker) : client_(p_client), vm_(p_vm), isolate_snapshot_(std::move(p_isolate_snapshot)), @@ -53,7 +54,8 @@ RuntimeController::RuntimeController( platform_data_(std::move(p_platform_data)), isolate_create_callback_(p_isolate_create_callback), isolate_shutdown_callback_(p_isolate_shutdown_callback), - persistent_isolate_data_(std::move(p_persistent_isolate_data)) {} + persistent_isolate_data_(std::move(p_persistent_isolate_data)), + volatile_path_tracker_(std::move(p_volatile_path_tracker)) {} RuntimeController::~RuntimeController() { FML_DCHECK(Dart_CurrentIsolate() == nullptr); @@ -93,7 +95,8 @@ std::unique_ptr RuntimeController::Clone() const { platform_data_, // isolate_create_callback_, // isolate_shutdown_callback_, // - persistent_isolate_data_ // + persistent_isolate_data_, // + volatile_path_tracker_ // )); } @@ -369,7 +372,8 @@ bool RuntimeController::LaunchRootIsolate( isolate_shutdown_callback_, // dart_entrypoint, // dart_entrypoint_library, // - std::move(isolate_configuration) // + std::move(isolate_configuration), // + volatile_path_tracker_ // ) .lock(); diff --git a/runtime/runtime_controller.h b/runtime/runtime_controller.h index 5bca3a4c9d89c..5e2c26da52726 100644 --- a/runtime/runtime_controller.h +++ b/runtime/runtime_controller.h @@ -15,6 +15,7 @@ #include "flutter/lib/ui/io_manager.h" #include "flutter/lib/ui/text/font_collection.h" #include "flutter/lib/ui/ui_dart_state.h" +#include "flutter/lib/ui/volatile_path_tracker.h" #include "flutter/lib/ui/window/platform_configuration.h" #include "flutter/lib/ui/window/pointer_data_packet.h" #include "flutter/runtime/dart_vm.h" @@ -110,6 +111,8 @@ class RuntimeController : public PlatformConfigurationClient { /// @param[in] persistent_isolate_data Unstructured persistent read-only /// data that the root isolate can /// access in a synchronous manner. + /// @param[in] volatile_path_tracker Cache for tracking path + /// volatility. /// RuntimeController( RuntimeDelegate& client, @@ -127,7 +130,8 @@ class RuntimeController : public PlatformConfigurationClient { const PlatformData& platform_data, const fml::closure& isolate_create_callback, const fml::closure& isolate_shutdown_callback, - std::shared_ptr persistent_isolate_data); + std::shared_ptr persistent_isolate_data, + std::shared_ptr volatile_path_tracker); // |PlatformConfigurationClient| ~RuntimeController() override; @@ -576,6 +580,7 @@ class RuntimeController : public PlatformConfigurationClient { const fml::closure isolate_create_callback_; const fml::closure isolate_shutdown_callback_; std::shared_ptr persistent_isolate_data_; + std::shared_ptr volatile_path_tracker_; PlatformConfiguration* GetPlatformConfigurationIfAvailable(); diff --git a/shell/common/engine.cc b/shell/common/engine.cc index 2a90326bc558d..84de3e25bf92c 100644 --- a/shell/common/engine.cc +++ b/shell/common/engine.cc @@ -66,7 +66,8 @@ Engine::Engine(Delegate& delegate, std::unique_ptr animator, fml::WeakPtr io_manager, fml::RefPtr unref_queue, - fml::WeakPtr snapshot_delegate) + fml::WeakPtr snapshot_delegate, + std::shared_ptr volatile_path_tracker) : Engine(delegate, dispatcher_maker, vm.GetConcurrentWorkerTaskRunner(), @@ -91,7 +92,8 @@ Engine::Engine(Delegate& delegate, platform_data, // platform data settings_.isolate_create_callback, // isolate create callback settings_.isolate_shutdown_callback, // isolate shutdown callback - settings_.persistent_isolate_data // persistent isolate data + settings_.persistent_isolate_data, // persistent isolate data + std::move(volatile_path_tracker) // volatile path tracker ); } diff --git a/shell/common/engine.h b/shell/common/engine.h index 85b7ece2c69ca..8041e14199add 100644 --- a/shell/common/engine.h +++ b/shell/common/engine.h @@ -19,6 +19,7 @@ #include "flutter/lib/ui/semantics/semantics_node.h" #include "flutter/lib/ui/snapshot_delegate.h" #include "flutter/lib/ui/text/font_collection.h" +#include "flutter/lib/ui/volatile_path_tracker.h" #include "flutter/lib/ui/window/platform_message.h" #include "flutter/lib/ui/window/viewport_metrics.h" #include "flutter/runtime/dart_vm.h" @@ -350,7 +351,8 @@ class Engine final : public RuntimeDelegate, std::unique_ptr animator, fml::WeakPtr io_manager, fml::RefPtr unref_queue, - fml::WeakPtr snapshot_delegate); + fml::WeakPtr snapshot_delegate, + std::shared_ptr volatile_path_tracker); //---------------------------------------------------------------------------- /// @brief Destroys the engine engine. Called by the shell on the UI task diff --git a/shell/common/shell.cc b/shell/common/shell.cc index a8fa087317c3c..a58451fd2d5ba 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -20,6 +20,7 @@ #include "flutter/fml/paths.h" #include "flutter/fml/trace_event.h" #include "flutter/fml/unique_fd.h" +#include "flutter/lib/ui/painting/path.h" #include "flutter/runtime/dart_vm.h" #include "flutter/shell/common/engine.h" #include "flutter/shell/common/skia_event_tracer_impl.h" @@ -52,8 +53,9 @@ std::unique_ptr Shell::CreateShellOnPlatformThread( return nullptr; } - auto shell = - std::unique_ptr(new Shell(std::move(vm), task_runners, settings)); + auto shell = std::unique_ptr(new Shell( + std::move(vm), task_runners, settings, + std::make_shared(task_runners.GetUITaskRunner()))); // Create the rasterizer on the raster thread. std::promise> rasterizer_promise; @@ -149,17 +151,18 @@ std::unique_ptr Shell::CreateShellOnPlatformThread( std::move(vsync_waiter)); engine_promise.set_value(std::make_unique( - *shell, // - dispatcher_maker, // - *shell->GetDartVM(), // - std::move(isolate_snapshot), // - task_runners, // - platform_data, // - shell->GetSettings(), // - std::move(animator), // - weak_io_manager_future.get(), // - unref_queue_future.get(), // - snapshot_delegate_future.get() // + *shell, // + dispatcher_maker, // + *shell->GetDartVM(), // + std::move(isolate_snapshot), // + task_runners, // + platform_data, // + shell->GetSettings(), // + std::move(animator), // + weak_io_manager_future.get(), // + unref_queue_future.get(), // + snapshot_delegate_future.get(), // + shell->volatile_path_tracker_ // )); })); @@ -321,11 +324,15 @@ std::unique_ptr Shell::Create( return shell; } -Shell::Shell(DartVMRef vm, TaskRunners task_runners, Settings settings) +Shell::Shell(DartVMRef vm, + TaskRunners task_runners, + Settings settings, + std::shared_ptr volatile_path_tracker) : task_runners_(std::move(task_runners)), settings_(std::move(settings)), vm_(std::move(vm)), is_gpu_disabled_sync_switch_(new fml::SyncSwitch()), + volatile_path_tracker_(std::move(volatile_path_tracker)), weak_factory_gpu_(nullptr), weak_factory_(this) { FML_CHECK(vm_) << "Must have access to VM to create a shell."; @@ -1029,6 +1036,7 @@ void Shell::OnAnimatorNotifyIdle(int64_t deadline) { if (engine_) { engine_->NotifyIdle(deadline); + volatile_path_tracker_->OnFrame(); } } diff --git a/shell/common/shell.h b/shell/common/shell.h index 2166450f09656..503347c39bbd6 100644 --- a/shell/common/shell.h +++ b/shell/common/shell.h @@ -27,6 +27,7 @@ #include "flutter/fml/time/time_point.h" #include "flutter/lib/ui/semantics/custom_accessibility_action.h" #include "flutter/lib/ui/semantics/semantics_node.h" +#include "flutter/lib/ui/volatile_path_tracker.h" #include "flutter/lib/ui/window/platform_message.h" #include "flutter/runtime/dart_vm_lifecycle.h" #include "flutter/runtime/service_protocol.h" @@ -410,6 +411,7 @@ class Shell final : public PlatformView::Delegate, std::unique_ptr rasterizer_; // on raster task runner std::unique_ptr io_manager_; // on IO task runner std::shared_ptr is_gpu_disabled_sync_switch_; + std::shared_ptr volatile_path_tracker_; fml::WeakPtr weak_engine_; // to be shared across threads fml::TaskRunnerAffineWeakPtr @@ -459,7 +461,10 @@ class Shell final : public PlatformView::Delegate, // How many frames have been timed since last report. size_t UnreportedFramesCount() const; - Shell(DartVMRef vm, TaskRunners task_runners, Settings settings); + Shell(DartVMRef vm, + TaskRunners task_runners, + Settings settings, + std::shared_ptr volatile_path_tracker); static std::unique_ptr CreateShellOnPlatformThread( DartVMRef vm, diff --git a/testing/dart/path_test.dart b/testing/dart/path_test.dart index cd9db9d09c34a..e635ed87bdda2 100644 --- a/testing/dart/path_test.dart +++ b/testing/dart/path_test.dart @@ -78,6 +78,14 @@ void main() { expect(p1.getBounds().bottom, equals(p2.getBounds().bottom + 10)); }); + test('shift tests', () { + const Rect bounds = Rect.fromLTRB(0.0, 0.0, 10.0, 10.0); + final Path p = Path()..addRect(bounds); + expect(p.getBounds(), equals(bounds)); + final Path shifted = p.shift(const Offset(10, 10)); + expect(shifted.getBounds(), equals(const Rect.fromLTRB(10, 10, 20, 20))); + }); + test('transformation tests', () { const Rect bounds = Rect.fromLTRB(0.0, 0.0, 10.0, 10.0); final Path p = Path()..addRect(bounds); diff --git a/testing/dart_isolate_runner.cc b/testing/dart_isolate_runner.cc index 43d77c0165c28..86f6c66892e9f 100644 --- a/testing/dart_isolate_runner.cc +++ b/testing/dart_isolate_runner.cc @@ -56,7 +56,8 @@ std::unique_ptr RunDartCodeInIsolateOnUITaskRunner( std::string entrypoint, const std::vector& args, const std::string& fixtures_path, - fml::WeakPtr io_manager) { + fml::WeakPtr io_manager, + std::shared_ptr volatile_path_tracker) { FML_CHECK(task_runners.GetUITaskRunner()->RunsTasksOnCurrentThread()); if (!vm_ref) { @@ -126,7 +127,8 @@ std::unique_ptr RunDartCodeInIsolateOnUITaskRunner( settings.isolate_shutdown_callback, // isolate shutdown callback entrypoint, // entrypoint std::nullopt, // library - std::move(isolate_configuration) // isolate configuration + std::move(isolate_configuration), // isolate configuration + std::move(volatile_path_tracker) // volatile path tracker ) .lock(); @@ -146,14 +148,15 @@ std::unique_ptr RunDartCodeInIsolate( std::string entrypoint, const std::vector& args, const std::string& fixtures_path, - fml::WeakPtr io_manager) { + fml::WeakPtr io_manager, + std::shared_ptr volatile_path_tracker) { std::unique_ptr result; fml::AutoResetWaitableEvent latch; fml::TaskRunner::RunNowOrPostTask( task_runners.GetUITaskRunner(), fml::MakeCopyable([&]() mutable { result = RunDartCodeInIsolateOnUITaskRunner( vm_ref, settings, task_runners, entrypoint, args, fixtures_path, - io_manager); + io_manager, std::move(volatile_path_tracker)); latch.Signal(); })); latch.Wait(); diff --git a/testing/dart_isolate_runner.h b/testing/dart_isolate_runner.h index 83c91a1e5c3a8..ab6029e38b02b 100644 --- a/testing/dart_isolate_runner.h +++ b/testing/dart_isolate_runner.h @@ -42,14 +42,16 @@ class AutoIsolateShutdown { FML_DISALLOW_COPY_AND_ASSIGN(AutoIsolateShutdown); }; -void RunDartCodeInIsolate(DartVMRef& vm_ref, - std::unique_ptr& result, - const Settings& settings, - const TaskRunners& task_runners, - std::string entrypoint, - const std::vector& args, - const std::string& fixtures_path, - fml::WeakPtr io_manager = {}); +void RunDartCodeInIsolate( + DartVMRef& vm_ref, + std::unique_ptr& result, + const Settings& settings, + const TaskRunners& task_runners, + std::string entrypoint, + const std::vector& args, + const std::string& fixtures_path, + fml::WeakPtr io_manager = {}, + std::shared_ptr volatile_path_tracker = nullptr); std::unique_ptr RunDartCodeInIsolate( DartVMRef& vm_ref, @@ -58,7 +60,8 @@ std::unique_ptr RunDartCodeInIsolate( std::string entrypoint, const std::vector& args, const std::string& fixtures_path, - fml::WeakPtr io_manager = {}); + fml::WeakPtr io_manager = {}, + std::shared_ptr volatile_path_tracker = nullptr); } // namespace testing } // namespace flutter diff --git a/third_party/tonic/tests/dart_state_unittest.cc b/third_party/tonic/tests/dart_state_unittest.cc index 3d053de40e75b..8a91cc20b41ae 100644 --- a/third_party/tonic/tests/dart_state_unittest.cc +++ b/third_party/tonic/tests/dart_state_unittest.cc @@ -44,7 +44,8 @@ TEST_F(DartState, IsShuttingDown) { settings.isolate_shutdown_callback, // isolate shutdown callback "main", // dart entrypoint std::nullopt, // dart entrypoint library - std::move(isolate_configuration) // isolate configuration + std::move(isolate_configuration), // isolate configuration + nullptr // Volatile path tracker ); auto root_isolate = weak_isolate.lock(); ASSERT_TRUE(root_isolate); From 6f4f6f838c281456b184c6aadf586d521b7f0f63 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Mon, 21 Dec 2020 15:21:54 -0800 Subject: [PATCH 2/4] allow disabling based on whether deterministic rendering is needed --- lib/ui/volatile_path_tracker.cc | 19 +++++++++++++++---- lib/ui/volatile_path_tracker.h | 9 ++++++++- shell/common/shell.cc | 8 +++++--- 3 files changed, 28 insertions(+), 8 deletions(-) diff --git a/lib/ui/volatile_path_tracker.cc b/lib/ui/volatile_path_tracker.cc index 2922a1de769fc..b82012bfa30a8 100644 --- a/lib/ui/volatile_path_tracker.cc +++ b/lib/ui/volatile_path_tracker.cc @@ -7,17 +7,25 @@ namespace flutter { VolatilePathTracker::VolatilePathTracker( - fml::RefPtr ui_task_runner) - : ui_task_runner_(ui_task_runner) {} + fml::RefPtr ui_task_runner, + bool enabled) + : ui_task_runner_(ui_task_runner), enabled_(enabled) {} void VolatilePathTracker::Insert(std::shared_ptr path) { FML_DCHECK(ui_task_runner_->RunsTasksOnCurrentThread()); FML_DCHECK(path); FML_DCHECK(path->path.isVolatile()); + if (!enabled_) { + path->path.setIsVolatile(false); + return; + } paths_.insert(path); } void VolatilePathTracker::Erase(std::shared_ptr path) { + if (!enabled_) { + return; + } FML_DCHECK(path); if (ui_task_runner_->RunsTasksOnCurrentThread()) { paths_.erase(path); @@ -31,6 +39,9 @@ void VolatilePathTracker::Erase(std::shared_ptr path) { void VolatilePathTracker::OnFrame() { FML_DCHECK(ui_task_runner_->RunsTasksOnCurrentThread()); + if (!enabled_) { + return; + } std::string total_count = std::to_string(paths_.size()); TRACE_EVENT1("flutter", "VolatilePathTracker::OnFrame", "total_count", total_count.c_str()); @@ -38,7 +49,7 @@ void VolatilePathTracker::OnFrame() { Drain(); std::set> surviving_paths_; - for (const std::shared_ptr path : paths_) { + for (const std::shared_ptr& path : paths_) { path->frame_count++; if (path->frame_count >= kFramesOfVolatility) { path->path.setIsVolatile(false); @@ -65,7 +76,7 @@ void VolatilePathTracker::Drain() { std::string count = std::to_string(paths_to_remove.size()); TRACE_EVENT_INSTANT1("flutter", "VolatilePathTracker::Drain", "count", count.c_str()); - for (auto path : paths_to_remove) { + for (auto& path : paths_to_remove) { paths_.erase(path); } } diff --git a/lib/ui/volatile_path_tracker.h b/lib/ui/volatile_path_tracker.h index a0ca69983acf1..9bb317d761c80 100644 --- a/lib/ui/volatile_path_tracker.h +++ b/lib/ui/volatile_path_tracker.h @@ -23,6 +23,11 @@ namespace flutter { /// cache will flip the volatility bit on the SkPath and remove it from the /// cache. If the Dart object is released, Erase must be called to avoid /// tracking a path that is no longer referenced in Dart code. +/// +/// Enabling this cache may cause difficult to predict minor pixel differences +/// when paths are rendered. If deterministic rendering is needed, e.g. for a +/// screen diffing test, this class will not cache any paths and will +/// automatically set the volatility of the path to false. class VolatilePathTracker { public: /// The fields of this struct must only accessed on the UI task runner. @@ -32,7 +37,8 @@ class VolatilePathTracker { SkPath path; }; - explicit VolatilePathTracker(fml::RefPtr ui_task_runner); + VolatilePathTracker(fml::RefPtr ui_task_runner, + bool enabled); static constexpr int kFramesOfVolatility = 2; @@ -62,6 +68,7 @@ class VolatilePathTracker { std::mutex paths_to_remove_mutex_; std::deque> paths_to_remove_; std::set> paths_; + bool enabled_ = true; void Drain(); diff --git a/shell/common/shell.cc b/shell/common/shell.cc index a58451fd2d5ba..046f56373601a 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -53,9 +53,11 @@ std::unique_ptr Shell::CreateShellOnPlatformThread( return nullptr; } - auto shell = std::unique_ptr(new Shell( - std::move(vm), task_runners, settings, - std::make_shared(task_runners.GetUITaskRunner()))); + auto shell = std::unique_ptr( + new Shell(std::move(vm), task_runners, settings, + std::make_shared( + task_runners.GetUITaskRunner(), + !settings.skia_deterministic_rendering_on_cpu))); // Create the rasterizer on the raster thread. std::promise> rasterizer_promise; From d75cf20a812ab7ce4bb95eae227664a45865d8be Mon Sep 17 00:00:00 2001 From: Dan Field Date: Mon, 21 Dec 2020 15:41:55 -0800 Subject: [PATCH 3/4] fix build --- lib/ui/ui_benchmarks.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ui/ui_benchmarks.cc b/lib/ui/ui_benchmarks.cc index 16a1ae2a0c158..739885710d7a2 100644 --- a/lib/ui/ui_benchmarks.cc +++ b/lib/ui/ui_benchmarks.cc @@ -76,7 +76,7 @@ static void BM_PathVolatilityTracker(benchmark::State& state) { thread_host.ui_thread->GetTaskRunner(), thread_host.io_thread->GetTaskRunner()); - VolatilePathTracker tracker(task_runners.GetUITaskRunner()); + VolatilePathTracker tracker(task_runners.GetUITaskRunner(), true); while (state.KeepRunning()) { std::vector> paths; From e553673a5467a258b89a75f7cadb4c38fd6a7743 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Mon, 21 Dec 2020 21:36:28 -0800 Subject: [PATCH 4/4] test --- lib/ui/painting/path_unittests.cc | 57 +++++++++++++++++++++++++++++++ lib/ui/volatile_path_tracker.h | 2 ++ 2 files changed, 59 insertions(+) diff --git a/lib/ui/painting/path_unittests.cc b/lib/ui/painting/path_unittests.cc index ed5a52500e1a2..277e9e5b1e5da 100644 --- a/lib/ui/painting/path_unittests.cc +++ b/lib/ui/painting/path_unittests.cc @@ -124,5 +124,62 @@ TEST_F(ShellTest, PathVolatilityGCRemovesPathFromTracker) { DestroyShell(std::move(shell), std::move(task_runners)); } +// Screen diffing tests use deterministic rendering. Allowing a path to be +// volatile or not for an individual frame can result in minor pixel differences +// that cause the test to fail. +// If deterministic rendering is enabled, the tracker should be disabled and +// paths should always be non-volatile. +TEST_F(ShellTest, DeterministicRenderingDisablesPathVolatility) { + auto message_latch = std::make_shared(); + + auto native_validate_path = [message_latch](Dart_NativeArguments args) { + auto handle = Dart_GetNativeArgument(args, 0); + intptr_t peer = 0; + Dart_Handle result = Dart_GetNativeInstanceField( + handle, tonic::DartWrappable::kPeerIndex, &peer); + EXPECT_FALSE(Dart_IsError(result)); + CanvasPath* path = reinterpret_cast(peer); + EXPECT_TRUE(path); + EXPECT_FALSE(path->path().isVolatile()); + std::shared_ptr tracker = + UIDartState::Current()->GetVolatilePathTracker(); + EXPECT_TRUE(tracker); + EXPECT_FALSE(tracker->enabled()); + + for (int i = 0; i < VolatilePathTracker::kFramesOfVolatility; i++) { + tracker->OnFrame(); + EXPECT_FALSE(path->path().isVolatile()); + } + EXPECT_FALSE(path->path().isVolatile()); + message_latch->Signal(); + }; + + Settings settings = CreateSettingsForFixture(); + settings.skia_deterministic_rendering_on_cpu = true; + TaskRunners task_runners("test", // label + GetCurrentTaskRunner(), // platform + CreateNewThread(), // raster + CreateNewThread(), // ui + CreateNewThread() // io + ); + + AddNativeCallback("ValidatePath", CREATE_NATIVE_ENTRY(native_validate_path)); + + std::unique_ptr shell = + CreateShell(std::move(settings), std::move(task_runners)); + + ASSERT_TRUE(shell->IsSetup()); + auto configuration = RunConfiguration::InferFromSettings(settings); + configuration.SetEntrypoint("createPath"); + + shell->RunEngine(std::move(configuration), [](auto result) { + ASSERT_EQ(result, Engine::RunStatus::Success); + }); + + message_latch->Wait(); + + DestroyShell(std::move(shell), std::move(task_runners)); +} + } // namespace testing } // namespace flutter diff --git a/lib/ui/volatile_path_tracker.h b/lib/ui/volatile_path_tracker.h index 9bb317d761c80..40f28311a008a 100644 --- a/lib/ui/volatile_path_tracker.h +++ b/lib/ui/volatile_path_tracker.h @@ -62,6 +62,8 @@ class VolatilePathTracker { // Must be called from the UI task runner. void OnFrame(); + bool enabled() const { return enabled_; } + private: fml::RefPtr ui_task_runner_; std::atomic_bool needs_drain_ = false;