-
Notifications
You must be signed in to change notification settings - Fork 6k
[platform_view]fix regression for addSubview when re-ordering #40091
[platform_view]fix regression for addSubview when re-ordering #40091
Conversation
c7bffed to
e920d92
Compare
e920d92 to
bd3011e
Compare
| NSArray* existing_platform_subviews = [flutter_view.subviews | ||
| filteredArrayUsingPredicate:[NSPredicate predicateWithBlock:^BOOL(id object, | ||
| NSDictionary* bindings) { | ||
| return [desired_platform_subviews_set containsObject:object]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BringLayersIntoView is meant to be called after RemoveUnusedLayers();, so I think any view in existing_platform_subviews should already be in desired_platform_subviews_set? Did you actually see this filter reduces the size of existing_platform_subviews?
Maybe instead we can NSAssert it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you actually see this filter reduces the size of existing_platform_subviews?
Yes, and my understanding is that there are non-platform views added somewhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if it's dummy scroll view on the top status bar that I saw (which enables tap-to-scroll for table views), but that's not important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah ok forgot about those. Yeah makes sense.
cyanglaz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
It turns out that in the benchmark test,
BringLayersIntoViewis called every frame, even if view hierarchy stays the same, resulting in unnecessary subview reordering.For example, let's say we have subviews array
[v1, v2, v3]:[v1, v2, v3](initial config, v3 is the top)=>
addSubview(v1)brings v1 to top, it becomes[v2, v3, v1]=>
addSubview(v2)brings v2 to top, it becomes[v3, v1, v2]=>
addSubview(v3)brings v3 to top, it becomes[v1, v2, v3]The fix is simply to check if the view hierarchy stays the same, and avoid manipulating the subviews if not necessary.
It used to be fine before #39527, because we simply set the zPosition, which is much less costly. However, we should probably investigate whether we can avoid calling
BringLayersIntoViewin the first place.Performance
Right before #39527:
On #39527:
This fix on top of #39527:
List which issues are fixed by this PR. You must list at least one issue.
Fixes flutter/flutter#121833
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
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.