Recompute offset-path transform when containing block resizes [chromium/src : main]
Wangsong Jin (Gerrit)
Message from Wangsong Jin![]( "Open in Gerrit")
Set Ready For Review
Related details
- Wangsong Jin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wangsong Jin (Gerrit)
Wangsong Jin added 9 comments![]( "Open in Gerrit")
Wangsong JinI think PaintLayer::UpdateTransform is also stale in this scenario.
Is the spec clear on this behavior? Is offset-path the only transform property that depends on ancestor size like this, or are there others?
Wangsong JinIs the spec clear on this behavior?
Per CSS Motion Path §2.1, the <coord-box> reference box is "from the element that establishes the containing block for this element," (https://drafts.csswg.org/motion-1/#offset-path-property) so a containing block resize should trigger recomputation.
Does that address your spec question, or is there a specific aspect of the behavior defined in this change that you think the spec is unclear on?
I think PaintLayer::UpdateTransform is also stale in this scenario.
Good catch! I've added an explicit `UpdateTransform()` call to keep the cached matrix in sync, along with a unit test that confirms the test fails without it.
Wangsong JinCould we use a descendant dependent flag (see: UpdateDescendantDependentFlags and has_fixed_position_descendant_)? These flags are updated prior to pre-paint / property tree building. Would it be any better to save and detect size changes and issue invalidations from there?
Thanks for the suggestion! I've updated the CL to follow that approach — invalidation is now deferred to `UpdateDescendantDependentFlags()`, and a `has_offset_path_descendant_` flag on `PaintLayer` (following the `has_fixed_position_descendant_` pattern) gates the scheduling in `SizeChanged()`. This avoids the tree walk during layout.
InvalidateDescendantOffsetPaths();Wangsong JinThis is to slow to do a full tree walk within SizeChanged, should this be moved elsewhere?
Yep, moved to `UpdateDescendantDependentFlags()`
static bool OffsetPathDependsOnContainingBlock(Wangsong JinThis should be a direct instance method of `OffsetPathOperation`, to facilitate re-use and avoid polluting layout classes.
Thanks. I've added `OffsetPathOperation::DependsOnContainingBlock()` as a virtual method, with overrides on each subclass (`ShapeOffsetPathOperation`, `CoordBoxOffsetPathOperation`, `ReferenceOffsetPathOperation`).
void LayoutBox::InvalidateDescendantOffsetPaths() {Wangsong JinAre you sure that a `LayoutInline` is never used as a container? I did some brief testing, and was able to get the apparent travel paths of an offset-path to change by adjusting the font size of the box.
`ContainingBlock()` returns `LayoutBlock*` , and `InclusiveContainingBlock()` explicitly skips LayoutInline and walks up to the nearest `LayoutBlock`. So a `LayoutInline` shouldn't act as the containing block for `offset-path` resolution. I think the `font-size` change you saw affecting the travel path was likely because the inline's content reflow caused its enclosing `LayoutBlock` to resize.
const auto* offset_path = descendant->StyleRef().OffsetPath();Wangsong JinThe control flow of this function is a little weird.
You should be checking `descendant->MayContainOffsetPath()` first as that will also rule out `descendant` itself containing `offset-path`. This would allow you to go to the else early, skipping one check. It also makes it easier to read, becoming...
```
if(descendant->MayContainOffsetPath()) {
... core logic ...
} else {
// Skip subtrees that cannot contain offset-path elements.
descendant = descendant->NextInPreOrderAfterChildren(this);
}
I've removed the DOM tree walk and `MayContainOffsetPath` entirely. The invalidation now happens in `UpdateDescendantDependentFlags()`.
OffsetPathDependsOnContainingBlock(*offset_path) &&Wangsong JinI feel like this should also be checked before setting the flag. You'll also have to check it here, to avoid over-invalidation, but less flag setting = less walking = more happy web platform engineers :)
Done
descendant->ContainingBlock() == this) {Wangsong JinIt would be more complicated, but you could likely make this walk more efficient by incorporating WHAT kind of containing block this is. If this is, say, an absolute containing block, and you go down two children, and you find another absolute containing block, you know that no children will ever use you as an absolute containing block, as the deepest element takes precedence.
This is convenient because these values are cached in bitfields, it's a relatively cheap lookup. E.g. `can_contain_absolute_position_objects_`. This could help you abort deep tree walks early.
Thanks for the suggestion! This is less relevant now since we no longer do a DOM tree walk in latest update.
void LayoutObject::MarkMayContainOffsetPath() {
for (LayoutObject* runner = this; runner && !runner->MayContainOffsetPath();
runner = runner->Parent()) {
runner->SetSelfMayContainOffsetPath();
}
}Wangsong JinIt's marginal, but I think this should also be gated behind the killswitch.
Resolved this. This logic is removed.
Related details
- Claire Chambers
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 1 comment![]( "Open in Gerrit")
PaintLayer* layer = HasLayer() ? Layer() : EnclosingLayer();This still might be too slow - (i'll kick of some pinpoints). Can the invalidation be moved to pre-paint or elsewhere?
(This is called multiple times per layout and is very performance sensitive).
Related details
- Claire Chambers
- Philip Rogers
- Wangsong Jin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wangsong Jin (Gerrit)
Wangsong Jin added 1 comment![]( "Open in Gerrit")
PaintLayer* layer = HasLayer() ? Layer() : EnclosingLayer();This still might be too slow - (i'll kick of some pinpoints). Can the invalidation be moved to pre-paint or elsewhere?
(This is called multiple times per layout and is very performance sensitive).
Thanks for starting the pinpoints! If the overhead is non-trivial or worth the benefit, we should do that. Let's see the result first.
Related details
- Ian Kilpatrick
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m4-mini-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/12a15589c90000
Related details
- Ian Kilpatrick
- Wangsong Jin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wangsong Jin (Gerrit)
Wangsong Jin added 1 comment![]( "Open in Gerrit")
PaintLayer* layer = HasLayer() ? Layer() : EnclosingLayer();Wangsong JinThis still might be too slow - (i'll kick of some pinpoints). Can the invalidation be moved to pre-paint or elsewhere?
(This is called multiple times per layout and is very performance sensitive).
Thanks for starting the pinpoints! If the overhead is non-trivial or worth the benefit, we should do that. Let's see the result first.
The pinpoint is green https://pinpoint-dot-chromeperf.appspot.com/job/12a15589c90000. If we still have concerns about resize performance when offset-path is present on the page, I think there’s an opportunity to move this logic into pre-paint. I’ll post the PatchSet.
Related details
- Ian Kilpatrick
Wangsong Jin (Gerrit)
Wangsong Jin added 1 comment![]( "Open in Gerrit")
PaintLayer* layer = HasLayer() ? Layer() : EnclosingLayer();Wangsong JinThis still might be too slow - (i'll kick of some pinpoints). Can the invalidation be moved to pre-paint or elsewhere?
(This is called multiple times per layout and is very performance sensitive).
Wangsong JinThanks for starting the pinpoints! If the overhead is non-trivial or worth the benefit, we should do that. Let's see the result first.
The pinpoint is green https://pinpoint-dot-chromeperf.appspot.com/job/12a15589c90000. If we still have concerns about resize performance when offset-path is present on the page, I think there’s an opportunity to move this logic into pre-paint. I’ll post the PatchSet.
Hi Ian, in the latest PatchSet 14, I moved all invalidation out of layout resize to pre-paint anyway. We detect CB resize in `PaintPropertyTreeBuilder::UpdateForSelf()` by comparing `StitchedSize()` vs `PreviousSize()`. The HasOffsetPathDescendant() flag on PaintLayer still serves as a cheap guard to skip the descendant walk for boxes without offset-path children. Do you prefer this approach?
Also cc @p...@chromium.org — would appreciate a quick glance at the overall approach to make sure we're on the right track.
Related details
- Ian Kilpatrick
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m4-mini-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1417c761c90000
Related details
- Ian Kilpatrick
- Wangsong Jin
Wangsong Jin (Gerrit)
Wangsong Jin added 1 comment![]( "Open in Gerrit")
PaintLayer* layer = HasLayer() ? Layer() : EnclosingLayer();Wangsong JinThis still might be too slow - (i'll kick of some pinpoints). Can the invalidation be moved to pre-paint or elsewhere?
(This is called multiple times per layout and is very performance sensitive).
Wangsong JinThanks for starting the pinpoints! If the overhead is non-trivial or worth the benefit, we should do that. Let's see the result first.
Wangsong JinThe pinpoint is green https://pinpoint-dot-chromeperf.appspot.com/job/12a15589c90000. If we still have concerns about resize performance when offset-path is present on the page, I think there’s an opportunity to move this logic into pre-paint. I’ll post the PatchSet.
Hi Ian, in the latest PatchSet 14, I moved all invalidation out of layout resize to pre-paint anyway. We detect CB resize in `PaintPropertyTreeBuilder::UpdateForSelf()` by comparing `StitchedSize()` vs `PreviousSize()`. The HasOffsetPathDescendant() flag on PaintLayer still serves as a cheap guard to skip the descendant walk for boxes without offset-path children. Do you prefer this approach?
Also cc @p...@chromium.org — would appreciate a quick glance at the overall approach to make sure we're on the right track.
Hi Ian — I just noticed Philip is OOF until July 6. I think the layout-resize perf concern is now addressed (invalidation moved to pre-paint). Aside from that, do you feel good about the overall approach in general? If anything structural needs changing, I'm happy to address it before we go further. And feel free to let me know if I should cc another owner to keep things moving. Thank you!
Related details
- Ian Kilpatrick
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job linux-perf/blink_perf.layout complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/12bb1a1dc90000
Related details
- Ian Kilpatrick
- Wangsong Jin
Wangsong Jin (Gerrit)
Wangsong Jin added 1 comment![]( "Open in Gerrit")
Hi Ian, would appreciate a quick look when you have a chance. Thanks!
Related details
- Ian Kilpatrick
- Ian Kilpatrick
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 4 comments![]( "Open in Gerrit")
for (LayoutObject* descendant = object_.SlowFirstChild(); descendant;is this really any descendant or just "child"?
for (LayoutObject* descendant = object_.SlowFirstChild(); descendant;box
descendant = descendant->NextInPreOrder(&object_)) {Why does this need NextInPreOrder?
descendant->ContainingBlock() != box) {What about a structure like:
```
<div style="position: relative;">
<div>
<div style="position: absolute; offset-path: blah;"></div>
</div>
</div>
```
Related details
- Philip Rogers
- Wangsong Jin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wangsong Jin (Gerrit)
Wangsong Jin added 8 comments![]( "Open in Gerrit")
Wangsong JinI think PaintLayer::UpdateTransform is also stale in this scenario.
Is the spec clear on this behavior? Is offset-path the only transform property that depends on ancestor size like this, or are there others?
Wangsong JinIs the spec clear on this behavior?
Per CSS Motion Path §2.1, the <coord-box> reference box is "from the element that establishes the containing block for this element," (https://drafts.csswg.org/motion-1/#offset-path-property) so a containing block resize should trigger recomputation.
Does that address your spec question, or is there a specific aspect of the behavior defined in this change that you think the spec is unclear on?
I think PaintLayer::UpdateTransform is also stale in this scenario.
Good catch! I've added an explicit `UpdateTransform()` call to keep the cached matrix in sync, along with a unit test that confirms the test fails without it.
Done
PaintLayer* layer = HasLayer() ? Layer() : EnclosingLayer();Wangsong JinThis still might be too slow - (i'll kick of some pinpoints). Can the invalidation be moved to pre-paint or elsewhere?
(This is called multiple times per layout and is very performance sensitive).
Wangsong JinThanks for starting the pinpoints! If the overhead is non-trivial or worth the benefit, we should do that. Let's see the result first.
Wangsong JinThe pinpoint is green https://pinpoint-dot-chromeperf.appspot.com/job/12a15589c90000. If we still have concerns about resize performance when offset-path is present on the page, I think there’s an opportunity to move this logic into pre-paint. I’ll post the PatchSet.
Wangsong JinHi Ian, in the latest PatchSet 14, I moved all invalidation out of layout resize to pre-paint anyway. We detect CB resize in `PaintPropertyTreeBuilder::UpdateForSelf()` by comparing `StitchedSize()` vs `PreviousSize()`. The HasOffsetPathDescendant() flag on PaintLayer still serves as a cheap guard to skip the descendant walk for boxes without offset-path children. Do you prefer this approach?
Also cc @p...@chromium.org — would appreciate a quick glance at the overall approach to make sure we're on the right track.
Hi Ian — I just noticed Philip is OOF until July 6. I think the layout-resize perf concern is now addressed (invalidation moved to pre-paint). Aside from that, do you feel good about the overall approach in general? If anything structural needs changing, I'm happy to address it before we go further. And feel free to let me know if I should cc another owner to keep things moving. Thank you!
Done
void LayoutBox::InvalidateDescendantOffsetPaths() {Wangsong JinAre you sure that a `LayoutInline` is never used as a container? I did some brief testing, and was able to get the apparent travel paths of an offset-path to change by adjusting the font size of the box.
`ContainingBlock()` returns `LayoutBlock*` , and `InclusiveContainingBlock()` explicitly skips LayoutInline and walks up to the nearest `LayoutBlock`. So a `LayoutInline` shouldn't act as the containing block for `offset-path` resolution. I think the `font-size` change you saw affecting the travel path was likely because the inline's content reflow caused its enclosing `LayoutBlock` to resize.
Done
for (LayoutObject* descendant = object_.SlowFirstChild(); descendant;is this really any descendant or just "child"?
We want to walk all descendants here — the offset-path element might not be a direct child of its CB (e.g., there could be a non-positioned intermediate div in between, like in your example). Traversing only direct children would miss those cases.
for (LayoutObject* descendant = object_.SlowFirstChild(); descendant;Wangsong Jinbox
Marked as resolved.
descendant = descendant->NextInPreOrder(&object_)) {Why does this need NextInPreOrder?
Similar reason as above. `NextInPreOrder` walks the full subtree to find any non-direct offset-path descendant that needs to be updated.
What about a structure like:
```
<div style="position: relative;">
<div>
<div style="position: absolute; offset-path: blah;"></div>
</div>
</div>
```
Yes, this is the nested case we handle. We've added a WPT test `offset-path-containing-block-resize-nested-absolute.html` which covers this structure.
Related details
- Ian Kilpatrick
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job linux-perf/blink_perf.layout complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1645a55fc90000
Related details
- Ian Kilpatrick
- Philip Rogers
- Wangsong Jin
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
😿 Job linux-perf/blink_perf.layout failed.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1351ee90290000
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 1 comment![]( "Open in Gerrit")
descendant = descendant->NextInPreOrder(&object_)) {Wangsong JinWhy does this need NextInPreOrder?
Similar reason as above. `NextInPreOrder` walks the full subtree to find any non-direct offset-path descendant that needs to be updated.
Ok - yeah I don't think we should land this approach. These full tree traversals are very much a performance footgun for developers. (e.g. it looks like if we have an offset-path on every element on the page this goes to being quadratic or worse performance wise). We are basically trading one correctness but for a performance bug.
Stepping back a bit - ideally we should be reworking how transforms are computed.
E.g. The issue is that ComputedStyle::ApplyTransform is taking a LayoutBox, and then walking up the containing-block chain to grab (potentially) stale data, then we are sporadically recomputing it elsewhere.
Instead we should change ComputedStyle::ApplyTransform by removing the LayoutBox argument, and replacing it with (at least) a std::optional<PhysicalRect> (its position in the container in additional to whatever other input data we need). We can omit this argument when we are passing in kExcludeMotionPath for example.
Then there are two different callpaths.
In PaintLayer::UpdateTransform we should store the previous cached value of the PhysicalRect/etc next to the transform so we can update it as-is.
We introduce a new PaintLayer::UpdateTransform(std::optional<InputForMotionPathWhateverThatIs>);
This will compare the new input to the old (stored beside the transform) - then update if needed.
We add this call to perhaps - GetTransformForChildFragment for example. But I'll need to think on this a little.
Additionally the UpdateTransform call within LocationChanged should be removed.
I know this is a lot - but this will prevent future issues.
Related details
- Philip Rogers
- Wangsong Jin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wangsong Jin (Gerrit)
Wangsong Jin added 1 comment![]( "Open in Gerrit")
descendant = descendant->NextInPreOrder(&object_)) {Wangsong JinWhy does this need NextInPreOrder?
Ian KilpatrickSimilar reason as above. `NextInPreOrder` walks the full subtree to find any non-direct offset-path descendant that needs to be updated.
Ok - yeah I don't think we should land this approach. These full tree traversals are very much a performance footgun for developers. (e.g. it looks like if we have an offset-path on every element on the page this goes to being quadratic or worse performance wise). We are basically trading one correctness but for a performance bug.
Stepping back a bit - ideally we should be reworking how transforms are computed.
E.g. The issue is that ComputedStyle::ApplyTransform is taking a LayoutBox, and then walking up the containing-block chain to grab (potentially) stale data, then we are sporadically recomputing it elsewhere.
Instead we should change ComputedStyle::ApplyTransform by removing the LayoutBox argument, and replacing it with (at least) a std::optional<PhysicalRect> (its position in the container in additional to whatever other input data we need). We can omit this argument when we are passing in kExcludeMotionPath for example.
Then there are two different callpaths.
In PaintLayer::UpdateTransform we should store the previous cached value of the PhysicalRect/etc next to the transform so we can update it as-is.
We introduce a new PaintLayer::UpdateTransform(std::optional<InputForMotionPathWhateverThatIs>);
This will compare the new input to the old (stored beside the transform) - then update if needed.
We add this call to perhaps - GetTransformForChildFragment for example. But I'll need to think on this a little.
Additionally the UpdateTransform call within LocationChanged should be removed.
I know this is a lot - but this will prevent future issues.
Thanks for the detailed proposal. When I was working through the nested case, I had a sense that something wasn't quite right with the data flow, but I couldn't put my finger on what the cause was. Your proposed direction of having each element detect when its own motion path input has changed makes sense to me, which avoids the tree traversal cost.
I took a closer look and the refactor has a pretty wide surface area. To be honest, I don't have much experience with the transform computation pipeline, so I'm probably not the best person to drive this. I worry it'd end up taking more of your review time than if someone with more context took it on. But if you're okay with that trade-off, I'm very happy to pick this up incrementally with your input along the way, just a heads up I have other priorities so it might move slowly. Either way works for me, let me know how you'd like to proceed.
Also, if the refactor won't be in place for a while, should we land a scoped fix that only handles direct children? That would cover the most common cases without the deep tree traversal concern.
Related details
- Ian Kilpatrick
- Philip Rogers