Set SkPath::setIsVolatile based on whether the path survives at least two frames #22620

dnfield · 2020-11-19T23:26:40Z

This patch defaults the volatility bit on SkPaths to false, and then flips it to true if the path survives at least two frames.

It does this by maintaining a set of shared_ptr's to a struct containing the SkPath and some bookkeeping information about how many frames they've been around for, and removing paths from that set when we get a call to ReleaseDartWrappableReference, which is called by the finalizer we add for the Dart VM to let us know when we're going to get collected.

This results in substantial memory savings when using a Rive animation, and a slight boost in transition perf overall frame count based on local testing (see numbers in flutter/flutter#58870).

This is a less than ideal solution - it would be better if we had our own displaylist format and could track this information a bit more clearly, and just set the bit on paths whenever they actually got rendered from the displaylist. There are also planned optimizations in Skia that may make this a moot point for many of the types of paths that Rive would typically use, although this could still help in other cases where the new Skia optimization path cannot be followed.

This also avoids the need to expose the flag directly to application developers - it is a relatively confusing flag to use correctly, and it is entirely possible that an application (or the framework) won't know how to set it correctly, e.g. because some application component receives a path from another component and does not know if that same path will get drawn again elsewhere.

/cc @blasten FYI since we were discussing this earlier

/cc @brianosman @bsalomon @reed-at-google - this is a "safe" version of what we were discussing earlier today.

ci/licenses_golden/licenses_flutter

dnfield · 2020-11-21T00:57:08Z

I just realized I don't have any tests checked in for this yet. I'll get to that after thanksgiving.

jason-simmons · 2020-11-21T01:37:17Z

lib/ui/painting/path.cc


 int CanvasPath::getFillType() {
-  return static_cast<int>(path_.getFillType());
+  return static_cast<int>(tracked_path_->path_.getFillType());


Use path() to access the SkPath in these methods. Add a private non-const mutable_path() accessor for methods that modify the path.

jason-simmons · 2020-11-21T01:47:33Z

lib/ui/painting/path.h

-  SkPath path_;
+  static constexpr int number_of_frames_until_non_volatile = 2;
+  static std::mutex volatile_paths_mutex_;
+  static std::unordered_set<std::shared_ptr<TrackedVolatilePath>>


Can this be moved to an object that lives at Shell scope and can be accessed through the UIDataState?
(similar to something like IOManager)

That would have some advantages:

no need for a mutex if this object is only accessed on the UI thread

if multiple engine instances exist, then one engine's NotifyIdle will not affect other engines' paths

paths in the set will be cleaned up when the shell is deleted

That SGTM, but I think we'd still need the mutex because the erase call could come from a Dart VM worker thread.

Although I suppose we could just do a post task for it...

The problem with this will be we don't know if we're in isolate scope or not when a path gets collected.

Ok - I made this more or less work by sharing the task runner and just grabbing out the tracker when we create the path, at wihch point we'll always have isolate scope.

I think this should be correct.

liyuqian · 2020-11-28T23:22:36Z

lib/ui/volatile_path_tracker.h

+
+namespace flutter {
+
+class VolatilePathTracker {


Add documentations for public classes.

liyuqian · 2020-11-28T23:29:04Z

lib/ui/volatile_path_tracker.h

+
+  // Starts tracking a path.
+  // Must be called from the UI task runner.
+  void insert(std::shared_ptr<Path> path);


Flutter engine uses Google C++ naming style https://google.github.io/styleguide/cppguide.html#Function_Names so Insert might be better.

liyuqian · 2020-11-28T23:41:24Z

lib/ui/painting/path.cc

-CanvasPath::~CanvasPath() {}
+void CanvasPath::ReleaseDartWrappableReference() const {
+  FML_DCHECK(path_tracker_);
+  path_tracker_->erase(tracked_path_);


Can we do the following?

if (tracked_path_->tracking_volatility) { path_tracker_->erase(tracked_path_); }

This seems to be able to save some fml::TaskRunner::RunNowOrPostTask calls when the path is non-volatile.

liyuqian · 2020-11-28T23:47:34Z

lib/ui/painting/path.cc

+
+CanvasPath::~CanvasPath() = default;
+
+void CanvasPath::resetVolatility() {


The name resetVolatility sounds like it will also reset the volatility frame count but that does not seem to be the case. I believe resetting the count is the intended behavior as it's called in those methods that mutate the path.

liyuqian · 2020-11-28T23:52:29Z

lib/ui/painting/path.cc

+  auto other_mutable_path = path->mutable_path();
+  mutable_path().transform(ToSkMatrix(matrix4), &other_mutable_path);
  matrix4.Release();
+  resetVolatility();


This method does not seem to mutate the current path. Can we do the following?

path().transform(ToSkMatrix(matrix4), &other_mutable_path); matrix4.Release(); matrix4.Release(); // resetVolatility() is not needed.

Ahh it mutates the new path, got it. Done.

liyuqian · 2020-11-28T23:55:03Z

lib/ui/painting/path.h

                                            const SkPath& src) {
    fml::RefPtr<CanvasPath> path = CanvasPath::Create(path_handle);
-    path->path_ = src;
+    path->tracked_path_->path_ = src;


Nit: path->tracked_path_->path_ -> path->mutable_path()

liyuqian · 2020-11-29T00:00:21Z

lib/ui/volatile_path_tracker.cc

+                                    [&]() { paths_.erase(path); });
+}
+
+void VolatilePathTracker::onFrame() {


It might be nice to add some tracing here. For example, how many paths are tracked, how many survived, and how many are marked as non-volatile.

Added some arguments.

liyuqian · 2020-11-29T00:05:15Z

lib/ui/volatile_path_tracker.cc

+
+void VolatilePathTracker::erase(std::shared_ptr<Path> path) {
+  FML_DCHECK(path);
+  fml::TaskRunner::RunNowOrPostTask(ui_task_runner_,


I wonder if fml::TaskRunner::RunNowOrPostTask's overhead could be significant here. Can we add a benchmark that inserts/erases 10,000 simple paths (e.g., tiny rects) per frame?

This will get called from whatever thread Dart is doing GC on. In local testing, that's working out to be a random worker thread from the VM.

I'm not sure what it is you'd like to benchmark - speed of erasure when a different thread is actually used?

The only other option I can think of for this is to use a mutex to guard the collection, but that should be slower than this method since it would need to acquire the lock in all three methods rather than just jumping the thread in this one.

Another possibility would be a batching mechanism that schedules a deletion task with a delay and deletes all paths that were accumulated during the delay (similar to how SkiaUnrefQueue works).

How many paths are created while rendering an animation? Are most of these paths typically GCed around the same time?

In the rive example app, I'm seeing about 100 paths per frame. It's not entirely clear to me about the timing of the GC.

I'll look at writing a benchmark for this.

I wrote a benchmark.

I went with this strategy:

Erase() now adds paths to a separate collection, guarded by a mutex

OnFrame() calls Drain() before processing paths

Drain() checks whether that collection needs to be drained. If so, it swaps it with an empty collection using the mutex, and then removes those paths from the set of paths we're tracking.

OnFrame goes on to process any remaining paths.

The benchmark is now silly fast for this, whereas before it was relatively slow.

liyuqian · 2020-11-29T00:11:36Z

runtime/runtime_controller.h

  ///                                         data that the root isolate can
  ///                                         access in a synchronous manner.
+  /// @param[in]  volatile_path_tracker       Cache for tracking path
+  /// volatility.


Format problem.

jason-simmons · 2020-12-01T20:38:15Z

runtime/dart_isolate.h

              std::string advisory_script_entrypoint,
-              bool is_root_isolate);
+              bool is_root_isolate,
+              std::shared_ptr<VolatilePathTracker> volatile_path_tracker_);


Remove the trailing underscore on volatile_path_tracker_

jason-simmons · 2020-12-01T21:12:03Z

lib/ui/volatile_path_tracker.cc

+    for (auto path : paths_to_remove) {
+      paths_.erase(path);
+    }
+    needs_drain_ = false;


Do the writes to needs_drain_ while holding the paths_to_remove_mutex_ (this also applies to VolatilePathTracker::Erase)

This will ensure that needs_drain_ is always in sync with the emptiness of paths_to_remove_ and that a needs_drain_ = false write here does not overwrite a true value set by VolatilePathTracker::Erase

dnfield · 2020-12-01T21:20:50Z

This is failing to render the FAB buttin in our hello world app. Looking into why.

dnfield · 2020-12-01T21:21:07Z

Also, the unit test, as written, is flaky in non-debug mode.

dnfield · 2020-12-02T17:31:03Z

LWE failure is flutter/flutter#71581. Rerunning.

chinmaygarde

Other than the thread safety of the tracking_volatility flag and issues around mutation during enumeration, everything else is nits.

chinmaygarde · 2020-12-10T00:27:29Z

lib/ui/painting/path_unittests.cc

+        UIDartState::Current()->GetVolatilePathTracker();
+    EXPECT_TRUE(tracker);
+
+    EXPECT_TRUE(path->path().isVolatile());


Minor Nit: This test is brittle to changes to the value of VolatilePathTracker::kFramesOfVolatility. Maybe perform that many checks for false before the eventual true assertion.

chinmaygarde · 2020-12-10T00:30:27Z

lib/ui/fixtures/ui_test.dart

 Future<void> _onBeginFrameDone() native 'OnBeginFrameDone';
+
+@pragma('vm:entry-point')
+void create100CollectablePaths() {


I'm confused. This seems dead code. Did you forget to check in the test maybe?

I forgot to remove this code when I changed how a test worked.

chinmaygarde · 2020-12-10T00:32:55Z

lib/ui/painting/path_unittests.cc

+namespace flutter {
+namespace testing {
+
+TEST_F(ShellTest, PathVolatilityTracking) {


Feel free to be a bit more verbose with names of test cases. PathVolatilityOldPathsBecomeNonVolatile and PathVolatilityGCRemovesPathFromTracker

chinmaygarde · 2020-12-10T00:36:23Z

lib/ui/volatile_path_tracker.cc

+void VolatilePathTracker::Insert(std::shared_ptr<Path> path) {
+  FML_DCHECK(ui_task_runner_->RunsTasksOnCurrentThread());
+  FML_DCHECK(path);
+  FML_DCHECK(path->path.isVolatile());


Should this component care if the path is already volatile? You could just drop it on the floor and move on. I don't see it documented that the path must be non-volatile.

It just seems like an error to add a path that's already been marked as non-volatile, and makes for slightly more work/memory usage. I can update the doc.

Sounds good.

chinmaygarde · 2020-12-10T00:44:17Z

lib/ui/volatile_path_tracker.cc

+void VolatilePathTracker::OnFrame() {
+  FML_DCHECK(ui_task_runner_->RunsTasksOnCurrentThread());
+  std::string total_count = std::to_string(paths_.size());
+  TRACE_EVENT1("flutter", "VolatilePathTracker::OnFrame", "count",


Nit: The string-ification of trace counts is done automatically for you by the macros. So paths_.size() can go directly in total_count.c_str()

Ditto elsewhere.

I'm missing something then - TraceArg is defined as const char*. paths_.size() is size_type. If I use paths_.size() it's a compile time error.

Ok, I guess that is FML_TRACE_EVENT which are the new and easier to use namespaced variants of these trace macros. These should use the arithmetic specialization of TraceToString. I guess we should get to this TODO.

chinmaygarde · 2020-12-10T00:56:14Z

lib/ui/volatile_path_tracker.cc

+
+  Drain();
+
+  for (auto it = paths_.begin(), last = paths_.end(); it != last;) {


It really easy to introduce bugs because of mutation while enumeration. Also, it's really hard to reason about.

Here, I am still not sure if this is fully safe. First, I believe you are getting away with modifying the path because the unordered set hash function is looking at the pointer to the path. So I guess thats safe but not immediately clear. Second, I am not sure if the iterator returned by erase is the same as that returned by it++ if the erase was not necessary. Thats because unordered set iteration does not guarantee any stable order. So, depending on the implementation, I am not sure if you will end up skipping over some paths in the drain.

I suggest something way simpler:

Iterate over all paths like normal and add the paths to keep in a separate set.

Swap that set with paths_ after.

I believe that also has better algorithmic complexity. You also don't need unordered_set so you'll have better cache locality with std::set. Of course, the perf aspects aside, my main motivation for the suggestion is that it's just easier to read and bulletproof against mutation while enumeration concerns 😄

unordered_set::erase ensures that iterators (other than the erased iterator) remain valid and the iteration order is unchanged (see https://en.cppreference.com/w/cpp/container/unordered_set/erase)

unordered_set iterators are const, so the elements in the set can not be mutated during iteration. It would be clearer if path is explictly declared as a const std::shared_ptr<Path> instead of using auto.

I've updated to be explicit that the shared_ptr is const.

I'm ambivalent here. This should be a pretty safe way to iterate and erase in one pass without additional space overhead. This collection shouldn't typically be super large (in local testing with a rive animation, it hits a little over 100 paths). That few a number should still get helped by cache locality though. I kind of doubt doing it either way is worth the effort of writing a benchmark beyond what we'll already have for transition perf in the devicelab right now.

@chinmaygarde - if you agree this is safe, would you still rather see this refactored?

unordered_set::erase ensures that iterators (other than the erased iterator) remain valid and the iteration order is unchanged (see https://en.cppreference.com/w/cpp/container/unordered_set/erase)

I read that as the validity of existing iterators. But what about the order of iteration? The result of the ++ I mean. Perhaps I am confusing this. But just using a separate set with a swap should be faster and easier to reason about right?

I'll go ahead and refactor to use a set for now.

Jason is correct. Quoting from the linked documentation: "The order of the elements that are not erased is preserved. (This makes it possible to erase individual elements while iterating through the container.)"

chinmaygarde · 2020-12-10T01:29:26Z

lib/ui/volatile_path_tracker.h

+/// tracking a path that is no longer referenced in Dart code.
+class VolatilePathTracker {
+ public:
+  struct Path {


Nit: Suggest naming this TrackedPath. I keep confusing this path and CanvasPath. Your call.

chinmaygarde · 2020-12-10T01:38:57Z

lib/ui/painting/path.cc

+
+void CanvasPath::ReleaseDartWrappableReference() const {
+  FML_DCHECK(path_tracker_);
+  if (tracked_path_->tracking_volatility) {


This can happen on any thread right? For example, the thread on which the GC happens. In that case, isn't there a data race between this and the the setter in resetVolatility? Maybe you are depending on the fact that once the Dart VM releases the reference to the path, we won't be in a situation where we will call reset volatility. But I am not sure that we can guarantee that case since we could still be holding onto the shared pointer on the native side.

Just tracking volatility from the get go in the constructor and not needing the flag (and its unsafe check) should make it so that you don't need the check.

I believe this would be safe if the tracking_volatility flag was made an atomic<bool>.

This relies on the contract that tracking_volatility will only be set to true by resetVolatility, which only happens during Dart->native method calls that run on the UI thread. When ReleaseDartWrappableReference is called, no such method calls can be active because the Dart object is being GCed.

There is a race against VolatilePathTracker::OnFrame which could be running concurrently on the UI thread and might clear tracking_volatility while removing the path from the tracker. However, the worst case is that ReleaseDartWrappableReference does an unnecessaryErase of a path that was already removed. If that happens Erase/Drain will just call unordered_set::erase for that path which will do nothing.

However, this is confusing and makes it easy to introduce errors.

The advantage of the check is avoiding the performance cost of the Erase (which might take a lock) and the later Drain if the GCed path is not tracked.

If the cost is small, another alternative would be to always call Erase in ReleaseDartWrappableReference and avoid reading tracking_volatility on the GC thread. Drain on the UI thread could then safely check tracking_volatility in order to avoid doing the unordered_set::erase if the path is not tracked.

So my thought is that at best, this will save us taking a lock, and at worst we'll get a data race that causes us to insert this into the collection even though the path has been GC'd, and it will be removed anyway.

I guess the risk is that some refactor happens which changes that worst case scenario to something bad.

@chinmaygarde - WDYT about making it an atomic<bool> and documenting this expectation?

no such method calls can be active because the Dart object is being GCed.

I was thinking of calls to CanvasPath::Create from the native side. But I suppose that doesn't happen today. I may be overthinking cases where these objects are created from the native side.

If the cost is small, another alternative would be to always call Erase in ReleaseDartWrappableReference and avoid reading tracking_volatility on the GC thread.

IMO, that would be a lot clearer.

SG, done. We can always come back to this later with a benchmark if it makes a difference.

chinmaygarde · 2020-12-10T01:42:54Z

lib/ui/volatile_path_tracker.h

+/// tracking a path that is no longer referenced in Dart code.
+class VolatilePathTracker {
+ public:
+  struct Path {


I'd add a docstring statement here saying that fields of this struct will only be accessed on the UI task runner (see the note about the access of one of the fields in the ReleaseDartWrappable callback).

I'll do that on resolution of whether using an atomic and documenting it is worthwhile here.

chinmaygarde · 2020-12-10T01:43:17Z

lib/ui/painting/path.cc

+void CanvasPath::resetVolatility() {
+  if (!tracked_path_->tracking_volatility) {
+    mutable_path().setIsVolatile(true);
+    tracked_path_->frame_count = 0;


Seems redundant.

If this path had previously been marked as non-volatile, but now has been mutated and is volatile again, we have to reset the frame count so that we don't immediately mark it volatile if it is only drawn for e.g. 1 frame.

…at least two frames (flutter/engine#22620)

* 3a30ae3 Fix ios voiceover (for safari >13.4) (flutter/engine#22965) * 4338849 Replace g_object_weak_ref with g_object_add_weak_pointer * 3b9937a Load macOS dart bundle by URL fallback (flutter/engine#22979) * 96927bb add ffi_struct_patch.dart to libraries.yaml (flutter/engine#23000) * 2efc7c1 Set SkPath::setIsVolatile based on whether the path survives at least two frames (flutter/engine#22620) * bb81b95 Allow Tile mode for blur filter and add new decal TileMode (flutter/engine#22982) * 9df2157 Load iOS dart bundle by URL fallback (flutter/engine#22997) * 7647fdb Roll Skia from 22f80a60b17f to 6b07e0eb497c (26 revisions) (flutter/engine#23005) * 062cbd8 Freiling warmup memory (flutter/engine#22984) * 1646966 Revert "Freiling warmup memory (#22984)" (flutter/engine#23007) * 50d830a [web] Do not reset 'cursor' in PersistedPlatformView. (flutter/engine#22977) * 6ebf5c3 Roll Dart SDK from e4c9b06267d3 to a4e6fe145bf7 (2 revisions) (flutter/engine#23006) * 14c8c24 [web] Fix regression in foreground style (flutter/engine#22999) * 6678efa Implement SystemSound.play * fb769a4 Roll Fuchsia Linux SDK from rnN_X2o75... to ESzmO-yOF... (flutter/engine#23010) * b424356 Roll Skia from 6b07e0eb497c to f7cce2b243b2 (6 revisions) (flutter/engine#23018) * 56035c7 Roll Fuchsia Linux SDK from ESzmO-yOF... to K4cPd0-Xd... (flutter/engine#23020) * cb4a2ef Roll Skia from f7cce2b243b2 to b0cb8372c1ef (3 revisions) (flutter/engine#23021) * cc8c9d4 Roll Skia from b0cb8372c1ef to 5284e96599a8 (2 revisions) (flutter/engine#23023) * 8e9a943 Roll Dart SDK from a4e6fe145bf7 to c287db6bf232 (2 revisions) (flutter/engine#23024) * 714b543 Roll Fuchsia Mac SDK from OUQEzH1oE... to a9yuHfriB... (flutter/engine#23025) * d50cdda Roll Dart SDK from c287db6bf232 to 2553a84fe438 (1 revision) (flutter/engine#23026) * 4794d04 Roll Skia from 5284e96599a8 to f7fdf1aa2911 (1 revision) (flutter/engine#23027)

…at least two frames (#22620)" This reverts commit 2efc7c1.

…at least two frames (#22620)" (#23044) This reverts commit 2efc7c1.

…at least two frames (flutter/engine#22620)

…urvives at least two frames (flutter#22620)" (flutter#23044)" This reverts commit 4f91425.

…at least two frames (flutter/engine#22620)

* Revert "Revert "Set SkPath::setIsVolatile based on whether the path survives at least two frames (#22620)" (#23044)" This reverts commit 4f91425. * Fix tracing

* Revert "Revert "Set SkPath::setIsVolatile based on whether the path survives at least two frames (flutter#22620)" (flutter#23044)" This reverts commit 4f91425. * Fix tracing

dnfield marked this pull request as draft November 19, 2020 23:26

google-cla bot added the cla: yes label Nov 19, 2020

dnfield mentioned this pull request Nov 19, 2020

Optimize setIsVolatile for Path flutter/flutter#58870

Closed

dnfield commented Nov 19, 2020

View reviewed changes

ci/licenses_golden/licenses_flutter Outdated Show resolved Hide resolved

dnfield added 3 commits November 20, 2020 16:40

does not work yet

bc78eea

safe?

f2867c8

Merge remote-tracking branch 'upstream/master' into path_vol

34e2523

dnfield force-pushed the path_vol branch from 88d3fe4 to 34e2523 Compare November 21, 2020 00:41

one more

100b2ec

dnfield requested review from chinmaygarde, jason-simmons and liyuqian November 21, 2020 00:50

missing mutex

6a84f9e

jason-simmons reviewed Nov 21, 2020

View reviewed changes

UIDartState/shell ownership

24df8c7

dnfield mentioned this pull request Nov 23, 2020

Investigate and identify graphics memory usage, and document our findings flutter/flutter#70658

Closed

liyuqian added perf: memory Performance issues related to memory perf: speed Performance issues related to (mostly rendering) speed severe: performance Relates to speed or footprint issues. labels Nov 29, 2020

liyuqian reviewed Nov 29, 2020

View reviewed changes

dnfield added 2 commits November 30, 2020 09:25

review and fixup errors introduced

280b3c9

test

07d2d99

jason-simmons reviewed Dec 1, 2020

View reviewed changes

dnfield added 3 commits December 1, 2020 12:39

benchmark and improvement

88d4125

oops

62c238f

..

9fc3504

jason-simmons reviewed Dec 1, 2020

View reviewed changes

dnfield added 2 commits December 1, 2020 16:06

NO MORE FLAKES

38281c2

unnecessary code

7767698

dnfield added 3 commits December 2, 2020 13:45

Merge remote-tracking branch 'upstream/master' into path_vol

b1d122e

update new test

847a998

Merge remote-tracking branch 'upstream/master' into path_vol

cfaa5b0

chinmaygarde reviewed Dec 10, 2020

View reviewed changes

dnfield added 2 commits December 9, 2020 20:05

Merge remote-tracking branch 'upstream/master' into path_vol

71153fe

review/update tests so I can run them with null safety

1eac95f

chinmaygarde approved these changes Dec 10, 2020

View reviewed changes

dnfield merged commit 2efc7c1 into flutter:master Dec 10, 2020

engine-flutter-autoroll mentioned this pull request Dec 11, 2020

Roll Engine from 4797b0665242 to 4794d0448f8f (23 revisions) flutter/flutter#72147

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 11, 2020

2efc7c1 Set SkPath::setIsVolatile based on whether the path survives …

a2f7afb

…at least two frames (flutter/engine#22620)

dnfield added a commit that referenced this pull request Dec 11, 2020

Revert "Set SkPath::setIsVolatile based on whether the path survives …

bda74f6

…at least two frames (#22620)" This reverts commit 2efc7c1.

dnfield mentioned this pull request Dec 11, 2020

Revert "Set SkPath::setIsVolatile based on whether the path survives at least two frames" #23044

Merged

dnfield added a commit that referenced this pull request Dec 11, 2020

Revert "Set SkPath::setIsVolatile based on whether the path survives …

4f91425

…at least two frames (#22620)" (#23044) This reverts commit 2efc7c1.

engine-flutter-autoroll mentioned this pull request Dec 14, 2020

Roll Engine from 4797b0665242 to 3ef9b93657cb (34 revisions) flutter/flutter#72283

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 14, 2020

2efc7c1 Set SkPath::setIsVolatile based on whether the path survives …

a76ec2d

…at least two frames (flutter/engine#22620)

engine-flutter-autoroll mentioned this pull request Dec 14, 2020

Roll Engine from 4797b0665242 to 46d7bf9d5aff (39 revisions) flutter/flutter#72296

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 14, 2020

2efc7c1 Set SkPath::setIsVolatile based on whether the path survives …

34140bf

…at least two frames (flutter/engine#22620)

engine-flutter-autoroll mentioned this pull request Dec 14, 2020

Roll Engine from 4797b0665242 to 360a16ad7542 (40 revisions) flutter/flutter#72304

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 14, 2020

2efc7c1 Set SkPath::setIsVolatile based on whether the path survives …

5ec965d

…at least two frames (flutter/engine#22620)

dnfield added a commit to dnfield/engine that referenced this pull request Dec 14, 2020

Revert "Revert "Set SkPath::setIsVolatile based on whether the path s…

ff6697f

…urvives at least two frames (flutter#22620)" (flutter#23044)" This reverts commit 4f91425.

dnfield mentioned this pull request Dec 14, 2020

Reland path volatility tracker #23063

Merged

engine-flutter-autoroll mentioned this pull request Dec 15, 2020

Roll Engine from 4797b0665242 to 1749dbcc1b09 (49 revisions) flutter/flutter#72318

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 15, 2020

2efc7c1 Set SkPath::setIsVolatile based on whether the path survives …

0246139

…at least two frames (flutter/engine#22620)

dnfield added a commit that referenced this pull request Dec 15, 2020

Reland path volatility tracker (#23063)

205d2b8

* Revert "Revert "Set SkPath::setIsVolatile based on whether the path survives at least two frames (#22620)" (#23044)" This reverts commit 4f91425. * Fix tracing


		CanvasPath::~CanvasPath() = default;

		void CanvasPath::resetVolatility() {


		Drain();

		for (auto it = paths_.begin(), last = paths_.end(); it != last;) {


		namespace flutter {

		class VolatilePathTracker {

Set SkPath::setIsVolatile based on whether the path survives at least two frames #22620

Set SkPath::setIsVolatile based on whether the path survives at least two frames #22620

Uh oh!

Conversation

dnfield commented Nov 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

dnfield commented Nov 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield commented Dec 1, 2020

Uh oh!

dnfield commented Dec 1, 2020

Uh oh!

dnfield commented Dec 2, 2020

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

dnfield commented Nov 19, 2020 •

edited

Loading