Skip to content

Conversation

@0xZOne
Copy link
Member

@0xZOne 0xZOne commented Jan 28, 2024

After #100990, we should use initExpensiveAndroidView for Android Hybrid Composition mode instead of initSurfaceAndroidView.

initSurfaceAndroidView attempts to use TLHC when possible. In cases where that is not supported, it falls back to using Hybrid Composition.

flutter/engine#49414

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

…instead of `initSurfaceAndroidView`

`initSurfaceAndroidView` attempts to use the newest and most efficient platform view implementation when possible. In cases where that is not supported, it falls back to using Hybrid Composition, which is the mode used by `initExpensiveAndroidView`.
@0xZOne 0xZOne requested a review from stuartmorgan-g January 28, 2024 14:11
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

LGTM

@0xZOne 0xZOne added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 30, 2024
@auto-submit auto-submit bot merged commit ccb2b44 into flutter:master Jan 30, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 30, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 30, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 30, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 30, 2024
@johnmccutchan
Copy link
Contributor

In the future please include someone from the Flutter Android team on PRs like this.

@johnmccutchan johnmccutchan added the revert Autorevert PR (with "Reason for revert:" comment) label Feb 1, 2024
@auto-submit
Copy link
Contributor

auto-submit bot commented Feb 1, 2024

Time to revert pull request flutter/flutter/142399 has elapsed.
You need to open the revert manually and process as a regular pull request.

@auto-submit auto-submit bot removed the revert Autorevert PR (with "Reason for revert:" comment) label Feb 1, 2024
@johnmccutchan
Copy link
Contributor

@zanderso Based on the benchmark name this benchmark is supposed to be testing the hybrid composition path. It was testing the TLHC path before this PR.

Not sure what we want to do. This change invalidates historical data as it is now measuring something completely different. I do think this change brings the benchmark in line with what it claims to be measuring.

WDYT?

@johnmccutchan
Copy link
Contributor

@zanderso I sent a revert PR your way, feel free to submit it.

@stuartmorgan-g
Copy link
Contributor

Not sure what we want to do. This change invalidates historical data as it is now measuring something completely different. I do think this change brings the benchmark in line with what it claims to be measuring

I don't think we should revert this. I approved the PR because the benchmark had clearly become incorrect when Emmanuel changed what this function did and added a new, different way of getting the old behavior (HC). The benchmark clearly should have been changed at that time, but wasn't, and has therefore been wrong for quite some time.

stuartmorgan-g pushed a commit to flutter/packages that referenced this pull request Feb 1, 2024
flutter/flutter@ace9181...75a2e5b

2024-01-30 [email protected] Reset
framesEnabled to default value at the end of each test
(flutter/flutter#141844)
2024-01-30 [email protected] Fix SliverMainAxisGroup geometry
cacheExtent (flutter/flutter#142482)
2024-01-30 [email protected] Roll Flutter Engine from
0e586d1c28c8 to f02a4a80a77e (3 revisions) (flutter/flutter#142528)
2024-01-30 [email protected] Roll Packages from
516648a to 25abb5d (6 revisions) (flutter/flutter#142527)
2024-01-30 [email protected] Roll Flutter Engine from
438e9b4d7d4e to 0e586d1c28c8 (4 revisions) (flutter/flutter#142515)
2024-01-30 [email protected] Using
`initExpensiveAndroidView` for Android Hybrid Composition Mode
(flutter/flutter#142399)
2024-01-30 [email protected] Roll Flutter Engine from
ed73d40a8c93 to 438e9b4d7d4e (1 revision) (flutter/flutter#142508)
2024-01-30 [email protected] Organize leak tracking TODOs.
(flutter/flutter#142460)
2024-01-30 [email protected] Roll Flutter Engine from
5584a78a439b to ed73d40a8c93 (1 revision) (flutter/flutter#142504)
2024-01-30 [email protected] Roll Flutter Engine from
df5f1afd4991 to 5584a78a439b (2 revisions) (flutter/flutter#142503)
2024-01-30 [email protected] Roll Flutter Engine from
65bf8b1db4d1 to df5f1afd4991 (1 revision) (flutter/flutter#142501)
2024-01-30 [email protected] Roll Flutter Engine from
c9268c7db03c to 65bf8b1db4d1 (1 revision) (flutter/flutter#142496)
2024-01-30 [email protected] Roll Flutter Engine from
e21208583956 to c9268c7db03c (1 revision) (flutter/flutter#142492)
2024-01-29 [email protected] Roll Flutter Engine from
bedafa8794b6 to e21208583956 (1 revision) (flutter/flutter#142483)
2024-01-29 [email protected] Revert "Reland:
"Fix how Gradle resolves Android plugin" (#137115)"
(flutter/flutter#142464)
2024-01-29 [email protected] Marks Mac_pixel_7pro
native_assets_android to be unflaky (flutter/flutter#141675)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected] on the revert to
ensure that a human
is aware of the problem.

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

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@johnmccutchan
Copy link
Contributor

@zanderso Given we've decided to keep this change, how do we reset the benchmark history?

@zanderso
Copy link
Member

zanderso commented Feb 1, 2024

To answer the question, renaming the benchmark is the only way I know of to do that.

Unfortunately, that's kind of a big hammer since the benchmark with the old name will persist in the database and clutter the UI.

If we do nothing here but clear the triage bit, future regressions against the new baseline will still be flagged and come up as needing triage, so that's the path I would recommend.

@0xZOne 0xZOne deleted the task/fix_hc_benchmark branch February 2, 2024 16:42
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants