diff --git a/engine/src/flutter/lib/ui/painting/path.cc b/engine/src/flutter/lib/ui/painting/path.cc index 650aa231f8..223f3058b5 100644 --- a/engine/src/flutter/lib/ui/painting/path.cc +++ b/engine/src/flutter/lib/ui/painting/path.cc @@ -81,16 +81,10 @@ void CanvasPath::resetVolatility() { mutable_path().setIsVolatile(true); tracked_path_->frame_count = 0; tracked_path_->tracking_volatility = true; - path_tracker_->Insert(tracked_path_); + path_tracker_->Track(tracked_path_); } } -void CanvasPath::ReleaseDartWrappableReference() const { - FML_DCHECK(path_tracker_); - path_tracker_->Erase(tracked_path_); - RefCountedDartWrappable::ReleaseDartWrappableReference(); -} - int CanvasPath::getFillType() { return static_cast(path().getFillType()); } diff --git a/engine/src/flutter/lib/ui/painting/path.h b/engine/src/flutter/lib/ui/painting/path.h index 3c05cdd140..bb00442f5f 100644 --- a/engine/src/flutter/lib/ui/painting/path.h +++ b/engine/src/flutter/lib/ui/painting/path.h @@ -115,8 +115,6 @@ class CanvasPath : public RefCountedDartWrappable { static void RegisterNatives(tonic::DartLibraryNatives* natives); - virtual void ReleaseDartWrappableReference() const override; - private: CanvasPath(); diff --git a/engine/src/flutter/lib/ui/painting/path_unittests.cc b/engine/src/flutter/lib/ui/painting/path_unittests.cc index f8b2eabc9f..dec896f9cd 100644 --- a/engine/src/flutter/lib/ui/painting/path_unittests.cc +++ b/engine/src/flutter/lib/ui/painting/path_unittests.cc @@ -67,6 +67,7 @@ TEST_F(ShellTest, PathVolatilityOldPathsBecomeNonVolatile) { } TEST_F(ShellTest, PathVolatilityGCRemovesPathFromTracker) { + static_assert(VolatilePathTracker::kFramesOfVolatility > 1); auto message_latch = std::make_shared(); auto native_validate_path = [message_latch](Dart_NativeArguments args) { @@ -77,26 +78,23 @@ TEST_F(ShellTest, PathVolatilityGCRemovesPathFromTracker) { EXPECT_FALSE(Dart_IsError(result)); CanvasPath* path = reinterpret_cast(peer); EXPECT_TRUE(path); - path->AddRef(); EXPECT_TRUE(path->path().isVolatile()); std::shared_ptr tracker = UIDartState::Current()->GetVolatilePathTracker(); EXPECT_TRUE(tracker); - - static_assert(VolatilePathTracker::kFramesOfVolatility > 1); + EXPECT_EQ(GetLiveTrackedPathCount(tracker), 1ul); EXPECT_TRUE(path->path().isVolatile()); + tracker->OnFrame(); + EXPECT_EQ(GetLiveTrackedPathCount(tracker), 1ul); EXPECT_TRUE(path->path().isVolatile()); // simulate GC - path->ReleaseDartWrappableReference(); + path->Release(); + EXPECT_EQ(GetLiveTrackedPathCount(tracker), 0ul); 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()); - - path->Release(); + EXPECT_EQ(GetLiveTrackedPathCount(tracker), 0ul); message_latch->Signal(); }; diff --git a/engine/src/flutter/lib/ui/ui_benchmarks.cc b/engine/src/flutter/lib/ui/ui_benchmarks.cc index e659b30b94..a88877dac7 100644 --- a/engine/src/flutter/lib/ui/ui_benchmarks.cc +++ b/engine/src/flutter/lib/ui/ui_benchmarks.cc @@ -91,7 +91,7 @@ static void BM_PathVolatilityTracker(benchmark::State& state) { fml::AutoResetWaitableEvent latch; task_runners.GetUITaskRunner()->PostTask([&]() { for (auto path : paths) { - tracker.Insert(path); + tracker.Track(path); } latch.Signal(); }); @@ -101,7 +101,7 @@ static void BM_PathVolatilityTracker(benchmark::State& state) { task_runners.GetUITaskRunner()->PostTask([&]() { tracker.OnFrame(); }); for (int i = 0; i < path_count - 10; ++i) { - tracker.Erase(paths[i]); + paths[i].reset(); } task_runners.GetUITaskRunner()->PostTask([&]() { tracker.OnFrame(); }); diff --git a/engine/src/flutter/lib/ui/volatile_path_tracker.cc b/engine/src/flutter/lib/ui/volatile_path_tracker.cc index b82012bfa3..9b91ef0fdc 100644 --- a/engine/src/flutter/lib/ui/volatile_path_tracker.cc +++ b/engine/src/flutter/lib/ui/volatile_path_tracker.cc @@ -11,7 +11,7 @@ VolatilePathTracker::VolatilePathTracker( bool enabled) : ui_task_runner_(ui_task_runner), enabled_(enabled) {} -void VolatilePathTracker::Insert(std::shared_ptr path) { +void VolatilePathTracker::Track(std::shared_ptr path) { FML_DCHECK(ui_task_runner_->RunsTasksOnCurrentThread()); FML_DCHECK(path); FML_DCHECK(path->path.isVolatile()); @@ -19,22 +19,7 @@ void VolatilePathTracker::Insert(std::shared_ptr path) { path->path.setIsVolatile(false); return; } - paths_.insert(path); -} - -void VolatilePathTracker::Erase(std::shared_ptr path) { - if (!enabled_) { - return; - } - FML_DCHECK(path); - if (ui_task_runner_->RunsTasksOnCurrentThread()) { - paths_.erase(path); - return; - } - - std::scoped_lock lock(paths_to_remove_mutex_); - needs_drain_ = true; - paths_to_remove_.push_back(path); + paths_.push_back(path); } void VolatilePathTracker::OnFrame() { @@ -42,44 +27,35 @@ void VolatilePathTracker::OnFrame() { if (!enabled_) { return; } +#if !FLUTTER_RELEASE std::string total_count = std::to_string(paths_.size()); TRACE_EVENT1("flutter", "VolatilePathTracker::OnFrame", "total_count", total_count.c_str()); +#else + TRACE_EVENT0("flutter", "VolatilePathTracker::OnFrame"); +#endif - Drain(); + paths_.erase(std::remove_if(paths_.begin(), paths_.end(), + [](std::weak_ptr weak_path) { + auto path = weak_path.lock(); + if (!path) { + return true; + } + path->frame_count++; + if (path->frame_count >= kFramesOfVolatility) { + path->path.setIsVolatile(false); + path->tracking_volatility = false; + return true; + } + return false; + }), + paths_.end()); - std::set> surviving_paths_; - for (const std::shared_ptr& path : paths_) { - path->frame_count++; - if (path->frame_count >= kFramesOfVolatility) { - path->path.setIsVolatile(false); - path->tracking_volatility = false; - } else { - surviving_paths_.insert(path); - } - } - paths_.swap(surviving_paths_); +#if !FLUTTER_RELEASE std::string post_removal_count = std::to_string(paths_.size()); TRACE_EVENT_INSTANT1("flutter", "VolatilePathTracker::OnFrame", "remaining_count", post_removal_count.c_str()); -} - -void VolatilePathTracker::Drain() { - if (needs_drain_) { - TRACE_EVENT0("flutter", "VolatilePathTracker::Drain"); - std::deque> paths_to_remove; - { - std::scoped_lock lock(paths_to_remove_mutex_); - paths_to_remove.swap(paths_to_remove_); - needs_drain_ = false; - } - std::string count = std::to_string(paths_to_remove.size()); - TRACE_EVENT_INSTANT1("flutter", "VolatilePathTracker::Drain", "count", - count.c_str()); - for (auto& path : paths_to_remove) { - paths_.erase(path); - } - } +#endif } } // namespace flutter diff --git a/engine/src/flutter/lib/ui/volatile_path_tracker.h b/engine/src/flutter/lib/ui/volatile_path_tracker.h index 40f28311a0..32636dc76d 100644 --- a/engine/src/flutter/lib/ui/volatile_path_tracker.h +++ b/engine/src/flutter/lib/ui/volatile_path_tracker.h @@ -6,8 +6,9 @@ #define FLUTTER_LIB_VOLATILE_PATH_TRACKER_H_ #include +#include #include -#include +#include #include "flutter/fml/macros.h" #include "flutter/fml/task_runner.h" @@ -16,6 +17,10 @@ namespace flutter { +namespace testing { +class ShellTest; +} // namespace testing + /// A cache for paths drawn from dart:ui. /// /// Whenever a flutter::CanvasPath is created, it must Insert an entry into @@ -46,12 +51,7 @@ class VolatilePathTracker { // Must be called from the UI task runner. // // Callers should only insert paths that are currently volatile. - void Insert(std::shared_ptr path); - - // Removes a path from tracking. - // - // May be called from any thread. - void Erase(std::shared_ptr path); + void Track(std::shared_ptr path); // Called by the shell at the end of a frame after notifying Dart about idle // time. @@ -66,13 +66,10 @@ class VolatilePathTracker { private: fml::RefPtr ui_task_runner_; - std::atomic_bool needs_drain_ = false; - std::mutex paths_to_remove_mutex_; - std::deque> paths_to_remove_; - std::set> paths_; + std::vector> paths_; bool enabled_ = true; - void Drain(); + friend class testing::ShellTest; FML_DISALLOW_COPY_AND_ASSIGN(VolatilePathTracker); }; diff --git a/engine/src/flutter/shell/common/shell_test.cc b/engine/src/flutter/shell/common/shell_test.cc index a8a8f35ba8..23e3466997 100644 --- a/engine/src/flutter/shell/common/shell_test.cc +++ b/engine/src/flutter/shell/common/shell_test.cc @@ -402,5 +402,14 @@ bool ShellTest::IsAnimatorRunning(Shell* shell) { return running; } +size_t ShellTest::GetLiveTrackedPathCount( + std::shared_ptr tracker) { + return std::count_if( + tracker->paths_.begin(), tracker->paths_.end(), + [](std::weak_ptr path) { + return path.lock(); + }); +} + } // namespace testing } // namespace flutter diff --git a/engine/src/flutter/shell/common/shell_test.h b/engine/src/flutter/shell/common/shell_test.h index 4a41060c75..c5625ea9de 100644 --- a/engine/src/flutter/shell/common/shell_test.h +++ b/engine/src/flutter/shell/common/shell_test.h @@ -15,6 +15,7 @@ #include "flutter/fml/build_config.h" #include "flutter/fml/macros.h" #include "flutter/fml/time/time_point.h" +#include "flutter/lib/ui/volatile_path_tracker.h" #include "flutter/lib/ui/window/platform_message.h" #include "flutter/shell/common/run_configuration.h" #include "flutter/shell/common/shell_test_external_view_embedder.h" @@ -124,6 +125,9 @@ class ShellTest : public FixtureTest { // is unpredictive. static int UnreportedTimingsCount(Shell* shell); + static size_t GetLiveTrackedPathCount( + std::shared_ptr tracker); + private: ThreadHost thread_host_;