Enable partial repaint for Android/OpenGL #29591

knopp · 2021-11-08T17:36:48Z

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 and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

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

flutter-dashboard · 2021-11-08T17:36:50Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

zanderso · 2021-11-12T23:00:46Z

Related flutter/flutter#33939

iskakaushik

Overall the direction looks good. Can we add some unit tests to android_context_gl_unittests.cc? Looks like as of 20ec34e we not have the capacity to run these tests on Android host devices.

shell/platform/android/android_context_gl.cc

iskakaushik · 2021-11-30T17:24:38Z

shell/platform/android/android_context_gl.cc

@@ -105,10 +106,119 @@ static bool TeardownContext(EGLDisplay display, EGLContext context) {
  return true;
 }

+class AndroidEGLSurfaceDamage {


any reason this class is forward declared rather than have the definition in android_context_gl.h?

I prefer not having to split it into header/implementation but don't have strong feelings about it, if you prefer the definition in header I can certainly do it.

shell/platform/android/android_context_gl.h

iskakaushik

Did not mean to approve before. I think it's important to have unittests figured out.

chinmaygarde · 2021-12-09T21:13:44Z

@iskakaushik was looking into gathering firm numbers on the effects of this patch in a large-ish application. Is there any progress to made here?

chinmaygarde · 2022-01-06T21:05:41Z

@iskakaushik Were you able to gather the numbers?

iskakaushik · 2022-01-07T20:39:19Z

I’ve been spending the last couple of weeks attempting to get better traces on why DRM wasn’t showing as much performance improvement on Android devices as it was for iOS. I wanted to summarize my findings so far.

In cases where there are transitions, having partial repaint enabled is causing significant performance regression on Android when testing on internal benchmarks.

Looking at Flutter Gallery Animations as an example in depth, I was able to see similar differences in performance, the key differences are:

Metric	With DRM	Without DRM
99th_percentile_frame_build_time_millis	22.891	18.006
99th_percentile_frame_rasterizer_time_millis	31.61	20.967
frame_count	1172	1232

The first theory I had was that maybe ComputeClipRect was the primary cause of this change, but on further examination, it looks like it isn’t the reason, it took < 500 us each frame on avg, but the difference was ~4 ms on avg for rasterization.

Taking a look further at the top causes of this regression, It seems to me that we might be using raster cache more effectively when DRM is not enabled. This is based on the counts in the table below. Also, the RasterCachePopulate seems to be taking more time with DRM enabled. I'm planning on taking a look at it: flutter/flutter#96310

count	mean	std	min	25%	50%	75%	max	name
553	2425.951175	2248.075344	249	1491	1784	2127	17935	RasterCachePopulate_with_drm
572	862.4213287	737.8742595	75	515	662.5	844.25	8657	RasterCachePopulate_without_drm
2240	75.33482143	23.46672769	43	59	69	87	279	RasterCacheResult::draw_with_drm
3282	26.65051798	11.76175858	6	18	24	33	149	RasterCacheResult::draw_without_drm

I’ve gathered more metrics here that I plan on looking at once I figure out the raster cache issue. Most of the metrics are worse than without enabling DRM, but I'm only investigating the ones with significant differences as some of the others might be due to the environment as well.

These benchmarks were run on a Pixel 4 XL device. I ran the Flutter gallery transitions benchmarks. Command for reference is (working dir: flutter/dev/integration_tests/flutter_gallery):

flutter drive --local-engine android_profile --profile --trace-startup -t test_driver/transitions_perf.dart

@knopp, @chinmaygarde would appreciate any thoughts here as well.

iskakaushik

LGTM!

zanderso · 2022-01-20T00:00:56Z

Was the solution to flutter/flutter#96310 disabling on older devices?

iskakaushik · 2022-01-20T00:08:51Z

@zanderso I wasn't able to consistently reproduce the regression on my Pixel 4 XL device, and did not see it on another device I own. I'm wondering if it is due to my setup / environment issues. The current plan is to land this patch and monitor the benchmarks to see if there are any noticeable regressions in the build/raster time or raster cache usage, in which case I plan on reverting the change.

Disabling on older devices is due to glitches caused by their implementations of the required OpenGL extensions. I will file an issue for this.

zanderso · 2022-01-20T00:14:19Z

@gaaclarke this week's sheriff to keep an eye on SkiaPerf when this rolls into the framework.

zanderso · 2022-01-20T22:18:44Z

FYI this landed in flutter/flutter in flutter/flutter#96941. So far no regressions or improvements flagged by SkiaPerf.

fzyzcjy · 2022-02-04T00:29:23Z

Interesting... So do you find out the actual cause why it does not have improvements? (The iOS pr sounds very performant)

knopp · 2022-02-04T00:48:48Z

Interesting... So do you find out the actual cause why it does not have improvements? (The iOS pr sounds very performant)

Partial repaint improves performance in very specific cases, where only small part of screen is being updated (i.e. spinner or a cursor). I don't think there are currently any benchmarks focusing on that.

I did submit a PR for one, but it got a bit stale.

This reverts commit 57a067c.

blasten · 2022-02-23T20:23:44Z

shell/platform/android/android_context_gl.cc

@@ -105,10 +109,135 @@ static bool TeardownContext(EGLDisplay display, EGLContext context) {
  return true;
 }

+class AndroidEGLSurfaceDamage {
+ public:
+  void init(EGLDisplay display, EGLContext context) {


context isn't used below. was the plan to use it in the future?

Not really. I think I just overlooked it.

flutter-dashboard bot added embedder platform-android platform-ios labels Nov 8, 2021

flutter-dashboard bot added the needs tests label Nov 8, 2021

google-cla bot added the cla: yes label Nov 8, 2021

knopp requested a review from iskakaushik November 8, 2021 17:38

CaseyHillers changed the base branch from master to main November 15, 2021 18:12

godofredoc changed the base branch from master to main November 24, 2021 07:08

iskakaushik approved these changes Nov 30, 2021

View reviewed changes

iskakaushik suggested changes Nov 30, 2021

View reviewed changes

knopp force-pushed the partial_update_android branch from 9463f1b to 51d5652 Compare December 5, 2021 14:33

iskakaushik mentioned this pull request Jan 7, 2022

Investigate raster cache usage when partial repaint is enabled flutter/flutter#96310

Closed

knopp force-pushed the partial_update_android branch 3 times, most recently from 6ca4ef0 to 291a519 Compare January 18, 2022 21:25

knopp added 5 commits January 18, 2022 22:45

Enable partial repaint on Android/OpenGL

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

a3620eb

Set FrameBufferInfo::supports_partial_repaint

fa494ce

Support for quad buffering

5a64880

Use std::array instead of vector

719f8ce

Disable partial redraw for older devices

dc74809

knopp force-pushed the partial_update_android branch from 291a519 to dc74809 Compare January 18, 2022 21:45

iskakaushik approved these changes Jan 19, 2022

View reviewed changes

Enable partial repaint on Android 9

6e2b31e

knopp force-pushed the partial_update_android branch from 8a035ef to 6e2b31e Compare January 20, 2022 16:41

knopp merged commit 57a067c into flutter:main Jan 20, 2022

knopp deleted the partial_update_android branch January 20, 2022 17:22

engine-flutter-autoroll mentioned this pull request Jan 20, 2022

Roll Engine from 1fbf5ef0b86e to 588d62123cff (13 revisions) flutter/flutter#96941

Merged

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

57a067c Enable partial repaint for Android/OpenGL (flutter/engine#29591)

faa9128

iskakaushik added a commit that referenced this pull request Feb 11, 2022

Revert "Enable partial repaint for Android/OpenGL (#29591)"

835ba47

This reverts commit 57a067c.

iskakaushik mentioned this pull request Feb 11, 2022

Revert "Enable partial repaint for Android/OpenGL" #31407

Closed

blasten reviewed Feb 23, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable partial repaint for Android/OpenGL #29591

Enable partial repaint for Android/OpenGL #29591

knopp commented Nov 8, 2021

flutter-dashboard bot commented Nov 8, 2021

zanderso commented Nov 12, 2021

iskakaushik left a comment

iskakaushik Nov 30, 2021

knopp Dec 5, 2021 •

edited

Loading

iskakaushik left a comment

chinmaygarde commented Dec 9, 2021

chinmaygarde commented Jan 6, 2022

iskakaushik commented Jan 7, 2022

iskakaushik left a comment

zanderso commented Jan 20, 2022

iskakaushik commented Jan 20, 2022

zanderso commented Jan 20, 2022

zanderso commented Jan 20, 2022

fzyzcjy commented Feb 4, 2022

knopp commented Feb 4, 2022

blasten Feb 23, 2022

knopp Feb 24, 2022

Enable partial repaint for Android/OpenGL #29591

Enable partial repaint for Android/OpenGL #29591

Conversation

knopp commented Nov 8, 2021

Pre-launch Checklist

flutter-dashboard bot commented Nov 8, 2021

zanderso commented Nov 12, 2021

iskakaushik left a comment

Choose a reason for hiding this comment

iskakaushik Nov 30, 2021

Choose a reason for hiding this comment

knopp Dec 5, 2021 • edited Loading

Choose a reason for hiding this comment

iskakaushik left a comment

Choose a reason for hiding this comment

chinmaygarde commented Dec 9, 2021

chinmaygarde commented Jan 6, 2022

iskakaushik commented Jan 7, 2022

iskakaushik left a comment

Choose a reason for hiding this comment

zanderso commented Jan 20, 2022

iskakaushik commented Jan 20, 2022

zanderso commented Jan 20, 2022

zanderso commented Jan 20, 2022

fzyzcjy commented Feb 4, 2022

knopp commented Feb 4, 2022

blasten Feb 23, 2022

Choose a reason for hiding this comment

knopp Feb 24, 2022

Choose a reason for hiding this comment

knopp Dec 5, 2021 •

edited

Loading