optimize out unnecessary save restore pairs #34967

JsouLiang · 2022-07-28T12:47:13Z

Add a mechanism for DisplayListBuilder to optimize out unnecessary save/restore pairs.

flar · 2022-07-28T20:44:35Z

I would write test cases along the lines of:

DisplayListBuilder builder1;
... inefficient use of save/restore ...
auto display_list1 = builder1.Build();

DisplayListBuilder builder2;
... same code with optimal save/restore usage ...
auto display_list2 = builder2.Build();

verbose compare dl1 and dl2

flar · 2022-07-28T21:26:30Z

I deleted some earlier comments about how the nested counts would add up and result in multiple extra save records in the stream, but that was before I noticed that the deferred save count was being kept in the layer info.

Still thinking through potential issues this solution might cause, but wanted to indicate why those comments were missing now.

flar · 2022-07-28T21:55:48Z

The failure in the smoke tests is likely due to the new deferred save counts not returning an accurate value. The framework code that calls a CustomPaint's paint method gets a save count before and after the method and expects them to match.

flar · 2022-07-28T22:00:00Z

getSaveCount() returns the number of records in the stack, but deferred saves are not accounted in that number. This will also affect uses of restoreToCount() which will only concern itself that it restores the right number of layer infos rather than undoing the real+deferred saves since the previous state. This might have caused the errors seen in the framework smoke tests. Fixing the value of getSaveCount() to account for deferred saves should also make sure that restoreToCount() functions correctly.

flar · 2022-07-28T22:05:03Z

I wrote the following 2 test cases on top of a local copy of the fix just to verify to myself that these cases work right (and they did). I leave them here in case you want to add them to the list of tests...

Test case 1

TEST(DisplayList, CollapseMultipleNestedSaveRestore) {
  DisplayListBuilder builder1;
  builder1.save();
  builder1.save();
  builder1.save();
  builder1.translate(10, 10);
  builder1.scale(2, 2);
  builder1.clipRect({10, 10, 20, 20}, SkClipOp::kIntersect, false);
  builder1.drawRect({0, 0, 100, 100});
  builder1.restore();
  builder1.restore();
  builder1.restore();
  auto display_list1 = builder1.Build();

  DisplayListBuilder builder2;
  builder2.save();
  builder2.translate(10, 10);
  builder2.scale(2, 2);
  builder2.clipRect({10, 10, 20, 20}, SkClipOp::kIntersect, false);
  builder2.drawRect({0, 0, 100, 100});
  builder2.restore();
  auto display_list2 = builder2.Build();

  ASSERT_TRUE(DisplayListsEQ_Verbose(display_list1, display_list2));
}

Test case 2

TEST(DisplayList, CollapseNestedSaveAndSaveLayerRestore) {
  DisplayListBuilder builder1;
  builder1.save();
  builder1.saveLayer(nullptr, false);
  builder1.translate(10, 10);
  builder1.scale(2, 2);
  builder1.clipRect({10, 10, 20, 20}, SkClipOp::kIntersect, false);
  builder1.drawRect({0, 0, 100, 100});
  builder1.restore();
  builder1.restore();
  auto display_list1 = builder1.Build();

  DisplayListBuilder builder2;
  builder2.saveLayer(nullptr, false);
  builder2.translate(10, 10);
  builder2.scale(2, 2);
  builder2.clipRect({10, 10, 20, 20}, SkClipOp::kIntersect, false);
  builder2.drawRect({0, 0, 100, 100});
  builder2.restore();
  auto display_list2 = builder2.Build();

  ASSERT_TRUE(DisplayListsEQ_Verbose(display_list1, display_list2));
}

flar

There is a known bug discovered by the PR checks where the value we return for getSaveCount() is wrong - I've added some ideas about that in the PR comments.

I believe the changes to dl_utils.h/cc are redundant and should be removed, but let me know if I missed something.

Other than that, just a naming nit and a potential inlining suggestion.

flar · 2022-07-28T22:35:25Z

display_list/display_list_builder.h


  sk_sp<DisplayList> Build();

+  unsigned save_count() const { return save_count_; }


This appears to be a new statistic for the total number of save calls added to the stream. In order to distinguish itself from getSaveCount() it should probably be called either total_save_count_ or save_records_count_?

If this is just for testing, then I think some good tests using the verbose DL comparison testing facility would be more targeted, but I do acknowledge that this adds a quick extra check to all of our existing unit tests. Were any of those additional checks of the "save_count()" informative in terms of noticing optimized save/restores?

display_list/display_list_builder.cc

flar · 2022-07-28T22:38:25Z

display_list/display_list_utils.cc

+  }
+}
+
+void DisplayListBoundsCalculator::save() {


deferred save elimination was already done during construction. Why is it being done here as well? The stream should already be reduced so all of this code in the calculator should be completely redundant. Does it actually encounter any missed deferred save/restores?

This still seems unnecessary and yet it is still here. Why are you checking the save/restores in the Calculator? That operation was already done in the Builder.

flar · 2022-07-28T23:29:42Z

I would like to get @ColdPaleLight to see how this will interact with the PR to collapse the bounds calculations into the constructor.

Also, a perhaps less "disruptive" change would be to switch to always having a layer_stack entry for every save/restore pair regardless of whether it is deferred or not and then just having a boolean in the layer info indicating whether it is deferred. That would mean that the size of the layer stack would remain a good answer for getSaveCount and it could potentially mean that it will fit in better with #34365

Pseudo-code for boolean always-layer-info solution

save() {
  // always emplace a layer_info
  // make sure it is marked as deferred
}
saveLayer() {
  // make sure its layer info is marked as not deferred
}
checkForDeferredSave() {
  if (current_layer_.is_deferred) {
    // Push Save record into stream
    current_layer_.is_deferred = false;
  }
}
restore() {
  if (!current_layer_.is_deferred) {
    // Push Restore record into stream
  }
  // always pop a layer_info
}

ColdPaleLight · 2022-07-29T00:43:01Z

I would like to get @ColdPaleLight to see how this will interact with the PR to collapse the bounds calculations into the constructor.

Also, a perhaps less "disruptive" change would be to switch to always having a layer_stack entry for every save/restore pair regardless of whether it is deferred or not and then just having a boolean in the layer info indicating whether it is deferred. That would mean that the size of the layer stack would remain a good answer for getSaveCount and it could potentially mean that it will fit in better with #34365

Pseudo-code for boolean always-layer-info solution
save() {
  // always emplace a layer_info
  // make sure it is marked as deferred
}
saveLayer() {
  // make sure its layer info is marked as not deferred
}
checkForDeferredSave() {
  if (current_layer_.is_deferred) {
    // Push Save record into stream
    current_layer_.is_deferred = false;
  }
}
restore() {
  if (!current_layer_.is_deferred) {
    // Push Restore record into stream
  }
  // always pop a layer_info
}

I think PR #34365 can be easily adapted whether it is the current implementation by @JsouLiang or the solution mentioned by @flar .

In fact, when @JsouLiang wrote this solution, we discussed how to implement it offline. The current implementation is from SkCanvas, https://github.com/google/skia/blob/b676a02b01727ba6453ae40c7e9ac5201785aba4/src/core/SkCanvas.cpp#L611

However, it should be noted here that since the additional save/restore is not pushed to the stream, this may cause the count of ops may not be equal between Displaylist and SkPicture when draw the same content. Because Skia records all Ops when using SkPictureRecorder. Skia does not optimize unnecessary save/restore when recording Ops, but when actually drawing.

And here we have a test that checks if the op counts of Displaylist and SkPicture are the same.
c.f.

engine/display_list/display_list_canvas_unittests.cc

Line 1861 in 862773f

EXPECT_EQ(static_cast<int>(display_list->op_count()),

flar · 2022-07-29T02:42:37Z

However, it should be noted here that since the additional save/restore is not pushed to the stream, this may cause the count of ops may not be equal between Displaylist and SkPicture when draw the same content. Because Skia records all Ops when using SkPictureRecorder. Skia does not optimize unnecessary save/restore when recording Ops, but when actually drawing.

And here we have a test that checks if the op counts of Displaylist and SkPicture are the same. c.f.

Where do you see Skia recording elided save records? I found the opposite which is why I had to write all of the save tests in display_list_unittests.cc to ensure that the optimization would not apply. Without that code I was getting op count mismatches. Now that DisplayList elides them as well, some of these workarounds may no longer be needed.

c.f.

engine/display_list/display_list_unittests.cc

Line 461 in 862773f

// There are many reasons that save and restore can elide content, including

flar · 2022-07-29T02:49:52Z

Note that SkRecorder records a save record on all calls to willSave(), but willSave() is not called on every call to SkCanvas::save(), it is only called when a deferred save is being realized.

c.f. https://skia.googlesource.com/skia/+/refs/heads/main/src/core/SkRecorder.cpp#326
and https://skia.googlesource.com/skia/+/refs/heads/main/src/core/SkCanvas.cpp#611

JsouLiang · 2022-07-29T03:06:33Z

Note that SkRecorder records a save record on all calls to willSave(), but willSave() is not called on every call to SkCanvas::save(), it is only called when a deferred save is being realized.

c.f. https://skia.googlesource.com/skia/+/refs/heads/main/src/core/SkRecorder.cpp#326 and https://skia.googlesource.com/skia/+/refs/heads/main/src/core/SkCanvas.cpp#611

Yes, the save action only happened when doSave was called.

ColdPaleLight · 2022-07-29T03:13:40Z

Note that SkRecorder records a save record on all calls to willSave(), but willSave() is not called on every call to SkCanvas::save(), it is only called when a deferred save is being realized.

c.f. https://skia.googlesource.com/skia/+/refs/heads/main/src/core/SkRecorder.cpp#326 and https://skia.googlesource.com/skia/+/refs/heads/main/src/core/SkCanvas.cpp#611

Ahh, yes, you are right. The background is that I am trying to solve the issue flutter/flutter#107580 . I've tried in DisplayListBuider to not push those draw operations that don't draw content to the stream, and I then got the ops count mismatches. So I thought save/restore would have the same problem, it doesn't seem to be, which is great.

flar · 2022-07-29T07:56:51Z

display_list/display_list_utils.cc

+  }
+}
+
+void DisplayListBoundsCalculator::save() {


This still seems unnecessary and yet it is still here. Why are you checking the save/restores in the Calculator? That operation was already done in the Builder.

display_list/display_list_builder.cc

flar · 2022-07-29T08:00:35Z

Note that SkRecorder records a save record on all calls to willSave(), but willSave() is not called on every call to SkCanvas::save(), it is only called when a deferred save is being realized.
c.f. https://skia.googlesource.com/skia/+/refs/heads/main/src/core/SkRecorder.cpp#326 and https://skia.googlesource.com/skia/+/refs/heads/main/src/core/SkCanvas.cpp#611

Ahh, yes, you are right. The background is that I am trying to solve the issue flutter/flutter#107580 . I've tried in DisplayListBuider to not push those draw operations that don't draw content to the stream, and I then got the ops count mismatches. So I thought save/restore would have the same problem, it doesn't seem to be, which is great.

Where do you get the op count mismatches? In the unit tests? If the unit tests are pushing NOP operations then the unit tests should be fixed. If they are not pushing NOP operations then there was something wrong with your logic that avoided pushing them.

ColdPaleLight · 2022-07-29T08:53:02Z

Where do you get the op count mismatches? In the unit tests? If the unit tests are pushing NOP operations then the unit tests should be fixed. If they are not pushing NOP operations then there was something wrong with your logic that avoided pushing them.

@flar Yes,in the unit tests. I didn't push NOP at first, so I encountered a count mismatches error. pushing NOP can fix them.

flar · 2022-07-29T18:06:00Z

Where do you get the op count mismatches? In the unit tests? If the unit tests are pushing NOP operations then the unit tests should be fixed. If they are not pushing NOP operations then there was something wrong with your logic that avoided pushing them.

@flar Yes,in the unit tests. I didn't push NOP at first, so I encountered a count mismatches error. pushing NOP can fix them.

Which tests? As far as I know all of our tests are testing non-NOP rendering. If the tests encountered cases where you failed to push an op then either the test is wrong or your code was wrong.

ColdPaleLight · 2022-07-30T03:06:45Z

Which tests? As far as I know all of our tests are testing non-NOP rendering. If the tests encountered cases where you failed to push an op then either the test is wrong or your code was wrong.

@flar please see the draft PR #35024 and the commit f8ad372 (based on PR #34365) for the detail.

flar · 2022-07-31T00:33:49Z

Which tests? As far as I know all of our tests are testing non-NOP rendering. If the tests encountered cases where you failed to push an op then either the test is wrong or your code was wrong.

@flar please see the draft PR #35024 and the commit f8ad372 (based on PR #34365) for the detail.

Ignore previous comments, I discovered a reason why those unit tests had an omitted op. We need to find a better solution that doesn't trigger that condition. I'll follow up with a comment in #35024.

flar

Almost there. Apologies for not double-checking the coverage for deferred save calls.

I suggested a new test for an odd "late save" case that we should check. Also, should we have a quick test for each of the methods that does a deferred check to verify that it does its check? Something small like:

{
  save
  <deferred-trigger-call>
  drawRect
  restore
  ...
  EQ_Verbose(...)
}
// Repeat that for each of:
// - translate
// - scale
// - skew
// - transform2D
// - transformPersp
// - resetTransform
// - clipRect
// - RRect
// - Path

To be even more complete in testing we might want to check things like NOP-transforms don't realize the deferred save, and that methods that forward their operation to a simpler method such as transformPerspective(2D affine matrix) and clipPath(rectangle/oval/rrect path) also trigger a deferred save.

display_list/display_list_builder.cc

display_list/display_list_unittests.cc

display_list/display_list_builder.cc

display_list/display_list_unittests.cc

flar

This is looking ready to go. Please fix the TranslateTriggersDeferredSave test that seems to have strayed from the pattern of the other Triggers tests.

display_list/display_list_unittests.cc

auto-submit · 2022-08-11T06:29:06Z

The status or check suite Mac Host Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

…4967)

* drafting the solution to optimize out unnecessary save restore pairs * remove unnecessary save/restore pairs * delete the calculator change; * fix some logic; Add some testcases * Add test for set DlPaint * update test cases * Prune TranslateTriggersDeferredSave unittest

JsouLiang requested a review from flar July 28, 2022 12:47

JsouLiang mentioned this pull request Jul 28, 2022

DisplayList does not optimize out unnecessary save/restore pairs flutter/flutter#107579

Closed

flar suggested changes Jul 28, 2022

View reviewed changes

JsouLiang requested a review from flar July 29, 2022 07:47

flar suggested changes Jul 29, 2022

View reviewed changes

JsouLiang requested a review from flar July 31, 2022 05:01

flar suggested changes Aug 2, 2022

View reviewed changes

display_list/display_list_builder.cc Show resolved Hide resolved

display_list/display_list_unittests.cc Show resolved Hide resolved

display_list/display_list_builder.cc Outdated Show resolved Hide resolved

display_list/display_list_builder.cc Outdated Show resolved Hide resolved

JsouLiang requested a review from flar August 3, 2022 08:35

flar suggested changes Aug 8, 2022

View reviewed changes

display_list/display_list_unittests.cc Outdated Show resolved Hide resolved

display_list/display_list_unittests.cc Outdated Show resolved Hide resolved

display_list/display_list_unittests.cc Outdated Show resolved Hide resolved

JsouLiang added 5 commits August 9, 2022 10:19

drafting the solution to optimize out unnecessary save restore pairs

d757fd8

remove unnecessary save/restore pairs

f1f376b

delete the calculator change;

0328e2e

fix some logic; Add some testcases

d7d374f

Add test for set DlPaint

04f303f

JsouLiang force-pushed the optimize-out-unnecessary-save-restore-pairs branch from a8f004a to 6fa6ce0 Compare August 9, 2022 03:16

update test cases

d280061

JsouLiang force-pushed the optimize-out-unnecessary-save-restore-pairs branch from 6fa6ce0 to d280061 Compare August 9, 2022 03:20

JsouLiang requested a review from flar August 9, 2022 03:20

flar approved these changes Aug 11, 2022

View reviewed changes

display_list/display_list_unittests.cc Outdated Show resolved Hide resolved

JsouLiang added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 11, 2022

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 11, 2022

Prune TranslateTriggersDeferredSave unittest

085100e

JsouLiang force-pushed the optimize-out-unnecessary-save-restore-pairs branch from 3531af1 to 085100e Compare August 11, 2022 06:33

JsouLiang merged commit ac81750 into flutter:main Aug 11, 2022

engine-flutter-autoroll mentioned this pull request Aug 11, 2022

Roll Flutter Engine from c9a31b7ea4fe to cf8d7c01d66d (10 revisions) flutter/flutter#109394

Merged

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

ac81750 optimize out unnecessary save restore pairs (flutter/engine#3…

d7d0540

…4967)


		sk_sp<DisplayList> Build();

		unsigned save_count() const { return save_count_; }

optimize out unnecessary save restore pairs #34967

optimize out unnecessary save restore pairs #34967

Uh oh!

Conversation

JsouLiang commented Jul 28, 2022

Uh oh!

flar commented Jul 28, 2022

Uh oh!

flar commented Jul 28, 2022

Uh oh!

flar commented Jul 28, 2022

Uh oh!

flar commented Jul 28, 2022

Uh oh!

flar commented Jul 28, 2022

Uh oh!

flar left a comment

Choose a reason for hiding this comment

Uh oh!

flar Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

flar Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

flar Jul 29, 2022

Choose a reason for hiding this comment

Uh oh!

flar commented Jul 28, 2022

Uh oh!

ColdPaleLight commented Jul 29, 2022

Uh oh!

flar commented Jul 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flar commented Jul 29, 2022

Uh oh!

JsouLiang commented Jul 29, 2022

Uh oh!

ColdPaleLight commented Jul 29, 2022

Uh oh!

flar Jul 29, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

flar commented Jul 29, 2022

Uh oh!

ColdPaleLight commented Jul 29, 2022

Uh oh!

flar commented Jul 29, 2022

Uh oh!

ColdPaleLight commented Jul 30, 2022

Uh oh!

flar commented Jul 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

flar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

auto-submit bot commented Aug 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

flar commented Jul 29, 2022 •

edited

Loading

flar commented Jul 31, 2022 •

edited

Loading