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

Conversation

hellohuanlin
Copy link
Contributor

@hellohuanlin hellohuanlin commented May 21, 2024

This PR uses CAShapeLayer as the mask to avoid software rendering.

Running benchmark now, will post in comment.

See design doc: https://docs.google.com/document/d/1TqG_N4GK_qctuk73Gk3zOdAiILUrwMqxoCMgroK_AeA/edit?resourcekey=0-jUiidfzIS642ngG2w9vSUA&tab=t.0

List which issues are fixed by this PR. You must list at least one issue.

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

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 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.

CGRectMake(-clipView.frame.origin.x, -clipView.frame.origin.y,
CGRectGetWidth(flutterView.bounds), CGRectGetHeight(flutterView.bounds));
clipView.maskView = [mask_view_pool_.get() getMaskViewWithFrame:frame];
clipView.layer.mask = [mask_view_pool_.get() getMaskViewWithFrame:frame];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is obviously still a draft since I haven't renamed mask_view_pool to mask_layer_pool etc.

@hellohuanlin
Copy link
Contributor Author

hellohuanlin commented May 22, 2024

@jonahwilliams @jmagman So this is still a draft PR, but wanna share the benchmark result first:

═════════════════════════╡ ••• A/B results so far ••• ╞═════════════════════════
Score	Average A (noise)	Average B (noise)	Speed-up
average_frame_build_time_millis		0.27 (4.64%)	
worst_frame_build_time_millis		0.79 (19.35%)	
90th_percentile_frame_build_time_millis		0.44 (6.86%)	
99th_percentile_frame_build_time_millis		0.63 (8.04%)	
average_frame_rasterizer_time_millis		3.59 (2.90%)	
worst_frame_rasterizer_time_millis		12.03 (18.04%)	
90th_percentile_frame_rasterizer_time_millis		6.16 (10.67%)	
99th_percentile_frame_rasterizer_time_millis		9.62 (15.11%)	
average_layer_cache_count		0.00 (0.00%)	
90th_percentile_layer_cache_count		0.00 (0.00%)	
99th_percentile_layer_cache_count		0.00 (0.00%)	
worst_layer_cache_count		0.00 (0.00%)	
average_layer_cache_memory		0.00 (0.00%)	
90th_percentile_layer_cache_memory		0.00 (0.00%)	
99th_percentile_layer_cache_memory		0.00 (0.00%)	
worst_layer_cache_memory		0.00 (0.00%)	
average_picture_cache_count		0.00 (0.00%)	
90th_percentile_picture_cache_count		0.00 (0.00%)	
99th_percentile_picture_cache_count		0.00 (0.00%)	
worst_picture_cache_count		0.00 (0.00%)	
average_picture_cache_memory		0.00 (0.00%)	
90th_percentile_picture_cache_memory		0.00 (0.00%)	
99th_percentile_picture_cache_memory		0.00 (0.00%)	
worst_picture_cache_memory		0.00 (0.00%)	
old_gen_gc_count		0.07 (538.52%)	
average_vsync_transitions_missed		0.58 (88.64%)	
90th_percentile_vsync_transitions_missed		0.60 (92.30%)	
99th_percentile_vsync_transitions_missed		0.70 (133.76%)	
average_frame_request_pending_latency		12736.34 (3.35%)	
90th_percentile_frame_request_pending_latency		15230.33 (2.32%)	
99th_percentile_frame_request_pending_latency		16407.67 (0.59%)	
average_cpu_usage		20.95 (13.81%)	
average_gpu_usage		7.19 (10.50%)	
average_memory_usage		175.05 (3.90%)	
90th_percentile_memory_usage		183.69 (5.62%)	
99th_percentile_memory_usage		189.17 (5.89%)	
total_ui_gc_time		1.43 (28.24%)	
30hz_frame_percentage		0.00 (0.00%)	
60hz_frame_percentage		100.00 (0.00%)	
80hz_frame_percentage		0.00 (0.00%)	
90hz_frame_percentage		0.00 (0.00%)	
120hz_frame_percentage		0.00 (0.00%)	
illegal_refresh_rate_frame_count		0.00 (0.00%)	
average_gpu_frame_time		1.07 (67.73%)	
90th_percentile_gpu_frame_time		0.00 (0.00%)	
99th_percentile_gpu_frame_time		39.58 (76.09%)	
worst_gpu_frame_time		54.17 (39.22%)	
average_gpu_memory_mb		0.00 (0.00%)	
90th_percentile_gpu_memory_mb		0.00 (0.00%)	
99th_percentile_gpu_memory_mb		0.00 (0.00%)	
worst_gpu_memory_mb		0.00 (0.00%)	

Observation
(I reused the benchmark of main branch here)

  • avg: 3.67 -> 3.59 (down 2%)
  • 90th: 6.13 -> 6.16 (up 0.5%)
  • 99th: 10.43 -> 9.62 (down 7.8%)
  • worst: 13.72 -> 12.03 (down 12.3%)

I think we should also ignore 90th/99th/worst due to large noise, and focus on the average frames, as discussed previously.

The result of this PR feels slightly better than clipsToBound, possibly because:

(1) it not only avoids software rendering, but also removes UIView; or
(2) just happen by chance

I think despite that the improvement is small (if any at all, due to noise), we should land it, let it run for a week or 2, and see a more accurate result


@implementation ChildClippingView

- (id<CAAction>)actionForLayer:(CALayer*)layer forKey:(NSString*)event {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cross ref: #51661 (comment)

@jonahwilliams
Copy link
Contributor

We should absolutely land this change, we know that software rasterization is slow - evne if there are other things going on on this benchmark. I would expect an improvement to 99%+ times and not average times, so we may need to run the benchmark on CI for a while to see an improvement there.

@hellohuanlin
Copy link
Contributor Author

I am closing this in favor of #53072, because there are 2 improvements in this PR and I want to separate them:

(1) avoid software rendering (PR linked above);
(2) use CALayer instead of UIView (created new issue to track)

This allows us to (1) measure improvement separately for a better understanding; and also (2) we can land the software rendering PR sooner, since there are quite a lot of testing changes needed to complete this PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants