Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[g3 roll] Revert "add non-rendering operation culling to DisplayListBuilder" #42097

Merged
merged 2 commits into from
May 17, 2023
Merged

[g3 roll] Revert "add non-rendering operation culling to DisplayListBuilder" #42097

merged 2 commits into from
May 17, 2023

Conversation

XilaiZhang
Copy link
Contributor

temporary revert of #41463

context: b/283038609

@XilaiZhang XilaiZhang requested review from flar and CaseyHillers May 17, 2023 17:52
@CaseyHillers
Copy link
Contributor

The goldens broken appear to be a divider line is missing, which is a regression.

@XilaiZhang
Copy link
Contributor Author

cc @Jasguerrero
I am rerunning linux unopt. as a follow up from the handoff, once this revert lands in main, if you would like to continue with your current roll, you can do either one of the two following:

  1. roll framework to a more recent commit, which would include the engine commit corresponding to this revert.
  2. if you already have a roll branch created, you can cherry pick this revert into your engine roll branch

@XilaiZhang XilaiZhang added the autosubmit Merge PR when tree becomes green via auto submit App label May 17, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label May 17, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented May 17, 2023

auto label is removed for flutter/engine, pr: 42097, due to - The status or check suite Linux linux_unopt has failed. Please fix the issues identified (or deflake) before re-applying this label.

@Jasguerrero
Copy link
Contributor

linux_unopt is consistently failing in this PR (after 3 runs) could it be a breakage of the revert? @flar

@CaseyHillers
Copy link
Contributor

@XilaiZhang you'll need to run ci/format.sh

ERROR: Found 1 C++/ObjC/Shader file which was formatted incorrectly.
To fix, run:

patch -p0 <<DONE
--- display_list/testing/dl_test_snippets.h	2023-05-17 12:55:52.688205554 -0700
+++ -	2023-05-17 13:02:54.641372540 -0700
@@ -91,7 +91,7 @@

@flar
Copy link
Contributor

flar commented May 17, 2023

I ran into a similar problem with the reland of the original fix where code that was formatted acceptably when the first change went in was suddenly not passing the new formatting rules - in particular the spaces on that one line grabbing the skia image. Running the format command and pushing the 1-line change should enable it to be landed.

flar
flar approved these changes May 17, 2023
View reviewed changes
Copy link
Contributor

@flar flar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once the formatting fix is applied.

@XilaiZhang
Copy link
Contributor Author

umm interesting. I rerun ./ci/format.sh a few times, but it says no formatting problems found

@XilaiZhang
Copy link
Contributor Author

umm if i edit this line manually, the formatting check would reject my change

@XilaiZhang
Copy link
Contributor Author

foregoing command line edits, and using github ui to manully override based on Jesús' insight

@flar
Copy link
Contributor

flar commented May 17, 2023

Is there a difference between the commit that your revert is based on and the commit that the PR is being tested against?

@XilaiZhang
Copy link
Contributor Author

Is there a difference between the commit that your revert is based on and the commit that the PR is being tested against?

umm looking at file history of /display_list/testing/dl_test_snippets.h i don't seem to find a difference.

commit that this revert is based on: umm this revert is a revert for #41463
commit that the PR is being tested against: my understanding is that linux unopt would test the current content included in this pr

@XilaiZhang XilaiZhang added the autosubmit Merge PR when tree becomes green via auto submit App label May 17, 2023
@auto-submit auto-submit bot merged commit a22dd64 into main May 17, 2023
@auto-submit auto-submit bot deleted the revert-41463-DL-builder-detect-nop branch May 17, 2023 23:48
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 18, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 18, 2023
@engine-flutter-autoroll
a22dd6438 [g3 roll] Revert "add non-rendering operation culling to Di…
d790159 
…splayListBuilder" (flutter/engine#42097)
XilaiZhang added a commit to XilaiZhang/engine that referenced this pull request May 18, 2023
@XilaiZhang
[g3 roll] Revert "add non-rendering operation culling to DisplayListB…
5eb653f 
…uilder" (flutter#42097)

temporary revert of flutter#41463

context: b/283038609
@XilaiZhang XilaiZhang mentioned this pull request May 18, 2023
Merged
XilaiZhang added a commit that referenced this pull request May 18, 2023
@XilaiZhang
[g3 roll + cherry pick] Revert "add non-rendering operation culling t…
95bde5c 
…o DisplayListB… (#42122)

…uilder" (#42097)

temporary revert of #41463

context: b/283038609

this revert is cherry picked into release branch, since engine ->
framework autoroller is currently blocked.
zanderso pushed a commit to flutter/flutter that referenced this pull request May 18, 2023
@engine-flutter-autoroll
Roll Flutter Engine from d97037077963 to a22dd6438335 (1 revision) (#…
4245883 
…127071)

flutter/engine@d970370...a22dd64

2023-05-17 xilaizhang@google.com [g3 roll] Revert "add non-rendering
operation culling to DisplayListBuilder" (flutter/engine#42097)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC rmistry@google.com,zra@google.com on the revert to ensure that
a human
is aware of the problem.

To file a bug in Flutter:
https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
CaseyHillers pushed a commit to CaseyHillers/flutter that referenced this pull request May 24, 2023
@engine-flutter-autoroll
Roll Flutter Engine from d97037077963 to a22dd6438335 (1 revision) (f…
f4a687d 
…lutter#127071)

flutter/engine@d970370...a22dd64

2023-05-17 xilaizhang@google.com [g3 roll] Revert "add non-rendering
operation culling to DisplayListBuilder" (flutter/engine#42097)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC rmistry@google.com,zra@google.com on the revert to ensure that
a human
is aware of the problem.

To file a bug in Flutter:
https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@CaseyHillers CaseyHillers CaseyHillers approved these changes

@flar flar flar approved these changes

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@XilaiZhang @CaseyHillers @Jasguerrero @flar