From bc78eeae2423a4cd47dd81be87094c052f5d23f2 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Thu, 19 Nov 2020 13:45:07 -0800 Subject: [PATCH 01/18] does not work yet --- fml/memory/thread_checker.h | 2 ++ lib/ui/painting/path.cc | 69 +++++++++++++++++++++++++++++++++++-- lib/ui/painting/path.h | 11 ++++++ shell/common/shell.cc | 2 ++ 4 files changed, 82 insertions(+), 2 deletions(-) diff --git a/fml/memory/thread_checker.h b/fml/memory/thread_checker.h index 5290fc2ba8115..c7b56583c2769 100644 --- a/fml/memory/thread_checker.h +++ b/fml/memory/thread_checker.h @@ -8,6 +8,7 @@ #ifndef FLUTTER_FML_MEMORY_THREAD_CHECKER_H_ #define FLUTTER_FML_MEMORY_THREAD_CHECKER_H_ +#include "flutter/fml/backtrace.h" #include "flutter/fml/build_config.h" #include "flutter/fml/logging.h" #include "flutter/fml/macros.h" @@ -61,6 +62,7 @@ class ThreadChecker final { FML_DLOG(ERROR) << "IsCreationThreadCurrent expected thread: '" << expected_thread << "' actual thread:'" << actual_thread << "'"; + FML_DLOG(ERROR) << fml::BacktraceHere(); } } #endif // __APPLE__ diff --git a/lib/ui/painting/path.cc b/lib/ui/painting/path.cc index d0898a4ca283c..79250076aa812 100644 --- a/lib/ui/painting/path.cc +++ b/lib/ui/painting/path.cc @@ -67,9 +67,46 @@ void CanvasPath::RegisterNatives(tonic::DartLibraryNatives* natives) { FOR_EACH_BINDING(DART_REGISTER_NATIVE)}); } -CanvasPath::CanvasPath() {} +CanvasPath::CanvasPath() : weak_factory_(this) { + resetVolatility(); +} + +CanvasPath::~CanvasPath() = default; -CanvasPath::~CanvasPath() {} +void CanvasPath::resetVolatility() { + if (!tracking_volatility_) { + path_.setIsVolatile(true); + volatile_paths_.push_back(weak_factory_.GetWeakPtr()); + tracking_volatility_ = true; + } +} + +// static +std::mutex CanvasPath::volatile_paths_mutex_; +// static +std::vector> CanvasPath::volatile_paths_; +// static +void CanvasPath::updatePathVolatility() { + FML_DCHECK(UIDartState::Current()); + FML_DCHECK(UIDartState::Current() + ->GetTaskRunners() + .GetUITaskRunner() + ->RunsTasksOnCurrentThread()); + std::scoped_lock guard(volatile_paths_mutex_); + std::vector> remaining_paths; + for (auto weak_path : volatile_paths_) { + if (weak_path) { + weak_path->volatility_count_++; + if (weak_path->volatility_count_ >= 2) { + weak_path->path_.setIsVolatile(false); + weak_path->tracking_volatility_ = false; + } else { + remaining_paths.push_back(weak_path); + } + } + } + volatile_paths_ = std::move(remaining_paths); +} int CanvasPath::getFillType() { return static_cast(path_.getFillType()); @@ -77,26 +114,32 @@ int CanvasPath::getFillType() { void CanvasPath::setFillType(int fill_type) { path_.setFillType(static_cast(fill_type)); + resetVolatility(); } void CanvasPath::moveTo(float x, float y) { path_.moveTo(x, y); + resetVolatility(); } void CanvasPath::relativeMoveTo(float x, float y) { path_.rMoveTo(x, y); + resetVolatility(); } void CanvasPath::lineTo(float x, float y) { path_.lineTo(x, y); + resetVolatility(); } void CanvasPath::relativeLineTo(float x, float y) { path_.rLineTo(x, y); + resetVolatility(); } void CanvasPath::quadraticBezierTo(float x1, float y1, float x2, float y2) { path_.quadTo(x1, y1, x2, y2); + resetVolatility(); } void CanvasPath::relativeQuadraticBezierTo(float x1, @@ -104,6 +147,7 @@ void CanvasPath::relativeQuadraticBezierTo(float x1, float x2, float y2) { path_.rQuadTo(x1, y1, x2, y2); + resetVolatility(); } void CanvasPath::cubicTo(float x1, @@ -113,6 +157,7 @@ void CanvasPath::cubicTo(float x1, float x3, float y3) { path_.cubicTo(x1, y1, x2, y2, x3, y3); + resetVolatility(); } void CanvasPath::relativeCubicTo(float x1, @@ -122,10 +167,12 @@ void CanvasPath::relativeCubicTo(float x1, float x3, float y3) { 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); + resetVolatility(); } void CanvasPath::relativeConicTo(float x1, @@ -134,6 +181,7 @@ void CanvasPath::relativeConicTo(float x1, float y2, float w) { path_.rConicTo(x1, y1, x2, y2, w); + resetVolatility(); } void CanvasPath::arcTo(float left, @@ -146,6 +194,7 @@ void CanvasPath::arcTo(float left, 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, @@ -162,6 +211,7 @@ void CanvasPath::arcToPoint(float arcEndX, path_.arcTo(radiusX, radiusY, xAxisRotation, arcSize, direction, arcEndX, arcEndY); + resetVolatility(); } void CanvasPath::relativeArcToPoint(float arcEndDeltaX, @@ -177,14 +227,17 @@ void CanvasPath::relativeArcToPoint(float arcEndDeltaX, isClockwiseDirection ? SkPathDirection::kCW : SkPathDirection::kCCW; 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)); + resetVolatility(); } void CanvasPath::addOval(float left, float top, float right, float bottom) { path_.addOval(SkRect::MakeLTRB(left, top, right, bottom)); + resetVolatility(); } void CanvasPath::addArc(float left, @@ -195,15 +248,18 @@ void CanvasPath::addArc(float left, float sweepAngle) { 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); + resetVolatility(); } void CanvasPath::addRRect(const RRect& rrect) { path_.addRRect(rrect.sk_rrect); + resetVolatility(); } void CanvasPath::addPath(CanvasPath* path, double dx, double dy) { @@ -212,6 +268,7 @@ void CanvasPath::addPath(CanvasPath* path, double dx, double dy) { return; } path_.addPath(path->path(), dx, dy, SkPath::kAppend_AddPathMode); + resetVolatility(); } void CanvasPath::addPathWithMatrix(CanvasPath* path, @@ -229,6 +286,7 @@ void CanvasPath::addPathWithMatrix(CanvasPath* path, matrix.setTranslateY(matrix.getTranslateY() + dy); path_.addPath(path->path(), matrix, SkPath::kAppend_AddPathMode); matrix4.Release(); + resetVolatility(); } void CanvasPath::extendWithPath(CanvasPath* path, double dx, double dy) { @@ -238,6 +296,7 @@ void CanvasPath::extendWithPath(CanvasPath* path, double dx, double dy) { return; } path_.addPath(path->path(), dx, dy, SkPath::kExtend_AddPathMode); + resetVolatility(); } void CanvasPath::extendWithPathAndMatrix(CanvasPath* path, @@ -255,14 +314,17 @@ void CanvasPath::extendWithPathAndMatrix(CanvasPath* path, matrix.setTranslateY(matrix.getTranslateY() + dy); path_.addPath(path->path(), matrix, SkPath::kExtend_AddPathMode); matrix4.Release(); + resetVolatility(); } void CanvasPath::close() { path_.close(); + resetVolatility(); } void CanvasPath::reset() { path_.reset(); + resetVolatility(); } bool CanvasPath::contains(double x, double y) { @@ -272,6 +334,7 @@ bool CanvasPath::contains(double x, double 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_); + resetVolatility(); } void CanvasPath::transform(Dart_Handle path_handle, @@ -279,6 +342,7 @@ void CanvasPath::transform(Dart_Handle path_handle, fml::RefPtr path = CanvasPath::Create(path_handle); path_.transform(ToSkMatrix(matrix4), &path->path_); matrix4.Release(); + resetVolatility(); } tonic::Float32List CanvasPath::getBounds() { @@ -294,6 +358,7 @@ tonic::Float32List CanvasPath::getBounds() { bool CanvasPath::op(CanvasPath* path1, CanvasPath* path2, int operation) { return Op(path1->path(), path2->path(), static_cast(operation), &path_); + resetVolatility(); } void CanvasPath::clone(Dart_Handle path_handle) { diff --git a/lib/ui/painting/path.h b/lib/ui/painting/path.h index dbd6d7a0d1cc2..c4018948d232b 100644 --- a/lib/ui/painting/path.h +++ b/lib/ui/painting/path.h @@ -5,6 +5,7 @@ #ifndef FLUTTER_LIB_UI_PAINTING_PATH_H_ #define FLUTTER_LIB_UI_PAINTING_PATH_H_ +#include "flutter/fml/memory/weak_ptr.h" #include "flutter/lib/ui/dart_wrapper.h" #include "flutter/lib/ui/painting/rrect.h" #include "third_party/skia/include/core/SkPath.h" @@ -114,10 +115,20 @@ class CanvasPath : public RefCountedDartWrappable { static void RegisterNatives(tonic::DartLibraryNatives* natives); + static void updatePathVolatility(); + private: CanvasPath(); + static std::mutex volatile_paths_mutex_; + static std::vector> volatile_paths_; + SkPath path_; + bool tracking_volatility_ = false; + int volatility_count_ = 0; + fml::WeakPtrFactory weak_factory_; + + void resetVolatility(); }; } // namespace flutter diff --git a/shell/common/shell.cc b/shell/common/shell.cc index a8a8b884e4c0f..8ecfc23270312 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" @@ -1014,6 +1015,7 @@ void Shell::OnAnimatorNotifyIdle(int64_t deadline) { if (engine_) { engine_->NotifyIdle(deadline); + CanvasPath::updatePathVolatility(); } } From f2867c889172938ce968cb3e5d00ea808fa4615e Mon Sep 17 00:00:00 2001 From: Dan Field Date: Fri, 20 Nov 2020 16:38:52 -0800 Subject: [PATCH 02/18] safe? --- lib/ui/painting/path.cc | 133 ++++++++++++++++++++++------------------ lib/ui/painting/path.h | 23 ++++--- 2 files changed, 87 insertions(+), 69 deletions(-) diff --git a/lib/ui/painting/path.cc b/lib/ui/painting/path.cc index 79250076aa812..35169f9581658 100644 --- a/lib/ui/painting/path.cc +++ b/lib/ui/painting/path.cc @@ -67,78 +67,83 @@ void CanvasPath::RegisterNatives(tonic::DartLibraryNatives* natives) { FOR_EACH_BINDING(DART_REGISTER_NATIVE)}); } -CanvasPath::CanvasPath() : weak_factory_(this) { +CanvasPath::CanvasPath() + : tracked_path_(std::make_shared()) { resetVolatility(); } CanvasPath::~CanvasPath() = default; void CanvasPath::resetVolatility() { - if (!tracking_volatility_) { - path_.setIsVolatile(true); - volatile_paths_.push_back(weak_factory_.GetWeakPtr()); - tracking_volatility_ = true; + if (!tracked_path_->tracking_volatility) { + tracked_path_->path_.setIsVolatile(true); + tracked_path_->tracking_volatility = true; + { + std::scoped_lock guard(volatile_paths_mutex_); + volatile_paths_.insert(tracked_path_); + } } } // static std::mutex CanvasPath::volatile_paths_mutex_; // static -std::vector> CanvasPath::volatile_paths_; +std::unordered_set> + CanvasPath::volatile_paths_; // static void CanvasPath::updatePathVolatility() { - FML_DCHECK(UIDartState::Current()); - FML_DCHECK(UIDartState::Current() - ->GetTaskRunners() - .GetUITaskRunner() - ->RunsTasksOnCurrentThread()); + TRACE_EVENT0("flutter", "updatePathVolatility"); std::scoped_lock guard(volatile_paths_mutex_); - std::vector> remaining_paths; - for (auto weak_path : volatile_paths_) { - if (weak_path) { - weak_path->volatility_count_++; - if (weak_path->volatility_count_ >= 2) { - weak_path->path_.setIsVolatile(false); - weak_path->tracking_volatility_ = false; - } else { - remaining_paths.push_back(weak_path); - } + for (auto it = volatile_paths_.begin(), last = volatile_paths_.end(); + it != last;) { + auto path = *it; + path->frame_count++; + if (path->frame_count >= 2) { + path->path_.setIsVolatile(false); + path->tracking_volatility = false; + it = volatile_paths_.erase(it); + } else { + ++it; } } - volatile_paths_ = std::move(remaining_paths); +} + +void CanvasPath::ReleaseDartWrappableReference() const { + std::scoped_lock guard(volatile_paths_mutex_); + volatile_paths_.erase(tracked_path_); } int CanvasPath::getFillType() { - return static_cast(path_.getFillType()); + return static_cast(tracked_path_->path_.getFillType()); } void CanvasPath::setFillType(int fill_type) { - path_.setFillType(static_cast(fill_type)); + tracked_path_->path_.setFillType(static_cast(fill_type)); resetVolatility(); } void CanvasPath::moveTo(float x, float y) { - path_.moveTo(x, y); + tracked_path_->path_.moveTo(x, y); resetVolatility(); } void CanvasPath::relativeMoveTo(float x, float y) { - path_.rMoveTo(x, y); + tracked_path_->path_.rMoveTo(x, y); resetVolatility(); } void CanvasPath::lineTo(float x, float y) { - path_.lineTo(x, y); + tracked_path_->path_.lineTo(x, y); resetVolatility(); } void CanvasPath::relativeLineTo(float x, float y) { - path_.rLineTo(x, y); + tracked_path_->path_.rLineTo(x, y); resetVolatility(); } void CanvasPath::quadraticBezierTo(float x1, float y1, float x2, float y2) { - path_.quadTo(x1, y1, x2, y2); + tracked_path_->path_.quadTo(x1, y1, x2, y2); resetVolatility(); } @@ -146,7 +151,7 @@ void CanvasPath::relativeQuadraticBezierTo(float x1, float y1, float x2, float y2) { - path_.rQuadTo(x1, y1, x2, y2); + tracked_path_->path_.rQuadTo(x1, y1, x2, y2); resetVolatility(); } @@ -156,7 +161,7 @@ void CanvasPath::cubicTo(float x1, float y2, float x3, float y3) { - path_.cubicTo(x1, y1, x2, y2, x3, y3); + tracked_path_->path_.cubicTo(x1, y1, x2, y2, x3, y3); resetVolatility(); } @@ -166,12 +171,12 @@ void CanvasPath::relativeCubicTo(float x1, float y2, float x3, float y3) { - path_.rCubicTo(x1, y1, x2, y2, x3, y3); + tracked_path_->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); + tracked_path_->path_.conicTo(x1, y1, x2, y2, w); resetVolatility(); } @@ -180,7 +185,7 @@ void CanvasPath::relativeConicTo(float x1, float x2, float y2, float w) { - path_.rConicTo(x1, y1, x2, y2, w); + tracked_path_->path_.rConicTo(x1, y1, x2, y2, w); resetVolatility(); } @@ -191,9 +196,9 @@ 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); + tracked_path_->path_.arcTo(SkRect::MakeLTRB(left, top, right, bottom), + startAngle * 180.0 / M_PI, + sweepAngle * 180.0 / M_PI, forceMoveTo); resetVolatility(); } @@ -209,8 +214,8 @@ void CanvasPath::arcToPoint(float arcEndX, const auto direction = isClockwiseDirection ? SkPathDirection::kCW : SkPathDirection::kCCW; - path_.arcTo(radiusX, radiusY, xAxisRotation, arcSize, direction, arcEndX, - arcEndY); + tracked_path_->path_.arcTo(radiusX, radiusY, xAxisRotation, arcSize, + direction, arcEndX, arcEndY); resetVolatility(); } @@ -225,18 +230,18 @@ 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); + tracked_path_->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)); + tracked_path_->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)); + tracked_path_->path_.addOval(SkRect::MakeLTRB(left, top, right, bottom)); resetVolatility(); } @@ -246,19 +251,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); + tracked_path_->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); + tracked_path_->path_.addPoly(reinterpret_cast(points.data()), + points.num_elements() / 2, close); resetVolatility(); } void CanvasPath::addRRect(const RRect& rrect) { - path_.addRRect(rrect.sk_rrect); + tracked_path_->path_.addRRect(rrect.sk_rrect); resetVolatility(); } @@ -267,7 +273,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); + tracked_path_->path_.addPath(path->path(), dx, dy, + SkPath::kAppend_AddPathMode); resetVolatility(); } @@ -284,7 +291,8 @@ 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); + tracked_path_->path_.addPath(path->path(), matrix, + SkPath::kAppend_AddPathMode); matrix4.Release(); resetVolatility(); } @@ -295,7 +303,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); + path->tracked_path_->path_.addPath(path->path(), dx, dy, + SkPath::kExtend_AddPathMode); resetVolatility(); } @@ -312,42 +321,44 @@ 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); + path->tracked_path_->path_.addPath(path->path(), matrix, + SkPath::kExtend_AddPathMode); matrix4.Release(); resetVolatility(); } void CanvasPath::close() { - path_.close(); + tracked_path_->path_.close(); resetVolatility(); } void CanvasPath::reset() { - path_.reset(); + tracked_path_->path_.reset(); resetVolatility(); } bool CanvasPath::contains(double x, double y) { - return path_.contains(x, y); + return tracked_path_->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_); + tracked_path_->path_.offset(dx, dy, &path->tracked_path_->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_); + tracked_path_->path_.transform(ToSkMatrix(matrix4), + &path->tracked_path_->path_); matrix4.Release(); resetVolatility(); } tonic::Float32List CanvasPath::getBounds() { tonic::Float32List rect(Dart_NewTypedData(Dart_TypedData_kFloat32, 4)); - const SkRect& bounds = path_.getBounds(); + const SkRect& bounds = tracked_path_->path_.getBounds(); rect[0] = bounds.left(); rect[1] = bounds.top(); rect[2] = bounds.right(); @@ -357,7 +368,7 @@ 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(); } @@ -365,14 +376,14 @@ 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->tracked_path_->path_ = tracked_path_->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) + tracked_path_->path_.approximateBytesUsed(); } } // namespace flutter diff --git a/lib/ui/painting/path.h b/lib/ui/painting/path.h index c4018948d232b..5855600e1807c 100644 --- a/lib/ui/painting/path.h +++ b/lib/ui/painting/path.h @@ -5,7 +5,8 @@ #ifndef FLUTTER_LIB_UI_PAINTING_PATH_H_ #define FLUTTER_LIB_UI_PAINTING_PATH_H_ -#include "flutter/fml/memory/weak_ptr.h" +#include + #include "flutter/lib/ui/dart_wrapper.h" #include "flutter/lib/ui/painting/rrect.h" #include "third_party/skia/include/core/SkPath.h" @@ -37,7 +38,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; } @@ -109,7 +110,7 @@ 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; @@ -117,16 +118,22 @@ class CanvasPath : public RefCountedDartWrappable { static void updatePathVolatility(); + virtual void ReleaseDartWrappableReference() const override; + private: + struct TrackedVolatilePath { + bool tracking_volatility = false; + int frame_count = 0; + SkPath path_; + }; + CanvasPath(); static std::mutex volatile_paths_mutex_; - static std::vector> volatile_paths_; + static std::unordered_set> + volatile_paths_; - SkPath path_; - bool tracking_volatility_ = false; - int volatility_count_ = 0; - fml::WeakPtrFactory weak_factory_; + std::shared_ptr tracked_path_; void resetVolatility(); }; From 100b2eca6d84277d802798b6a5f66dd71f9fe260 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Fri, 20 Nov 2020 16:50:17 -0800 Subject: [PATCH 03/18] one more --- lib/ui/painting/path.cc | 2 +- lib/ui/painting/path.h | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/lib/ui/painting/path.cc b/lib/ui/painting/path.cc index 35169f9581658..85cdd26eb059d 100644 --- a/lib/ui/painting/path.cc +++ b/lib/ui/painting/path.cc @@ -98,7 +98,7 @@ void CanvasPath::updatePathVolatility() { it != last;) { auto path = *it; path->frame_count++; - if (path->frame_count >= 2) { + if (path->frame_count >= number_of_frames_until_non_volatile) { path->path_.setIsVolatile(false); path->tracking_volatility = false; it = volatile_paths_.erase(it); diff --git a/lib/ui/painting/path.h b/lib/ui/painting/path.h index 5855600e1807c..044d525f0c1d6 100644 --- a/lib/ui/painting/path.h +++ b/lib/ui/painting/path.h @@ -116,6 +116,11 @@ class CanvasPath : public RefCountedDartWrappable { static void RegisterNatives(tonic::DartLibraryNatives* natives); + // 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 |number_of_frames_until_non_volatile|. static void updatePathVolatility(); virtual void ReleaseDartWrappableReference() const override; @@ -129,12 +134,14 @@ class CanvasPath : public RefCountedDartWrappable { CanvasPath(); + static constexpr int number_of_frames_until_non_volatile = 2; static std::mutex volatile_paths_mutex_; static std::unordered_set> volatile_paths_; std::shared_ptr tracked_path_; + // Must be called whenever the path is created or mutated. void resetVolatility(); }; From 6a84f9eb2ef56610d8877f363f2f519f73c9f576 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Fri, 20 Nov 2020 16:58:01 -0800 Subject: [PATCH 04/18] missing mutex --- fml/memory/thread_checker.h | 2 -- lib/ui/painting/path.h | 1 + 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/fml/memory/thread_checker.h b/fml/memory/thread_checker.h index c7b56583c2769..5290fc2ba8115 100644 --- a/fml/memory/thread_checker.h +++ b/fml/memory/thread_checker.h @@ -8,7 +8,6 @@ #ifndef FLUTTER_FML_MEMORY_THREAD_CHECKER_H_ #define FLUTTER_FML_MEMORY_THREAD_CHECKER_H_ -#include "flutter/fml/backtrace.h" #include "flutter/fml/build_config.h" #include "flutter/fml/logging.h" #include "flutter/fml/macros.h" @@ -62,7 +61,6 @@ class ThreadChecker final { FML_DLOG(ERROR) << "IsCreationThreadCurrent expected thread: '" << expected_thread << "' actual thread:'" << actual_thread << "'"; - FML_DLOG(ERROR) << fml::BacktraceHere(); } } #endif // __APPLE__ diff --git a/lib/ui/painting/path.h b/lib/ui/painting/path.h index 044d525f0c1d6..486db1c7d0c3b 100644 --- a/lib/ui/painting/path.h +++ b/lib/ui/painting/path.h @@ -5,6 +5,7 @@ #ifndef FLUTTER_LIB_UI_PAINTING_PATH_H_ #define FLUTTER_LIB_UI_PAINTING_PATH_H_ +#include #include #include "flutter/lib/ui/dart_wrapper.h" From 24df8c7da1b800e18a925f3e2cc29e1487871dd0 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Fri, 20 Nov 2020 21:40:46 -0800 Subject: [PATCH 05/18] UIDartState/shell ownership --- ci/licenses_golden/licenses_flutter | 2 + lib/ui/BUILD.gn | 2 + lib/ui/painting/path.cc | 124 +++++++----------- lib/ui/painting/path.h | 27 +--- lib/ui/ui_dart_state.cc | 9 +- lib/ui/ui_dart_state.h | 7 +- lib/ui/volatile_path_tracker.cc | 42 ++++++ lib/ui/volatile_path_tracker.h | 55 ++++++++ runtime/dart_isolate.cc | 64 +++++---- runtime/dart_isolate.h | 10 +- runtime/dart_isolate_unittests.cc | 9 +- runtime/dart_lifecycle_unittests.cc | 3 +- 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_isolate_runner.cc | 3 +- .../tonic/tests/dart_state_unittest.cc | 3 +- 20 files changed, 273 insertions(+), 159 deletions(-) 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 d7eb1dac4dc2a..97433821ac1a5 100755 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -404,6 +404,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/lib/ui/BUILD.gn b/lib/ui/BUILD.gn index 6ce9238b2738d..f4eeec64e60a2 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", diff --git a/lib/ui/painting/path.cc b/lib/ui/painting/path.cc index 85cdd26eb059d..9ff9e89536bb3 100644 --- a/lib/ui/painting/path.cc +++ b/lib/ui/painting/path.cc @@ -68,7 +68,9 @@ void CanvasPath::RegisterNatives(tonic::DartLibraryNatives* natives) { } CanvasPath::CanvasPath() - : tracked_path_(std::make_shared()) { + : path_tracker_(UIDartState::Current()->GetVolatilePathTracker()), + tracked_path_(std::make_shared()) { + FML_DCHECK(path_tracker_); resetVolatility(); } @@ -76,74 +78,48 @@ CanvasPath::~CanvasPath() = default; void CanvasPath::resetVolatility() { if (!tracked_path_->tracking_volatility) { - tracked_path_->path_.setIsVolatile(true); + mutable_path().setIsVolatile(true); tracked_path_->tracking_volatility = true; - { - std::scoped_lock guard(volatile_paths_mutex_); - volatile_paths_.insert(tracked_path_); - } - } -} - -// static -std::mutex CanvasPath::volatile_paths_mutex_; -// static -std::unordered_set> - CanvasPath::volatile_paths_; -// static -void CanvasPath::updatePathVolatility() { - TRACE_EVENT0("flutter", "updatePathVolatility"); - std::scoped_lock guard(volatile_paths_mutex_); - for (auto it = volatile_paths_.begin(), last = volatile_paths_.end(); - it != last;) { - auto path = *it; - path->frame_count++; - if (path->frame_count >= number_of_frames_until_non_volatile) { - path->path_.setIsVolatile(false); - path->tracking_volatility = false; - it = volatile_paths_.erase(it); - } else { - ++it; - } + path_tracker_->insert(tracked_path_); } } void CanvasPath::ReleaseDartWrappableReference() const { - std::scoped_lock guard(volatile_paths_mutex_); - volatile_paths_.erase(tracked_path_); + FML_DCHECK(path_tracker_); + path_tracker_->erase(tracked_path_); } int CanvasPath::getFillType() { - return static_cast(tracked_path_->path_.getFillType()); + return static_cast(path().getFillType()); } void CanvasPath::setFillType(int fill_type) { - tracked_path_->path_.setFillType(static_cast(fill_type)); + mutable_path().setFillType(static_cast(fill_type)); resetVolatility(); } void CanvasPath::moveTo(float x, float y) { - tracked_path_->path_.moveTo(x, y); + mutable_path().moveTo(x, y); resetVolatility(); } void CanvasPath::relativeMoveTo(float x, float y) { - tracked_path_->path_.rMoveTo(x, y); + mutable_path().rMoveTo(x, y); resetVolatility(); } void CanvasPath::lineTo(float x, float y) { - tracked_path_->path_.lineTo(x, y); + mutable_path().lineTo(x, y); resetVolatility(); } void CanvasPath::relativeLineTo(float x, float y) { - tracked_path_->path_.rLineTo(x, y); + mutable_path().rLineTo(x, y); resetVolatility(); } void CanvasPath::quadraticBezierTo(float x1, float y1, float x2, float y2) { - tracked_path_->path_.quadTo(x1, y1, x2, y2); + mutable_path().quadTo(x1, y1, x2, y2); resetVolatility(); } @@ -151,7 +127,7 @@ void CanvasPath::relativeQuadraticBezierTo(float x1, float y1, float x2, float y2) { - tracked_path_->path_.rQuadTo(x1, y1, x2, y2); + mutable_path().rQuadTo(x1, y1, x2, y2); resetVolatility(); } @@ -161,7 +137,7 @@ void CanvasPath::cubicTo(float x1, float y2, float x3, float y3) { - tracked_path_->path_.cubicTo(x1, y1, x2, y2, x3, y3); + mutable_path().cubicTo(x1, y1, x2, y2, x3, y3); resetVolatility(); } @@ -171,12 +147,12 @@ void CanvasPath::relativeCubicTo(float x1, float y2, float x3, float y3) { - tracked_path_->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) { - tracked_path_->path_.conicTo(x1, y1, x2, y2, w); + mutable_path().conicTo(x1, y1, x2, y2, w); resetVolatility(); } @@ -185,7 +161,7 @@ void CanvasPath::relativeConicTo(float x1, float x2, float y2, float w) { - tracked_path_->path_.rConicTo(x1, y1, x2, y2, w); + mutable_path().rConicTo(x1, y1, x2, y2, w); resetVolatility(); } @@ -196,9 +172,9 @@ void CanvasPath::arcTo(float left, float startAngle, float sweepAngle, bool forceMoveTo) { - tracked_path_->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(); } @@ -214,8 +190,8 @@ void CanvasPath::arcToPoint(float arcEndX, const auto direction = isClockwiseDirection ? SkPathDirection::kCW : SkPathDirection::kCCW; - tracked_path_->path_.arcTo(radiusX, radiusY, xAxisRotation, arcSize, - direction, arcEndX, arcEndY); + mutable_path().arcTo(radiusX, radiusY, xAxisRotation, arcSize, direction, + arcEndX, arcEndY); resetVolatility(); } @@ -230,18 +206,18 @@ void CanvasPath::relativeArcToPoint(float arcEndDeltaX, : SkPath::ArcSize::kSmall_ArcSize; const auto direction = isClockwiseDirection ? SkPathDirection::kCW : SkPathDirection::kCCW; - tracked_path_->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) { - tracked_path_->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) { - tracked_path_->path_.addOval(SkRect::MakeLTRB(left, top, right, bottom)); + mutable_path().addOval(SkRect::MakeLTRB(left, top, right, bottom)); resetVolatility(); } @@ -251,20 +227,19 @@ void CanvasPath::addArc(float left, float bottom, float startAngle, float sweepAngle) { - tracked_path_->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) { - tracked_path_->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) { - tracked_path_->path_.addRRect(rrect.sk_rrect); + mutable_path().addRRect(rrect.sk_rrect); resetVolatility(); } @@ -273,8 +248,7 @@ void CanvasPath::addPath(CanvasPath* path, double dx, double dy) { Dart_ThrowException(ToDart("Path.addPath called with non-genuine Path.")); return; } - tracked_path_->path_.addPath(path->path(), dx, dy, - SkPath::kAppend_AddPathMode); + mutable_path().addPath(path->path(), dx, dy, SkPath::kAppend_AddPathMode); resetVolatility(); } @@ -291,8 +265,7 @@ void CanvasPath::addPathWithMatrix(CanvasPath* path, SkMatrix matrix = ToSkMatrix(matrix4); matrix.setTranslateX(matrix.getTranslateX() + dx); matrix.setTranslateY(matrix.getTranslateY() + dy); - tracked_path_->path_.addPath(path->path(), matrix, - SkPath::kAppend_AddPathMode); + mutable_path().addPath(path->path(), matrix, SkPath::kAppend_AddPathMode); matrix4.Release(); resetVolatility(); } @@ -303,8 +276,8 @@ void CanvasPath::extendWithPath(CanvasPath* path, double dx, double dy) { ToDart("Path.extendWithPath called with non-genuine Path.")); return; } - path->tracked_path_->path_.addPath(path->path(), dx, dy, - SkPath::kExtend_AddPathMode); + path->mutable_path().addPath(path->path(), dx, dy, + SkPath::kExtend_AddPathMode); resetVolatility(); } @@ -321,44 +294,45 @@ void CanvasPath::extendWithPathAndMatrix(CanvasPath* path, SkMatrix matrix = ToSkMatrix(matrix4); matrix.setTranslateX(matrix.getTranslateX() + dx); matrix.setTranslateY(matrix.getTranslateY() + dy); - path->tracked_path_->path_.addPath(path->path(), matrix, - SkPath::kExtend_AddPathMode); + path->mutable_path().addPath(path->path(), matrix, + SkPath::kExtend_AddPathMode); matrix4.Release(); resetVolatility(); } void CanvasPath::close() { - tracked_path_->path_.close(); + mutable_path().close(); resetVolatility(); } void CanvasPath::reset() { - tracked_path_->path_.reset(); + mutable_path().reset(); resetVolatility(); } bool CanvasPath::contains(double x, double y) { - return tracked_path_->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); - tracked_path_->path_.offset(dx, dy, &path->tracked_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); - tracked_path_->path_.transform(ToSkMatrix(matrix4), - &path->tracked_path_->path_); + auto other_mutable_path = path->mutable_path(); + mutable_path().transform(ToSkMatrix(matrix4), &other_mutable_path); matrix4.Release(); resetVolatility(); } tonic::Float32List CanvasPath::getBounds() { tonic::Float32List rect(Dart_NewTypedData(Dart_TypedData_kFloat32, 4)); - const SkRect& bounds = tracked_path_->path_.getBounds(); + const SkRect& bounds = path().getBounds(); rect[0] = bounds.left(); rect[1] = bounds.top(); rect[2] = bounds.right(); @@ -376,14 +350,14 @@ 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->tracked_path_->path_ = tracked_path_->path_; + path->tracked_path_->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) + tracked_path_->path_.approximateBytesUsed(); + return sizeof(CanvasPath) + path().approximateBytesUsed(); } } // namespace flutter diff --git a/lib/ui/painting/path.h b/lib/ui/painting/path.h index 486db1c7d0c3b..e4481824eff4f 100644 --- a/lib/ui/painting/path.h +++ b/lib/ui/painting/path.h @@ -5,11 +5,9 @@ #ifndef FLUTTER_LIB_UI_PAINTING_PATH_H_ #define FLUTTER_LIB_UI_PAINTING_PATH_H_ -#include -#include - #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" @@ -117,33 +115,18 @@ class CanvasPath : public RefCountedDartWrappable { static void RegisterNatives(tonic::DartLibraryNatives* natives); - // 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 |number_of_frames_until_non_volatile|. - static void updatePathVolatility(); - virtual void ReleaseDartWrappableReference() const override; private: - struct TrackedVolatilePath { - bool tracking_volatility = false; - int frame_count = 0; - SkPath path_; - }; - CanvasPath(); - static constexpr int number_of_frames_until_non_volatile = 2; - static std::mutex volatile_paths_mutex_; - static std::unordered_set> - volatile_paths_; - - std::shared_ptr tracked_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/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..c78c6870b7477 --- /dev/null +++ b/lib/ui/volatile_path_tracker.cc @@ -0,0 +1,42 @@ +// 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); + fml::TaskRunner::RunNowOrPostTask(ui_task_runner_, + [&]() { paths_.erase(path); }); +} + +void VolatilePathTracker::onFrame() { + FML_DCHECK(ui_task_runner_->RunsTasksOnCurrentThread()); + TRACE_EVENT0("flutter", "VolatilePathTracker::onFrame"); + for (auto it = paths_.begin(), last = paths_.end(); it != last;) { + auto path = *it; + path->frame_count++; + if (path->frame_count >= number_of_frames_until_non_volatile) { + path->path_.setIsVolatile(false); + path->tracking_volatility = false; + it = paths_.erase(it); + } else { + ++it; + } + } +} + +} // namespace flutter diff --git a/lib/ui/volatile_path_tracker.h b/lib/ui/volatile_path_tracker.h new file mode 100644 index 0000000000000..f44f69536314e --- /dev/null +++ b/lib/ui/volatile_path_tracker.h @@ -0,0 +1,55 @@ +// 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 "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 { + +class VolatilePathTracker { + public: + struct Path { + bool tracking_volatility = false; + int frame_count = 0; + SkPath path_; + }; + + explicit VolatilePathTracker(fml::RefPtr ui_task_runner); + + static constexpr int number_of_frames_until_non_volatile = 2; + + // Starts tracking a path. + // Must be called from the UI task runner. + 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 |number_of_frames_until_non_volatile|. + // + // Must be called from the UI task runner. + void onFrame(); + + private: + fml::RefPtr ui_task_runner_; + std::unordered_set> paths_; + 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 8a6215110e426..2ad8a91444171 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) { @@ -717,7 +724,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(); @@ -823,7 +831,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); @@ -868,7 +877,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 5e6913d52b185..5a0449739066f 100644 --- a/runtime/dart_isolate.h +++ b/runtime/dart_isolate.h @@ -18,6 +18,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" @@ -229,7 +230,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; @@ -419,7 +421,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, @@ -430,7 +433,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 a90599fd8a3fb..d59cb08b64eab 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); 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/runtime_controller.cc b/runtime/runtime_controller.cc index 3551e1967134e..481b2f7d8006a 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 22941c7388a1b..831a1a4d66ce7 100644 --- a/runtime/runtime_controller.h +++ b/runtime/runtime_controller.h @@ -14,6 +14,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" @@ -109,6 +110,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, @@ -126,7 +129,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; @@ -510,6 +514,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 1b9c02413138b..5497a1fb0bd70 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 93a5f2367dbc3..ab5beaec86470 100644 --- a/shell/common/engine.h +++ b/shell/common/engine.h @@ -18,6 +18,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" @@ -343,7 +344,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 4b46fc8c22049..a0867a38253b7 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -53,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; @@ -150,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_ // )); })); @@ -322,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."; @@ -1015,7 +1021,7 @@ void Shell::OnAnimatorNotifyIdle(int64_t deadline) { if (engine_) { engine_->NotifyIdle(deadline); - CanvasPath::updatePathVolatility(); + volatile_path_tracker_->onFrame(); } } diff --git a/shell/common/shell.h b/shell/common/shell.h index 7ac297e5d07c6..c399fdd7bbc21 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" @@ -397,6 +398,7 @@ class Shell final : public PlatformView::Delegate, std::unique_ptr rasterizer_; // on GPU 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 @@ -446,7 +448,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_isolate_runner.cc b/testing/dart_isolate_runner.cc index 43d77c0165c28..d3a7c49baa0dd 100644 --- a/testing/dart_isolate_runner.cc +++ b/testing/dart_isolate_runner.cc @@ -126,7 +126,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 + nullptr // volatile path tracker ) .lock(); 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 280b3c9238461eee2ef7e0e2264cd4bfdcd841b5 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Mon, 30 Nov 2020 09:25:35 -0800 Subject: [PATCH 06/18] review and fixup errors introduced --- lib/ui/painting/path.cc | 20 +++++++++++--------- lib/ui/volatile_path_tracker.cc | 11 +++++++---- lib/ui/volatile_path_tracker.h | 13 ++++++++++--- runtime/runtime_controller.h | 2 +- shell/common/shell.cc | 2 +- 5 files changed, 30 insertions(+), 18 deletions(-) diff --git a/lib/ui/painting/path.cc b/lib/ui/painting/path.cc index 9ff9e89536bb3..a509e247ea74a 100644 --- a/lib/ui/painting/path.cc +++ b/lib/ui/painting/path.cc @@ -79,14 +79,17 @@ 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_); + path_tracker_->Insert(tracked_path_); } } void CanvasPath::ReleaseDartWrappableReference() const { FML_DCHECK(path_tracker_); - path_tracker_->erase(tracked_path_); + if (tracked_path_->tracking_volatility) { + path_tracker_->Erase(tracked_path_); + } } int CanvasPath::getFillType() { @@ -276,8 +279,7 @@ void CanvasPath::extendWithPath(CanvasPath* path, double dx, double dy) { ToDart("Path.extendWithPath called with non-genuine Path.")); return; } - path->mutable_path().addPath(path->path(), dx, dy, - SkPath::kExtend_AddPathMode); + mutable_path().addPath(path->path(), dx, dy, SkPath::kExtend_AddPathMode); resetVolatility(); } @@ -294,8 +296,7 @@ void CanvasPath::extendWithPathAndMatrix(CanvasPath* path, SkMatrix matrix = ToSkMatrix(matrix4); matrix.setTranslateX(matrix.getTranslateX() + dx); matrix.setTranslateY(matrix.getTranslateY() + dy); - path->mutable_path().addPath(path->path(), matrix, - SkPath::kExtend_AddPathMode); + mutable_path().addPath(path->path(), matrix, SkPath::kExtend_AddPathMode); matrix4.Release(); resetVolatility(); } @@ -324,15 +325,16 @@ void CanvasPath::shift(Dart_Handle path_handle, double dx, double dy) { void CanvasPath::transform(Dart_Handle path_handle, tonic::Float64List& matrix4) { fml::RefPtr path = CanvasPath::Create(path_handle); - auto other_mutable_path = path->mutable_path(); + auto& other_mutable_path = path->mutable_path(); mutable_path().transform(ToSkMatrix(matrix4), &other_mutable_path); matrix4.Release(); - resetVolatility(); } tonic::Float32List CanvasPath::getBounds() { tonic::Float32List rect(Dart_NewTypedData(Dart_TypedData_kFloat32, 4)); const SkRect& bounds = path().getBounds(); + FML_DLOG(ERROR) << "Bounds " << bounds.left() << " " << bounds.top() << " " + << bounds.right() << " " << bounds.bottom(); rect[0] = bounds.left(); rect[1] = bounds.top(); rect[2] = bounds.right(); @@ -350,7 +352,7 @@ 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->tracked_path_->path_ = this->path(); + path->mutable_path() = this->path(); } // This is doomed to be called too early, since Paths are mutable. diff --git a/lib/ui/volatile_path_tracker.cc b/lib/ui/volatile_path_tracker.cc index c78c6870b7477..2809f642b5361 100644 --- a/lib/ui/volatile_path_tracker.cc +++ b/lib/ui/volatile_path_tracker.cc @@ -10,22 +10,23 @@ VolatilePathTracker::VolatilePathTracker( fml::RefPtr ui_task_runner) : ui_task_runner_(ui_task_runner) {} -void VolatilePathTracker::insert(std::shared_ptr path) { +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) { +void VolatilePathTracker::Erase(std::shared_ptr path) { FML_DCHECK(path); fml::TaskRunner::RunNowOrPostTask(ui_task_runner_, [&]() { paths_.erase(path); }); } -void VolatilePathTracker::onFrame() { +void VolatilePathTracker::OnFrame() { FML_DCHECK(ui_task_runner_->RunsTasksOnCurrentThread()); - TRACE_EVENT0("flutter", "VolatilePathTracker::onFrame"); + TRACE_EVENT1("flutter", "VolatilePathTracker::OnFrame", "count", + std::to_string(paths_.size()).c_str()); for (auto it = paths_.begin(), last = paths_.end(); it != last;) { auto path = *it; path->frame_count++; @@ -37,6 +38,8 @@ void VolatilePathTracker::onFrame() { ++it; } } + TRACE_EVENT_INSTANT1("flutter", "VolatilePathTracker::OnFrame", "count", + std::to_string(paths_.size()).c_str()); } } // namespace flutter diff --git a/lib/ui/volatile_path_tracker.h b/lib/ui/volatile_path_tracker.h index f44f69536314e..cc47a965f8f3f 100644 --- a/lib/ui/volatile_path_tracker.h +++ b/lib/ui/volatile_path_tracker.h @@ -14,6 +14,13 @@ 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: struct Path { @@ -28,12 +35,12 @@ class VolatilePathTracker { // Starts tracking a path. // Must be called from the UI task runner. - void insert(std::shared_ptr path); + void Insert(std::shared_ptr path); // Removes a path from tracking. // // May be called from any thread. - void erase(std::shared_ptr path); + void Erase(std::shared_ptr path); // Called by the shell at the end of a frame after notifying Dart about idle // time. @@ -42,7 +49,7 @@ class VolatilePathTracker { // survived the |number_of_frames_until_non_volatile|. // // Must be called from the UI task runner. - void onFrame(); + void OnFrame(); private: fml::RefPtr ui_task_runner_; diff --git a/runtime/runtime_controller.h b/runtime/runtime_controller.h index 831a1a4d66ce7..9eef9ada95340 100644 --- a/runtime/runtime_controller.h +++ b/runtime/runtime_controller.h @@ -111,7 +111,7 @@ class RuntimeController : public PlatformConfigurationClient { /// data that the root isolate can /// access in a synchronous manner. /// @param[in] volatile_path_tracker Cache for tracking path - /// volatility. + /// volatility. /// RuntimeController( RuntimeDelegate& client, diff --git a/shell/common/shell.cc b/shell/common/shell.cc index a0867a38253b7..af2e5c305e9e5 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -1021,7 +1021,7 @@ void Shell::OnAnimatorNotifyIdle(int64_t deadline) { if (engine_) { engine_->NotifyIdle(deadline); - volatile_path_tracker_->onFrame(); + volatile_path_tracker_->OnFrame(); } } From 07d2d990afc75011192f7e0960a4e328ec25f1b0 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Mon, 30 Nov 2020 10:12:55 -0800 Subject: [PATCH 07/18] test --- ci/licenses_golden/licenses_flutter | 1 + lib/ui/BUILD.gn | 1 + lib/ui/fixtures/ui_test.dart | 18 ++++- lib/ui/painting/path_unittests.cc | 120 ++++++++++++++++++++++++++++ 4 files changed, 139 insertions(+), 1 deletion(-) create mode 100644 lib/ui/painting/path_unittests.cc diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index 97433821ac1a5..53f4815710b23 100755 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -361,6 +361,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 diff --git a/lib/ui/BUILD.gn b/lib/ui/BUILD.gn index f4eeec64e60a2..fa31f9fe5575e 100644 --- a/lib/ui/BUILD.gn +++ b/lib/ui/BUILD.gn @@ -190,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..b7698329bc9ee 100644 --- a/lib/ui/fixtures/ui_test.dart +++ b/lib/ui/fixtures/ui_test.dart @@ -35,9 +35,25 @@ void createVertices() { ); _validateVertices(vertices); } - void _validateVertices(Vertices vertices) native 'ValidateVertices'; +@pragma('vm:entry-point') +void createRetainedPath() { + 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); + }); +} +@pragma('vm:entry-point') +void createCollectablePath() { + final Path path = Path()..lineTo(10, 10); + _validatePath(path); +} +void _validatePath(Path path) native 'ValidatePath'; + @pragma('vm:entry-point') void frameCallback(FrameInfo info) { print('called back'); diff --git a/lib/ui/painting/path_unittests.cc b/lib/ui/painting/path_unittests.cc new file mode 100644 index 0000000000000..3ae9215fe71f8 --- /dev/null +++ b/lib/ui/painting/path_unittests.cc @@ -0,0 +1,120 @@ +// 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, PathVolatilityTracking) { + auto message_latch = std::make_shared(); + + CanvasPath* path; + auto native_validate_path = [message_latch, + &path](Dart_NativeArguments args) { + auto handle = Dart_GetNativeArgument(args, 0); + intptr_t peer = 0; + Dart_Handle result = Dart_GetNativeInstanceField( + handle, tonic::DartWrappable::kPeerIndex, &peer); + ASSERT_FALSE(Dart_IsError(result)); + path = reinterpret_cast(peer); + ASSERT_TRUE(path); + ASSERT_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("createRetainedPath"); + PlatformViewNotifyCreated(shell.get()); + + shell->RunEngine(std::move(configuration), [](auto result) { + ASSERT_EQ(result, Engine::RunStatus::Success); + }); + + message_latch->Wait(); + ASSERT_TRUE(path); + PumpOneFrame(shell.get()); + ASSERT_TRUE(path->path().isVolatile()); + PumpOneFrame(shell.get()); + ASSERT_FALSE(path->path().isVolatile()); + + DestroyShell(std::move(shell), std::move(task_runners)); +} + +TEST_F(ShellTest, PathVolatilityTrackingCollected) { + auto message_latch = std::make_shared(); + + CanvasPath* path; + auto native_validate_path = [message_latch, + &path](Dart_NativeArguments args) { + auto handle = Dart_GetNativeArgument(args, 0); + intptr_t peer = 0; + Dart_Handle result = Dart_GetNativeInstanceField( + handle, tonic::DartWrappable::kPeerIndex, &peer); + ASSERT_FALSE(Dart_IsError(result)); + path = reinterpret_cast(peer); + ASSERT_TRUE(path); + ASSERT_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("createCollectablePath"); + PlatformViewNotifyCreated(shell.get()); + + shell->RunEngine(std::move(configuration), [](auto result) { + ASSERT_EQ(result, Engine::RunStatus::Success); + }); + + message_latch->Wait(); + ASSERT_TRUE(path); + PumpOneFrame(shell.get()); + ASSERT_TRUE(path->path().isVolatile()); + PumpOneFrame(shell.get()); + // Because the path got GC'd, it was removed from the cache and we're the + // only one holding it. + ASSERT_TRUE(path->path().isVolatile()); + + DestroyShell(std::move(shell), std::move(task_runners)); +} + +} // namespace testing +} // namespace flutter From 88d4125e8426795453c525cddfa9ec3b65a808f2 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Tue, 1 Dec 2020 12:39:04 -0800 Subject: [PATCH 08/18] benchmark and improvement --- lib/ui/fixtures/ui_test.dart | 9 +++ lib/ui/painting/path.cc | 2 +- lib/ui/painting/path.h | 6 +- lib/ui/ui_benchmarks.cc | 123 +++++++++++++++++++++++--------- lib/ui/volatile_path_tracker.cc | 36 ++++++++-- lib/ui/volatile_path_tracker.h | 14 +++- testing/dart_isolate_runner.cc | 10 +-- testing/dart_isolate_runner.h | 21 +++--- 8 files changed, 161 insertions(+), 60 deletions(-) diff --git a/lib/ui/fixtures/ui_test.dart b/lib/ui/fixtures/ui_test.dart index b7698329bc9ee..8aac5126621fe 100644 --- a/lib/ui/fixtures/ui_test.dart +++ b/lib/ui/fixtures/ui_test.dart @@ -148,3 +148,12 @@ Future pumpImage() async { } void _captureImageAndPicture(Image image, Picture picture) native 'CaptureImageAndPicture'; Future _onBeginFrameDone() native 'OnBeginFrameDone'; + +@pragma('vm:entry-point') +void create100CollectablePaths() { + const int numberOfPaths = 100; + final List paths = List.generate(numberOfPaths, (int index) { + return Path()..lineTo(index.toDouble(), index.toDouble() * 2); + }); + assert(paths.length == numberOfPaths); +} diff --git a/lib/ui/painting/path.cc b/lib/ui/painting/path.cc index a509e247ea74a..d9dc5bbd1d4bd 100644 --- a/lib/ui/painting/path.cc +++ b/lib/ui/painting/path.cc @@ -344,7 +344,7 @@ tonic::Float32List CanvasPath::getBounds() { bool CanvasPath::op(CanvasPath* path1, CanvasPath* path2, int operation) { return Op(path1->path(), path2->path(), static_cast(operation), - &tracked_path_->path_); + &tracked_path_->path); resetVolatility(); } diff --git a/lib/ui/painting/path.h b/lib/ui/painting/path.h index e4481824eff4f..0c0d2bb7d8123 100644 --- a/lib/ui/painting/path.h +++ b/lib/ui/painting/path.h @@ -37,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->tracked_path_->path_ = src; + path->tracked_path_->path = src; return path; } @@ -109,7 +109,7 @@ class CanvasPath : public RefCountedDartWrappable { bool op(CanvasPath* path1, CanvasPath* path2, int operation); void clone(Dart_Handle path_handle); - const SkPath& path() const { return tracked_path_->path_; } + const SkPath& path() const { return tracked_path_->path; } size_t GetAllocationSize() const override; @@ -126,7 +126,7 @@ class CanvasPath : public RefCountedDartWrappable { // Must be called whenever the path is created or mutated. void resetVolatility(); - SkPath& mutable_path() { return tracked_path_->path_; } + SkPath& mutable_path() { return tracked_path_->path; } }; } // namespace flutter diff --git a/lib/ui/ui_benchmarks.cc b/lib/ui/ui_benchmarks.cc index 293251400bcd3..b5ff1a1b38870 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,59 @@ 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::GPU | +// 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()); +// Fixture fixture; +// auto settings = fixture.CreateSettingsForFixture(); +// auto vm_ref = DartVMRef::Create(settings); +// auto isolate = +// testing::RunDartCodeInIsolate(vm_ref, settings, task_runners, "main", +// {}, +// testing::GetFixturesPath(), {}); + +// while (state.KeepRunning()) { +// state.PauseTiming(); +// bool successful = isolate->RunInIsolateScope([&]() -> bool { +// // Simulate a message of 3 MB +// std::vector data(3 << 20, 0); +// std::unique_ptr mapping = +// std::make_unique(data); + +// Dart_Handle library = Dart_RootLibrary(); +// Dart_Handle closure = +// Dart_GetField(library, +// Dart_NewStringFromCString("messageCallback")); + +// auto message = fml::MakeRefCounted( +// tonic::DartPersistentValue(isolate->get(), closure), +// thread_host.ui_thread->GetTaskRunner()); + +// message->Complete(std::move(mapping)); + +// return true; +// }); +// FML_CHECK(successful); +// state.ResumeTiming(); + +// // We skip timing everything above because the copy triggered by +// // message->Complete is a task posted on the UI thread. The following +// wait +// // for a UI task would let us know when that copy is done. +// std::promise completed; +// task_runners.GetUITaskRunner()->PostTask( +// [&completed] { completed.set_value(true); }); +// completed.get_future().wait(); +// } +// } + +static void BM_PathVolatilityTracker(benchmark::State& state) { ThreadHost thread_host("test", ThreadHost::Type::Platform | ThreadHost::Type::GPU | ThreadHost::Type::IO | ThreadHost::Type::UI); @@ -27,47 +79,48 @@ static void BM_PlatformMessageResponseDartComplete( thread_host.raster_thread->GetTaskRunner(), thread_host.ui_thread->GetTaskRunner(), thread_host.io_thread->GetTaskRunner()); - Fixture fixture; - auto settings = fixture.CreateSettingsForFixture(); - auto vm_ref = DartVMRef::Create(settings); - auto isolate = - testing::RunDartCodeInIsolate(vm_ref, settings, task_runners, "main", {}, - testing::GetFixturesPath(), {}); + + VolatilePathTracker tracker(task_runners.GetUITaskRunner()); while (state.KeepRunning()) { - state.PauseTiming(); - bool successful = isolate->RunInIsolateScope([&]() -> bool { - // Simulate a message of 3 MB - std::vector data(3 << 20, 0); - std::unique_ptr mapping = - std::make_unique(data); + 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)); + } - Dart_Handle library = Dart_RootLibrary(); - Dart_Handle closure = - Dart_GetField(library, Dart_NewStringFromCString("messageCallback")); + fml::AutoResetWaitableEvent latch; + task_runners.GetUITaskRunner()->PostTask([&]() { + for (auto path : paths) { + tracker.Insert(path); + } + latch.Signal(); + }); - auto message = fml::MakeRefCounted( - tonic::DartPersistentValue(isolate->get(), closure), - thread_host.ui_thread->GetTaskRunner()); + latch.Wait(); - message->Complete(std::move(mapping)); + task_runners.GetUITaskRunner()->PostTask([&]() { tracker.OnFrame(); }); - return true; - }); - FML_CHECK(successful); - state.ResumeTiming(); - - // We skip timing everything above because the copy triggered by - // message->Complete is a task posted on the UI thread. The following wait - // for a UI task would let us know when that copy is done. - std::promise completed; - task_runners.GetUITaskRunner()->PostTask( - [&completed] { completed.set_value(true); }); - completed.get_future().wait(); + 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_PlatformMessageResponseDartComplete) +// ->Unit(benchmark::kMicrosecond); + +BENCHMARK(BM_PathVolatilityTracker) + // ->MinTime(10.0) + ->Unit(benchmark::kMillisecond); } // namespace flutter diff --git a/lib/ui/volatile_path_tracker.cc b/lib/ui/volatile_path_tracker.cc index 2809f642b5361..2c8f7c2d2b9c7 100644 --- a/lib/ui/volatile_path_tracker.cc +++ b/lib/ui/volatile_path_tracker.cc @@ -13,25 +13,34 @@ VolatilePathTracker::VolatilePathTracker( void VolatilePathTracker::Insert(std::shared_ptr path) { FML_DCHECK(ui_task_runner_->RunsTasksOnCurrentThread()); FML_DCHECK(path); - FML_DCHECK(path->path_.isVolatile()); + FML_DCHECK(path->path.isVolatile()); paths_.insert(path); } void VolatilePathTracker::Erase(std::shared_ptr path) { FML_DCHECK(path); - fml::TaskRunner::RunNowOrPostTask(ui_task_runner_, - [&]() { paths_.erase(path); }); + if (ui_task_runner_->RunsTasksOnCurrentThread()) { + paths_.erase(path); + return; + } + + needs_drain_ = true; + std::scoped_lock lock(paths_to_remove_mutex_); + paths_to_remove_.push_back(path); } void VolatilePathTracker::OnFrame() { FML_DCHECK(ui_task_runner_->RunsTasksOnCurrentThread()); TRACE_EVENT1("flutter", "VolatilePathTracker::OnFrame", "count", std::to_string(paths_.size()).c_str()); + + Drain(); + for (auto it = paths_.begin(), last = paths_.end(); it != last;) { auto path = *it; path->frame_count++; - if (path->frame_count >= number_of_frames_until_non_volatile) { - path->path_.setIsVolatile(false); + if (path->frame_count >= kFramesOfVolatility) { + path->path.setIsVolatile(false); path->tracking_volatility = false; it = paths_.erase(it); } else { @@ -42,4 +51,21 @@ void VolatilePathTracker::OnFrame() { std::to_string(paths_.size()).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_); + } + TRACE_EVENT_INSTANT1("flutter", "VolatilePathTracker::Drain", "count", + std::to_string(paths_to_remove.size()).c_str()); + for (auto path : paths_to_remove) { + paths_.erase(path); + } + needs_drain_ = false; + } +} + } // namespace flutter diff --git a/lib/ui/volatile_path_tracker.h b/lib/ui/volatile_path_tracker.h index cc47a965f8f3f..762d2ecf4b1b6 100644 --- a/lib/ui/volatile_path_tracker.h +++ b/lib/ui/volatile_path_tracker.h @@ -5,6 +5,8 @@ #ifndef FLUTTER_LIB_VOLATILE_PATH_TRACKER_H_ #define FLUTTER_LIB_VOLATILE_PATH_TRACKER_H_ +#include +#include #include #include "flutter/fml/macros.h" @@ -26,12 +28,12 @@ class VolatilePathTracker { struct Path { bool tracking_volatility = false; int frame_count = 0; - SkPath path_; + SkPath path; }; explicit VolatilePathTracker(fml::RefPtr ui_task_runner); - static constexpr int number_of_frames_until_non_volatile = 2; + static constexpr int kFramesOfVolatility = 2; // Starts tracking a path. // Must be called from the UI task runner. @@ -46,14 +48,20 @@ class VolatilePathTracker { // time. // // This method will flip the volatility bit to false for any paths that have - // survived the |number_of_frames_until_non_volatile|. + // 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::unordered_set> paths_; + + void Drain(); + FML_DISALLOW_COPY_AND_ASSIGN(VolatilePathTracker); }; diff --git a/testing/dart_isolate_runner.cc b/testing/dart_isolate_runner.cc index d3a7c49baa0dd..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) { @@ -127,7 +128,7 @@ std::unique_ptr RunDartCodeInIsolateOnUITaskRunner( entrypoint, // entrypoint std::nullopt, // library std::move(isolate_configuration), // isolate configuration - nullptr // volatile path tracker + std::move(volatile_path_tracker) // volatile path tracker ) .lock(); @@ -147,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 From 62c238ffc6c608493c3c60e01196f827961d5937 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Tue, 1 Dec 2020 12:44:09 -0800 Subject: [PATCH 09/18] oops --- lib/ui/ui_benchmarks.cc | 106 +++++++++++++++++++--------------------- 1 file changed, 50 insertions(+), 56 deletions(-) diff --git a/lib/ui/ui_benchmarks.cc b/lib/ui/ui_benchmarks.cc index b5ff1a1b38870..f4dd781416b50 100644 --- a/lib/ui/ui_benchmarks.cc +++ b/lib/ui/ui_benchmarks.cc @@ -19,57 +19,53 @@ class Fixture : public testing::FixtureTest { void TestBody() override{}; }; -// static void BM_PlatformMessageResponseDartComplete(benchmark::State& state) { -// ThreadHost thread_host("test", -// ThreadHost::Type::Platform | ThreadHost::Type::GPU | -// 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()); -// Fixture fixture; -// auto settings = fixture.CreateSettingsForFixture(); -// auto vm_ref = DartVMRef::Create(settings); -// auto isolate = -// testing::RunDartCodeInIsolate(vm_ref, settings, task_runners, "main", -// {}, -// testing::GetFixturesPath(), {}); - -// while (state.KeepRunning()) { -// state.PauseTiming(); -// bool successful = isolate->RunInIsolateScope([&]() -> bool { -// // Simulate a message of 3 MB -// std::vector data(3 << 20, 0); -// std::unique_ptr mapping = -// std::make_unique(data); - -// Dart_Handle library = Dart_RootLibrary(); -// Dart_Handle closure = -// Dart_GetField(library, -// Dart_NewStringFromCString("messageCallback")); - -// auto message = fml::MakeRefCounted( -// tonic::DartPersistentValue(isolate->get(), closure), -// thread_host.ui_thread->GetTaskRunner()); - -// message->Complete(std::move(mapping)); - -// return true; -// }); -// FML_CHECK(successful); -// state.ResumeTiming(); - -// // We skip timing everything above because the copy triggered by -// // message->Complete is a task posted on the UI thread. The following -// wait -// // for a UI task would let us know when that copy is done. -// std::promise completed; -// task_runners.GetUITaskRunner()->PostTask( -// [&completed] { completed.set_value(true); }); -// completed.get_future().wait(); -// } -// } +static void BM_PlatformMessageResponseDartComplete(benchmark::State& state) { + ThreadHost thread_host("test", + ThreadHost::Type::Platform | ThreadHost::Type::GPU | + 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()); + Fixture fixture; + auto settings = fixture.CreateSettingsForFixture(); + auto vm_ref = DartVMRef::Create(settings); + auto isolate = + testing::RunDartCodeInIsolate(vm_ref, settings, task_runners, "main", {}, + testing::GetFixturesPath(), {}); + + while (state.KeepRunning()) { + state.PauseTiming(); + bool successful = isolate->RunInIsolateScope([&]() -> bool { + // Simulate a message of 3 MB + std::vector data(3 << 20, 0); + std::unique_ptr mapping = + std::make_unique(data); + + Dart_Handle library = Dart_RootLibrary(); + Dart_Handle closure = + Dart_GetField(library, Dart_NewStringFromCString("messageCallback")); + + auto message = fml::MakeRefCounted( + tonic::DartPersistentValue(isolate->get(), closure), + thread_host.ui_thread->GetTaskRunner()); + + message->Complete(std::move(mapping)); + + return true; + }); + FML_CHECK(successful); + state.ResumeTiming(); + + // We skip timing everything above because the copy triggered by + // message->Complete is a task posted on the UI thread. The following wait + // for a UI task would let us know when that copy is done. + std::promise completed; + task_runners.GetUITaskRunner()->PostTask( + [&completed] { completed.set_value(true); }); + completed.get_future().wait(); + } +} static void BM_PathVolatilityTracker(benchmark::State& state) { ThreadHost thread_host("test", @@ -116,11 +112,9 @@ static void BM_PathVolatilityTracker(benchmark::State& state) { } } -// BENCHMARK(BM_PlatformMessageResponseDartComplete) -// ->Unit(benchmark::kMicrosecond); +BENCHMARK(BM_PlatformMessageResponseDartComplete) + ->Unit(benchmark::kMicrosecond); -BENCHMARK(BM_PathVolatilityTracker) - // ->MinTime(10.0) - ->Unit(benchmark::kMillisecond); +BENCHMARK(BM_PathVolatilityTracker)->Unit(benchmark::kMillisecond); } // namespace flutter From 9fc3504f52844615c95248e94394f49408d84cd8 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Tue, 1 Dec 2020 12:59:52 -0800 Subject: [PATCH 10/18] .. --- runtime/dart_isolate.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runtime/dart_isolate.h b/runtime/dart_isolate.h index 5a0449739066f..5c26140e60979 100644 --- a/runtime/dart_isolate.h +++ b/runtime/dart_isolate.h @@ -434,7 +434,7 @@ class DartIsolate : public UIDartState { std::string advisory_script_uri, std::string advisory_script_entrypoint, bool is_root_isolate, - std::shared_ptr volatile_path_tracker_); + std::shared_ptr volatile_path_tracker); [[nodiscard]] bool Initialize(Dart_Isolate isolate); From c8171adbcdce62cb4cabb61fde9ae9653f635c61 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Tue, 1 Dec 2020 13:34:54 -0800 Subject: [PATCH 11/18] more review --- lib/ui/painting/path.cc | 4 +--- lib/ui/volatile_path_tracker.cc | 4 ++-- testing/dart/path_test.dart | 8 ++++++++ 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/lib/ui/painting/path.cc b/lib/ui/painting/path.cc index d9dc5bbd1d4bd..3e5694a54e020 100644 --- a/lib/ui/painting/path.cc +++ b/lib/ui/painting/path.cc @@ -317,7 +317,7 @@ bool CanvasPath::contains(double x, double y) { void CanvasPath::shift(Dart_Handle path_handle, double dx, double dy) { fml::RefPtr path = CanvasPath::Create(path_handle); - auto other_mutable_path = path->mutable_path(); + auto& other_mutable_path = path->mutable_path(); mutable_path().offset(dx, dy, &other_mutable_path); resetVolatility(); } @@ -333,8 +333,6 @@ void CanvasPath::transform(Dart_Handle path_handle, tonic::Float32List CanvasPath::getBounds() { tonic::Float32List rect(Dart_NewTypedData(Dart_TypedData_kFloat32, 4)); const SkRect& bounds = path().getBounds(); - FML_DLOG(ERROR) << "Bounds " << bounds.left() << " " << bounds.top() << " " - << bounds.right() << " " << bounds.bottom(); rect[0] = bounds.left(); rect[1] = bounds.top(); rect[2] = bounds.right(); diff --git a/lib/ui/volatile_path_tracker.cc b/lib/ui/volatile_path_tracker.cc index 2c8f7c2d2b9c7..7f86dd1e5ab80 100644 --- a/lib/ui/volatile_path_tracker.cc +++ b/lib/ui/volatile_path_tracker.cc @@ -24,8 +24,8 @@ void VolatilePathTracker::Erase(std::shared_ptr path) { return; } - needs_drain_ = true; std::scoped_lock lock(paths_to_remove_mutex_); + needs_drain_ = true; paths_to_remove_.push_back(path); } @@ -58,13 +58,13 @@ void VolatilePathTracker::Drain() { { std::scoped_lock lock(paths_to_remove_mutex_); paths_to_remove.swap(paths_to_remove_); + needs_drain_ = false; } TRACE_EVENT_INSTANT1("flutter", "VolatilePathTracker::Drain", "count", std::to_string(paths_to_remove.size()).c_str()); for (auto path : paths_to_remove) { paths_.erase(path); } - needs_drain_ = false; } } 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); From 9cd0ac92a8de7fe0d3841cc3583b038062bceda6 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Tue, 1 Dec 2020 14:30:43 -0800 Subject: [PATCH 12/18] deflake tests --- lib/ui/fixtures/ui_test.dart | 7 +--- lib/ui/painting/path_unittests.cc | 65 ++++++++++++++++++++----------- 2 files changed, 44 insertions(+), 28 deletions(-) diff --git a/lib/ui/fixtures/ui_test.dart b/lib/ui/fixtures/ui_test.dart index 8aac5126621fe..51aa168db927a 100644 --- a/lib/ui/fixtures/ui_test.dart +++ b/lib/ui/fixtures/ui_test.dart @@ -38,7 +38,7 @@ void createVertices() { void _validateVertices(Vertices vertices) native 'ValidateVertices'; @pragma('vm:entry-point') -void createRetainedPath() { +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 @@ -47,11 +47,6 @@ void createRetainedPath() { path.lineTo(100, 100); }); } -@pragma('vm:entry-point') -void createCollectablePath() { - final Path path = Path()..lineTo(10, 10); - _validatePath(path); -} void _validatePath(Path path) native 'ValidatePath'; @pragma('vm:entry-point') diff --git a/lib/ui/painting/path_unittests.cc b/lib/ui/painting/path_unittests.cc index 3ae9215fe71f8..c5d7180103318 100644 --- a/lib/ui/painting/path_unittests.cc +++ b/lib/ui/painting/path_unittests.cc @@ -20,16 +20,19 @@ TEST_F(ShellTest, PathVolatilityTracking) { auto message_latch = std::make_shared(); CanvasPath* path; - auto native_validate_path = [message_latch, - &path](Dart_NativeArguments args) { + std::shared_ptr tracker; + auto native_validate_path = [message_latch, &path, + &tracker](Dart_NativeArguments args) { auto handle = Dart_GetNativeArgument(args, 0); intptr_t peer = 0; Dart_Handle result = Dart_GetNativeInstanceField( handle, tonic::DartWrappable::kPeerIndex, &peer); - ASSERT_FALSE(Dart_IsError(result)); + EXPECT_FALSE(Dart_IsError(result)); path = reinterpret_cast(peer); - ASSERT_TRUE(path); - ASSERT_TRUE(path->path().isVolatile()); + EXPECT_TRUE(path); + EXPECT_TRUE(path->path().isVolatile()); + tracker = UIDartState::Current()->GetVolatilePathTracker(); + EXPECT_TRUE(tracker); message_latch->Signal(); }; @@ -48,7 +51,7 @@ TEST_F(ShellTest, PathVolatilityTracking) { ASSERT_TRUE(shell->IsSetup()); auto configuration = RunConfiguration::InferFromSettings(settings); - configuration.SetEntrypoint("createRetainedPath"); + configuration.SetEntrypoint("createPath"); PlatformViewNotifyCreated(shell.get()); shell->RunEngine(std::move(configuration), [](auto result) { @@ -57,11 +60,16 @@ TEST_F(ShellTest, PathVolatilityTracking) { message_latch->Wait(); ASSERT_TRUE(path); - PumpOneFrame(shell.get()); - ASSERT_TRUE(path->path().isVolatile()); - PumpOneFrame(shell.get()); - ASSERT_FALSE(path->path().isVolatile()); + message_latch->Reset(); + task_runners.GetUITaskRunner()->PostTask([&tracker, &path, &message_latch]() { + tracker->OnFrame(); + EXPECT_TRUE(path->path().isVolatile()); + tracker->OnFrame(); + EXPECT_FALSE(path->path().isVolatile()); + message_latch->Signal(); + }); + message_latch->Wait(); DestroyShell(std::move(shell), std::move(task_runners)); } @@ -69,16 +77,19 @@ TEST_F(ShellTest, PathVolatilityTrackingCollected) { auto message_latch = std::make_shared(); CanvasPath* path; - auto native_validate_path = [message_latch, - &path](Dart_NativeArguments args) { + std::shared_ptr tracker; + auto native_validate_path = [message_latch, &path, + &tracker](Dart_NativeArguments args) { auto handle = Dart_GetNativeArgument(args, 0); intptr_t peer = 0; Dart_Handle result = Dart_GetNativeInstanceField( handle, tonic::DartWrappable::kPeerIndex, &peer); - ASSERT_FALSE(Dart_IsError(result)); + EXPECT_FALSE(Dart_IsError(result)); path = reinterpret_cast(peer); - ASSERT_TRUE(path); - ASSERT_TRUE(path->path().isVolatile()); + EXPECT_TRUE(path); + EXPECT_TRUE(path->path().isVolatile()); + tracker = UIDartState::Current()->GetVolatilePathTracker(); + EXPECT_TRUE(tracker); message_latch->Signal(); }; @@ -97,7 +108,7 @@ TEST_F(ShellTest, PathVolatilityTrackingCollected) { ASSERT_TRUE(shell->IsSetup()); auto configuration = RunConfiguration::InferFromSettings(settings); - configuration.SetEntrypoint("createCollectablePath"); + configuration.SetEntrypoint("createPath"); PlatformViewNotifyCreated(shell.get()); shell->RunEngine(std::move(configuration), [](auto result) { @@ -106,12 +117,22 @@ TEST_F(ShellTest, PathVolatilityTrackingCollected) { message_latch->Wait(); ASSERT_TRUE(path); - PumpOneFrame(shell.get()); - ASSERT_TRUE(path->path().isVolatile()); - PumpOneFrame(shell.get()); - // Because the path got GC'd, it was removed from the cache and we're the - // only one holding it. - ASSERT_TRUE(path->path().isVolatile()); + + message_latch->Reset(); + task_runners.GetUITaskRunner()->PostTask([&tracker, &path, &message_latch]() { + 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(); + }); + message_latch->Wait(); DestroyShell(std::move(shell), std::move(task_runners)); } From e087e3da4b80eeb958d90efd9190590adcacdb80 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Tue, 1 Dec 2020 14:46:38 -0800 Subject: [PATCH 13/18] fix dangling pointer issue --- lib/ui/volatile_path_tracker.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/ui/volatile_path_tracker.cc b/lib/ui/volatile_path_tracker.cc index 7f86dd1e5ab80..5382d04698058 100644 --- a/lib/ui/volatile_path_tracker.cc +++ b/lib/ui/volatile_path_tracker.cc @@ -47,8 +47,9 @@ void VolatilePathTracker::OnFrame() { ++it; } } + std::string count = std::to_string(paths_.size()); TRACE_EVENT_INSTANT1("flutter", "VolatilePathTracker::OnFrame", "count", - std::to_string(paths_.size()).c_str()); + count.c_str()); } void VolatilePathTracker::Drain() { @@ -60,8 +61,9 @@ void VolatilePathTracker::Drain() { 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", - std::to_string(paths_to_remove.size()).c_str()); + count.c_str()); for (auto path : paths_to_remove) { paths_.erase(path); } From 50dadd30d16f3c2f37765eed91f1a1d39a8beea6 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Tue, 1 Dec 2020 15:27:10 -0800 Subject: [PATCH 14/18] missed one --- lib/ui/volatile_path_tracker.cc | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/ui/volatile_path_tracker.cc b/lib/ui/volatile_path_tracker.cc index 5382d04698058..ea6810dc6c4f3 100644 --- a/lib/ui/volatile_path_tracker.cc +++ b/lib/ui/volatile_path_tracker.cc @@ -31,8 +31,9 @@ void VolatilePathTracker::Erase(std::shared_ptr path) { void VolatilePathTracker::OnFrame() { FML_DCHECK(ui_task_runner_->RunsTasksOnCurrentThread()); + std::string total_count = std::to_string(paths_.size()); TRACE_EVENT1("flutter", "VolatilePathTracker::OnFrame", "count", - std::to_string(paths_.size()).c_str()); + total_count.c_str()); Drain(); @@ -47,9 +48,9 @@ void VolatilePathTracker::OnFrame() { ++it; } } - std::string count = std::to_string(paths_.size()); + std::string post_removal_count = std::to_string(paths_.size()); TRACE_EVENT_INSTANT1("flutter", "VolatilePathTracker::OnFrame", "count", - count.c_str()); + post_removal_count.c_str()); } void VolatilePathTracker::Drain() { From 38281c23cb5145e6eaca2e97202638c61df009f7 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Tue, 1 Dec 2020 16:06:15 -0800 Subject: [PATCH 15/18] NO MORE FLAKES --- lib/ui/painting/path_unittests.cc | 66 +++++++++++++------------------ 1 file changed, 27 insertions(+), 39 deletions(-) diff --git a/lib/ui/painting/path_unittests.cc b/lib/ui/painting/path_unittests.cc index c5d7180103318..c23593f3ccf06 100644 --- a/lib/ui/painting/path_unittests.cc +++ b/lib/ui/painting/path_unittests.cc @@ -19,20 +19,24 @@ namespace testing { TEST_F(ShellTest, PathVolatilityTracking) { auto message_latch = std::make_shared(); - CanvasPath* path; - std::shared_ptr tracker; - auto native_validate_path = [message_latch, &path, - &tracker](Dart_NativeArguments args) { + 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)); - path = reinterpret_cast(peer); + CanvasPath* path = reinterpret_cast(peer); EXPECT_TRUE(path); EXPECT_TRUE(path->path().isVolatile()); - tracker = UIDartState::Current()->GetVolatilePathTracker(); + std::shared_ptr tracker = + UIDartState::Current()->GetVolatilePathTracker(); EXPECT_TRUE(tracker); + + EXPECT_TRUE(path->path().isVolatile()); + tracker->OnFrame(); + EXPECT_TRUE(path->path().isVolatile()); + tracker->OnFrame(); + EXPECT_FALSE(path->path().isVolatile()); message_latch->Signal(); }; @@ -59,37 +63,38 @@ TEST_F(ShellTest, PathVolatilityTracking) { }); message_latch->Wait(); - ASSERT_TRUE(path); - message_latch->Reset(); - task_runners.GetUITaskRunner()->PostTask([&tracker, &path, &message_latch]() { - tracker->OnFrame(); - EXPECT_TRUE(path->path().isVolatile()); - tracker->OnFrame(); - EXPECT_FALSE(path->path().isVolatile()); - message_latch->Signal(); - }); - message_latch->Wait(); DestroyShell(std::move(shell), std::move(task_runners)); } TEST_F(ShellTest, PathVolatilityTrackingCollected) { auto message_latch = std::make_shared(); - CanvasPath* path; - std::shared_ptr tracker; - auto native_validate_path = [message_latch, &path, - &tracker](Dart_NativeArguments args) { + 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)); - path = reinterpret_cast(peer); + CanvasPath* path = reinterpret_cast(peer); EXPECT_TRUE(path); EXPECT_TRUE(path->path().isVolatile()); - tracker = UIDartState::Current()->GetVolatilePathTracker(); + std::shared_ptr tracker = + UIDartState::Current()->GetVolatilePathTracker(); EXPECT_TRUE(tracker); + + 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(); }; @@ -115,23 +120,6 @@ TEST_F(ShellTest, PathVolatilityTrackingCollected) { ASSERT_EQ(result, Engine::RunStatus::Success); }); - message_latch->Wait(); - ASSERT_TRUE(path); - - message_latch->Reset(); - task_runners.GetUITaskRunner()->PostTask([&tracker, &path, &message_latch]() { - 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(); - }); message_latch->Wait(); DestroyShell(std::move(shell), std::move(task_runners)); From 7767698295a7603d28431562ab62d57545375079 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Tue, 1 Dec 2020 16:08:05 -0800 Subject: [PATCH 16/18] unnecessary code --- lib/ui/painting/path_unittests.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/ui/painting/path_unittests.cc b/lib/ui/painting/path_unittests.cc index c23593f3ccf06..89424f3da8406 100644 --- a/lib/ui/painting/path_unittests.cc +++ b/lib/ui/painting/path_unittests.cc @@ -56,7 +56,6 @@ TEST_F(ShellTest, PathVolatilityTracking) { ASSERT_TRUE(shell->IsSetup()); auto configuration = RunConfiguration::InferFromSettings(settings); configuration.SetEntrypoint("createPath"); - PlatformViewNotifyCreated(shell.get()); shell->RunEngine(std::move(configuration), [](auto result) { ASSERT_EQ(result, Engine::RunStatus::Success); @@ -114,7 +113,6 @@ TEST_F(ShellTest, PathVolatilityTrackingCollected) { ASSERT_TRUE(shell->IsSetup()); auto configuration = RunConfiguration::InferFromSettings(settings); configuration.SetEntrypoint("createPath"); - PlatformViewNotifyCreated(shell.get()); shell->RunEngine(std::move(configuration), [](auto result) { ASSERT_EQ(result, Engine::RunStatus::Success); From 847a998c77da85b46929f47acc8e59425be54245 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Wed, 2 Dec 2020 16:28:40 -0800 Subject: [PATCH 17/18] update new test --- runtime/dart_isolate_unittests.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/runtime/dart_isolate_unittests.cc b/runtime/dart_isolate_unittests.cc index 73a95af73a844..cb97e2b74bb09 100644 --- a/runtime/dart_isolate_unittests.cc +++ b/runtime/dart_isolate_unittests.cc @@ -469,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); From 1eac95f026c8f4a506a53e95644a0c60d13a8c5d Mon Sep 17 00:00:00 2001 From: Dan Field Date: Wed, 9 Dec 2020 21:52:21 -0800 Subject: [PATCH 18/18] review/update tests so I can run them with null safety --- lib/ui/fixtures/ui_test.dart | 9 --------- lib/ui/painting/path.cc | 6 ++---- lib/ui/painting/path.h | 2 +- lib/ui/painting/path_unittests.cc | 13 +++++++------ lib/ui/ui_benchmarks.cc | 6 +++--- lib/ui/volatile_path_tracker.cc | 14 +++++++------- lib/ui/volatile_path_tracker.h | 15 +++++++++------ runtime/fixtures/runtime_test.dart | 2 ++ runtime/fixtures/split_lib_test.dart | 2 ++ 9 files changed, 33 insertions(+), 36 deletions(-) diff --git a/lib/ui/fixtures/ui_test.dart b/lib/ui/fixtures/ui_test.dart index 51aa168db927a..9e274e94eb70e 100644 --- a/lib/ui/fixtures/ui_test.dart +++ b/lib/ui/fixtures/ui_test.dart @@ -143,12 +143,3 @@ Future pumpImage() async { } void _captureImageAndPicture(Image image, Picture picture) native 'CaptureImageAndPicture'; Future _onBeginFrameDone() native 'OnBeginFrameDone'; - -@pragma('vm:entry-point') -void create100CollectablePaths() { - const int numberOfPaths = 100; - final List paths = List.generate(numberOfPaths, (int index) { - return Path()..lineTo(index.toDouble(), index.toDouble() * 2); - }); - assert(paths.length == numberOfPaths); -} diff --git a/lib/ui/painting/path.cc b/lib/ui/painting/path.cc index 3e5694a54e020..6cc2b0529d0b1 100644 --- a/lib/ui/painting/path.cc +++ b/lib/ui/painting/path.cc @@ -69,7 +69,7 @@ void CanvasPath::RegisterNatives(tonic::DartLibraryNatives* natives) { CanvasPath::CanvasPath() : path_tracker_(UIDartState::Current()->GetVolatilePathTracker()), - tracked_path_(std::make_shared()) { + tracked_path_(std::make_shared()) { FML_DCHECK(path_tracker_); resetVolatility(); } @@ -87,9 +87,7 @@ void CanvasPath::resetVolatility() { void CanvasPath::ReleaseDartWrappableReference() const { FML_DCHECK(path_tracker_); - if (tracked_path_->tracking_volatility) { - path_tracker_->Erase(tracked_path_); - } + path_tracker_->Erase(tracked_path_); } int CanvasPath::getFillType() { diff --git a/lib/ui/painting/path.h b/lib/ui/painting/path.h index 0c0d2bb7d8123..3c05cdd140837 100644 --- a/lib/ui/painting/path.h +++ b/lib/ui/painting/path.h @@ -121,7 +121,7 @@ class CanvasPath : public RefCountedDartWrappable { CanvasPath(); std::shared_ptr path_tracker_; - std::shared_ptr tracked_path_; + std::shared_ptr tracked_path_; // Must be called whenever the path is created or mutated. void resetVolatility(); diff --git a/lib/ui/painting/path_unittests.cc b/lib/ui/painting/path_unittests.cc index 89424f3da8406..ed5a52500e1a2 100644 --- a/lib/ui/painting/path_unittests.cc +++ b/lib/ui/painting/path_unittests.cc @@ -16,7 +16,7 @@ namespace flutter { namespace testing { -TEST_F(ShellTest, PathVolatilityTracking) { +TEST_F(ShellTest, PathVolatilityOldPathsBecomeNonVolatile) { auto message_latch = std::make_shared(); auto native_validate_path = [message_latch](Dart_NativeArguments args) { @@ -32,10 +32,10 @@ TEST_F(ShellTest, PathVolatilityTracking) { UIDartState::Current()->GetVolatilePathTracker(); EXPECT_TRUE(tracker); - EXPECT_TRUE(path->path().isVolatile()); - tracker->OnFrame(); - EXPECT_TRUE(path->path().isVolatile()); - tracker->OnFrame(); + for (int i = 0; i < VolatilePathTracker::kFramesOfVolatility; i++) { + EXPECT_TRUE(path->path().isVolatile()); + tracker->OnFrame(); + } EXPECT_FALSE(path->path().isVolatile()); message_latch->Signal(); }; @@ -66,7 +66,7 @@ TEST_F(ShellTest, PathVolatilityTracking) { DestroyShell(std::move(shell), std::move(task_runners)); } -TEST_F(ShellTest, PathVolatilityTrackingCollected) { +TEST_F(ShellTest, PathVolatilityGCRemovesPathFromTracker) { auto message_latch = std::make_shared(); auto native_validate_path = [message_latch](Dart_NativeArguments args) { @@ -82,6 +82,7 @@ TEST_F(ShellTest, PathVolatilityTrackingCollected) { UIDartState::Current()->GetVolatilePathTracker(); EXPECT_TRUE(tracker); + static_assert(VolatilePathTracker::kFramesOfVolatility > 1); EXPECT_TRUE(path->path().isVolatile()); tracker->OnFrame(); EXPECT_TRUE(path->path().isVolatile()); diff --git a/lib/ui/ui_benchmarks.cc b/lib/ui/ui_benchmarks.cc index 189d97c57eb0a..16a1ae2a0c158 100644 --- a/lib/ui/ui_benchmarks.cc +++ b/lib/ui/ui_benchmarks.cc @@ -69,7 +69,7 @@ static void BM_PlatformMessageResponseDartComplete(benchmark::State& state) { static void BM_PathVolatilityTracker(benchmark::State& state) { ThreadHost thread_host("test", - ThreadHost::Type::Platform | ThreadHost::Type::GPU | + 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(), @@ -79,10 +79,10 @@ static void BM_PathVolatilityTracker(benchmark::State& state) { VolatilePathTracker tracker(task_runners.GetUITaskRunner()); while (state.KeepRunning()) { - std::vector> paths; + std::vector> paths; constexpr int path_count = 1000; for (int i = 0; i < path_count; i++) { - auto path = std::make_shared(); + auto path = std::make_shared(); path->path = SkPath(); path->path.setIsVolatile(true); paths.push_back(std::move(path)); diff --git a/lib/ui/volatile_path_tracker.cc b/lib/ui/volatile_path_tracker.cc index ea6810dc6c4f3..c69f2fa23747e 100644 --- a/lib/ui/volatile_path_tracker.cc +++ b/lib/ui/volatile_path_tracker.cc @@ -10,14 +10,14 @@ VolatilePathTracker::VolatilePathTracker( fml::RefPtr ui_task_runner) : ui_task_runner_(ui_task_runner) {} -void VolatilePathTracker::Insert(std::shared_ptr path) { +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) { +void VolatilePathTracker::Erase(std::shared_ptr path) { FML_DCHECK(path); if (ui_task_runner_->RunsTasksOnCurrentThread()) { paths_.erase(path); @@ -37,17 +37,17 @@ void VolatilePathTracker::OnFrame() { Drain(); - for (auto it = paths_.begin(), last = paths_.end(); it != last;) { - auto path = *it; + 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; - it = paths_.erase(it); } else { - ++it; + surviving_paths_.insert(path); } } + paths_.swap(surviving_paths_); std::string post_removal_count = std::to_string(paths_.size()); TRACE_EVENT_INSTANT1("flutter", "VolatilePathTracker::OnFrame", "count", post_removal_count.c_str()); @@ -56,7 +56,7 @@ void VolatilePathTracker::OnFrame() { void VolatilePathTracker::Drain() { if (needs_drain_) { TRACE_EVENT0("flutter", "VolatilePathTracker::Drain"); - std::deque> paths_to_remove; + std::deque> paths_to_remove; { std::scoped_lock lock(paths_to_remove_mutex_); paths_to_remove.swap(paths_to_remove_); diff --git a/lib/ui/volatile_path_tracker.h b/lib/ui/volatile_path_tracker.h index 762d2ecf4b1b6..a0ca69983acf1 100644 --- a/lib/ui/volatile_path_tracker.h +++ b/lib/ui/volatile_path_tracker.h @@ -7,7 +7,7 @@ #include #include -#include +#include #include "flutter/fml/macros.h" #include "flutter/fml/task_runner.h" @@ -25,7 +25,8 @@ namespace flutter { /// tracking a path that is no longer referenced in Dart code. class VolatilePathTracker { public: - struct Path { + /// 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; @@ -37,12 +38,14 @@ class VolatilePathTracker { // Starts tracking a path. // Must be called from the UI task runner. - void Insert(std::shared_ptr path); + // + // 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); + void Erase(std::shared_ptr path); // Called by the shell at the end of a frame after notifying Dart about idle // time. @@ -57,8 +60,8 @@ class VolatilePathTracker { fml::RefPtr ui_task_runner_; std::atomic_bool needs_drain_ = false; std::mutex paths_to_remove_mutex_; - std::deque> paths_to_remove_; - std::unordered_set> paths_; + std::deque> paths_to_remove_; + std::set> paths_; void Drain(); 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) {