Optmize path volatility tracker (flutter#30299)

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<int>(path().getFillType());
 }

lib/ui/painting/path.h

@@ -115,8 +115,6 @@ class CanvasPath : public RefCountedDartWrappable<CanvasPath> {
 
   static void RegisterNatives(tonic::DartLibraryNatives* natives);
 
-  virtual void ReleaseDartWrappableReference() const override;
-
  private:
   CanvasPath();
 

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<fml::AutoResetWaitableEvent>();
 
   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<CanvasPath*>(peer);
     EXPECT_TRUE(path);
-    path->AddRef();
     EXPECT_TRUE(path->path().isVolatile());
     std::shared_ptr<VolatilePathTracker> 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();
   };

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(); });

lib/ui/volatile_path_tracker.cc

@@ -11,75 +11,51 @@ VolatilePathTracker::VolatilePathTracker(
     bool enabled)
     : ui_task_runner_(ui_task_runner), enabled_(enabled) {}
 
-void VolatilePathTracker::Insert(std::shared_ptr<TrackedPath> path) {
+void VolatilePathTracker::Track(std::shared_ptr<TrackedPath> path) {
   FML_DCHECK(ui_task_runner_->RunsTasksOnCurrentThread());
   FML_DCHECK(path);
   FML_DCHECK(path->path.isVolatile());
   if (!enabled_) {
     path->path.setIsVolatile(false);
     return;
   }
-  paths_.insert(path);
-}
-
-void VolatilePathTracker::Erase(std::shared_ptr<TrackedPath> 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() {
   FML_DCHECK(ui_task_runner_->RunsTasksOnCurrentThread());
   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());
-
-  Drain();
-
-  std::set<std::shared_ptr<TrackedPath>> surviving_paths_;
-  for (const std::shared_ptr<TrackedPath>& 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_);
+#else
+  TRACE_EVENT0("flutter", "VolatilePathTracker::OnFrame");
+#endif
+
+  paths_.erase(std::remove_if(paths_.begin(), paths_.end(),
+                              [](std::weak_ptr<TrackedPath> 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());
+
+#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<std::shared_ptr<TrackedPath>> 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

lib/ui/volatile_path_tracker.h

@@ -6,8 +6,9 @@
 #define FLUTTER_LIB_VOLATILE_PATH_TRACKER_H_
 
 #include <deque>
+#include <memory>
 #include <mutex>
-#include <set>
+#include <vector>
 
 #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<TrackedPath> path);
-
-  // Removes a path from tracking.
-  //
-  // May be called from any thread.
-  void Erase(std::shared_ptr<TrackedPath> path);
+  void Track(std::shared_ptr<TrackedPath> 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<fml::TaskRunner> ui_task_runner_;
-  std::atomic_bool needs_drain_ = false;
-  std::mutex paths_to_remove_mutex_;
-  std::deque<std::shared_ptr<TrackedPath>> paths_to_remove_;
-  std::set<std::shared_ptr<TrackedPath>> paths_;
+  std::vector<std::weak_ptr<TrackedPath>> paths_;
   bool enabled_ = true;
 
-  void Drain();
+  friend class testing::ShellTest;
 
   FML_DISALLOW_COPY_AND_ASSIGN(VolatilePathTracker);
 };

shell/common/shell_test.cc

@@ -402,5 +402,14 @@ bool ShellTest::IsAnimatorRunning(Shell* shell) {
   return running;
 }
 
+size_t ShellTest::GetLiveTrackedPathCount(
+    std::shared_ptr<VolatilePathTracker> tracker) {
+  return std::count_if(
+      tracker->paths_.begin(), tracker->paths_.end(),
+      [](std::weak_ptr<VolatilePathTracker::TrackedPath> path) {
+        return path.lock();
+      });
+}
+
 }  // namespace testing
 }  // namespace 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<VolatilePathTracker> tracker);
+
  private:
   ThreadHost thread_host_;