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

Enable ColorFilterLayer support RasterCache#31589

Merged
fluttergithubbot merged 6 commits intoflutter:mainfrom
wangying3426:fix_color_filter_raster_cache
Mar 21, 2022
Merged

Enable ColorFilterLayer support RasterCache#31589
fluttergithubbot merged 6 commits intoflutter:mainfrom
wangying3426:fix_color_filter_raster_cache

Conversation

@wangying3426
Copy link
Contributor

@wangying3426 wangying3426 commented Feb 21, 2022 •
edited
Loading

Fix flutter/flutter#98866

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.

@eggfly eggfly requested a review from flar February 22, 2022 05:02
flar
flar reviewed Feb 22, 2022
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.

Is there a benchmark which shows an improvement with this change? Have you tried running the https://github.com/flutter/flutter/blob/48c9bbe18247798d526373be4dcf7a54d8369f9c/.ci.yaml#L1479 or the https://github.com/flutter/flutter/blob/48c9bbe18247798d526373be4dcf7a54d8369f9c/.ci.yaml#L1490 benchmarks with this change?

Another situation to consider is when the CF is changing, but the children are not. ImageFilterLayer takes that case into account.

@wangying3426 wangying3426 marked this pull request as draft February 24, 2022 02:54
@wangying3426
Copy link
Contributor Author

wangying3426 commented Feb 24, 2022

Is there a benchmark which shows an improvement with this change? Have you tried running the https://github.com/flutter/flutter/blob/48c9bbe18247798d526373be4dcf7a54d8369f9c/.ci.yaml#L1479 or the https://github.com/flutter/flutter/blob/48c9bbe18247798d526373be4dcf7a54d8369f9c/.ci.yaml#L1490 benchmarks with this change?

Another situation to consider is when the CF is changing, but the children are not. ImageFilterLayer takes that case into account.

OK, I will add some benchmark data and example code later.

@wangying3426 wangying3426 mentioned this pull request Mar 4, 2022
Merged
8 tasks
@wangying3426
Copy link
Contributor Author

wangying3426 commented Mar 4, 2022 •
edited
Loading

Is there a benchmark which shows an improvement with this change? Have you tried running the https://github.com/flutter/flutter/blob/48c9bbe18247798d526373be4dcf7a54d8369f9c/.ci.yaml#L1479 or the https://github.com/flutter/flutter/blob/48c9bbe18247798d526373be4dcf7a54d8369f9c/.ci.yaml#L1490 benchmarks with this change?

Another situation to consider is when the CF is changing, but the children are not. ImageFilterLayer takes that case into account.

@flar Referred to ImageFilterLayer, I have optimized the CF raster cache code.

For benchmarks, I found that color_filter_and_fade_perf__timeline_summary will not create a ColorFilterLayer, so I add some benchmarks as flutter/flutter#99542 shown.
And the A/B tests data on a Redmi Note 3 device like this:

═════════════════════════╡ ••• Final A/B results ••• ╞══════════════════════════

Score	Average A (noise)	Average B (noise)	Speed-up
average_frame_build_time_millis	2.28 (0.00%)	2.10 (0.00%)	1.09x	
worst_frame_build_time_millis	4.96 (0.00%)	4.78 (0.00%)	1.04x	
90th_percentile_frame_build_time_millis	3.08 (0.00%)	2.76 (0.00%)	1.11x	
99th_percentile_frame_build_time_millis	4.07 (0.00%)	3.82 (0.00%)	1.07x	
average_frame_rasterizer_time_millis	5.12 (0.00%)	4.12 (0.00%)	1.24x	
worst_frame_rasterizer_time_millis	37.35 (0.00%)	7.21 (0.00%)	5.18x	
90th_percentile_frame_rasterizer_time_millis	5.87 (0.00%)	4.59 (0.00%)	1.28x	
99th_percentile_frame_rasterizer_time_millis	7.41 (0.00%)	5.61 (0.00%)	1.32x
average_layer_cache_count	0.00 (0.00%)	1.00 (0.00%)	0.00x	
90th_percentile_layer_cache_count	0.00 (0.00%)	1.00 (0.00%)	0.00x	
99th_percentile_layer_cache_count	0.00 (0.00%)	1.00 (0.00%)	0.00x	
worst_layer_cache_count	0.00 (0.00%)	1.00 (0.00%)	0.00x	
average_layer_cache_memory	0.00 (0.00%)	3.43 (0.00%)	0.00x	
90th_percentile_layer_cache_memory	0.00 (0.00%)	3.43 (0.00%)	0.00x	
99th_percentile_layer_cache_memory	0.00 (0.00%)	3.43 (0.00%)	0.00x	
worst_layer_cache_memory	0.00 (0.00%)	3.43 (0.00%)	0.00x	
average_picture_cache_count	1.00 (0.00%)	0.00 (0.00%)	Infinityx	
90th_percentile_picture_cache_count	1.00 (0.00%)	0.00 (0.00%)	Infinityx	
99th_percentile_picture_cache_count	1.00 (0.00%)	0.00 (0.00%)	Infinityx	
worst_picture_cache_count	1.00 (0.00%)	0.00 (0.00%)	Infinityx	
average_picture_cache_memory	3.43 (0.00%)	0.00 (0.00%)	Infinityx	
90th_percentile_picture_cache_memory	3.43 (0.00%)	0.00 (0.00%)	Infinityx	
99th_percentile_picture_cache_memory	3.43 (0.00%)	0.00 (0.00%)	Infinityx	
worst_picture_cache_memory	3.43 (0.00%)	0.00 (0.00%)	Infinityx	
new_gen_gc_count	10.00 (0.00%)	24.00 (0.00%)	0.42x	
old_gen_gc_count	0.00 (0.00%)	0.00 (0.00%)	NaNx	
average_vsync_transitions_missed	1.67 (0.00%)	0.00 (0.00%)	Infinityx	
90th_percentile_vsync_transitions_missed	2.00 (0.00%)	0.00 (0.00%)	Infinityx	
99th_percentile_vsync_transitions_missed	2.00 (0.00%)	0.00 (0.00%)	Infinityx

we can see that the average_frame_rasterizer_time_millis and 90th_percentile_frame_rasterizer_time_millis reduce about 20%~30%.

@wangying3426 wangying3426 marked this pull request as ready for review March 4, 2022 11:15
@wangying3426 wangying3426 changed the title Color filter layer supports raster cache Enable ColorFilterLayer support RasterCache Mar 15, 2022
@wangying3426 wangying3426 requested a review from flar March 17, 2022 03:08
@wangying3426
Copy link
Contributor Author

wangying3426 commented Mar 20, 2022

Is there a benchmark which shows an improvement with this change? Have you tried running the https://github.com/flutter/flutter/blob/48c9bbe18247798d526373be4dcf7a54d8369f9c/.ci.yaml#L1479 or the https://github.com/flutter/flutter/blob/48c9bbe18247798d526373be4dcf7a54d8369f9c/.ci.yaml#L1490 benchmarks with this change?
Another situation to consider is when the CF is changing, but the children are not. ImageFilterLayer takes that case into account.

@flar Referred to ImageFilterLayer, I have optimized the CF raster cache code.

For benchmarks, I found that color_filter_and_fade_perf__timeline_summary will not create a ColorFilterLayer, so I add some benchmarks as flutter/flutter#99542 shown. And the A/B tests data on a Redmi Note 3 device like this:

═════════════════════════╡ ••• Final A/B results ••• ╞══════════════════════════

Score	Average A (noise)	Average B (noise)	Speed-up
average_frame_build_time_millis	2.28 (0.00%)	2.10 (0.00%)	1.09x	
worst_frame_build_time_millis	4.96 (0.00%)	4.78 (0.00%)	1.04x	
90th_percentile_frame_build_time_millis	3.08 (0.00%)	2.76 (0.00%)	1.11x	
99th_percentile_frame_build_time_millis	4.07 (0.00%)	3.82 (0.00%)	1.07x	
average_frame_rasterizer_time_millis	5.12 (0.00%)	4.12 (0.00%)	1.24x	
worst_frame_rasterizer_time_millis	37.35 (0.00%)	7.21 (0.00%)	5.18x	
90th_percentile_frame_rasterizer_time_millis	5.87 (0.00%)	4.59 (0.00%)	1.28x	
99th_percentile_frame_rasterizer_time_millis	7.41 (0.00%)	5.61 (0.00%)	1.32x
average_layer_cache_count	0.00 (0.00%)	1.00 (0.00%)	0.00x	
90th_percentile_layer_cache_count	0.00 (0.00%)	1.00 (0.00%)	0.00x	
99th_percentile_layer_cache_count	0.00 (0.00%)	1.00 (0.00%)	0.00x	
worst_layer_cache_count	0.00 (0.00%)	1.00 (0.00%)	0.00x	
average_layer_cache_memory	0.00 (0.00%)	3.43 (0.00%)	0.00x	
90th_percentile_layer_cache_memory	0.00 (0.00%)	3.43 (0.00%)	0.00x	
99th_percentile_layer_cache_memory	0.00 (0.00%)	3.43 (0.00%)	0.00x	
worst_layer_cache_memory	0.00 (0.00%)	3.43 (0.00%)	0.00x	
average_picture_cache_count	1.00 (0.00%)	0.00 (0.00%)	Infinityx	
90th_percentile_picture_cache_count	1.00 (0.00%)	0.00 (0.00%)	Infinityx	
99th_percentile_picture_cache_count	1.00 (0.00%)	0.00 (0.00%)	Infinityx	
worst_picture_cache_count	1.00 (0.00%)	0.00 (0.00%)	Infinityx	
average_picture_cache_memory	3.43 (0.00%)	0.00 (0.00%)	Infinityx	
90th_percentile_picture_cache_memory	3.43 (0.00%)	0.00 (0.00%)	Infinityx	
99th_percentile_picture_cache_memory	3.43 (0.00%)	0.00 (0.00%)	Infinityx	
worst_picture_cache_memory	3.43 (0.00%)	0.00 (0.00%)	Infinityx	
new_gen_gc_count	10.00 (0.00%)	24.00 (0.00%)	0.42x	
old_gen_gc_count	0.00 (0.00%)	0.00 (0.00%)	NaNx	
average_vsync_transitions_missed	1.67 (0.00%)	0.00 (0.00%)	Infinityx	
90th_percentile_vsync_transitions_missed	2.00 (0.00%)	0.00 (0.00%)	Infinityx	
99th_percentile_vsync_transitions_missed	2.00 (0.00%)	0.00 (0.00%)	Infinityx

we can see that the average_frame_rasterizer_time_millis and 90th_percentile_frame_rasterizer_time_millis reduce about 20%~30%.

@flar Could you please review this PR again?

flar
flar approved these changes Mar 20, 2022
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, but you have to fix the licenses before it is ready to merge.

@wangying3426 wangying3426 force-pushed the fix_color_filter_raster_cache branch from e6efad5 to e315f1e Compare March 21, 2022 03:09
@eggfly eggfly requested a review from ColdPaleLight March 21, 2022 03:59
ColdPaleLight
ColdPaleLight approved these changes Mar 21, 2022
View reviewed changes
Copy link
Member

@ColdPaleLight ColdPaleLight left a comment

Choose a reason for hiding this comment

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

LGTM

@ColdPaleLight ColdPaleLight added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Mar 21, 2022
@fluttergithubbot fluttergithubbot merged commit e535969 into flutter:main Mar 21, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 21, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 21, 2022
@engine-flutter-autoroll
e535969 Enable ColorFilterLayer support RasterCache (flutter/engine#3…
d8b8f1a 
…1589)
@wangying3426 wangying3426 mentioned this pull request Mar 25, 2022
Merged
8 tasks
@flar flar mentioned this pull request Apr 12, 2022
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@ColdPaleLight ColdPaleLight ColdPaleLight approved these changes

@flar flar flar approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Color filter layer does not support raster cache

4 participants

@wangying3426 @ColdPaleLight @flar @fluttergithubbot