Bug 66901 – Enable RenderLayer::updateLayerPosition's cachedOffset optimization for more cases

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Julien Chaffraix]

Currently cachedOffset is only setup when doing a layout of the whole tree (see FrameView::layout()). However RenderLayer::updateLayerPositions has multiple call sites which could benefit from the optimization. Among them is a layout of a subtree inside FrameView::layout.

This is a trade-off as not hitting the cache later means that we have wasted a traversal to the root of the tree. However discussing this with James, it seems OK to go this route and on average should be at least a wash.

[Julien Chaffraix]

Created attachment 105117 [details]
Proposed change: Extend the use of cachedOffset.

[WebKit Review Bot]

Attachment 105117 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/rendering/RenderLayer.h:637:  Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Julien Chaffraix]

> If any of these errors are false positives, please file a bug against check-webkit-style.

The style issue is a false positive: it's just a trailing ; from the 'if' statement because we have some SVG specific checks.

The windows build breakage needs to be addressed though.

[Julien Chaffraix]

Created attachment 105208 [details]
Version 2: Should fix windows bot.

[WebKit Review Bot]

Attachment 105208 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/rendering/RenderLayer.h:637:  Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dave Hyatt]

Comment on attachment 105208 [details]
Version 2: Should fix windows bot.

View in context: https://bugs.webkit.org/attachment.cgi?id=105208&action=review

> Source/WebCore/page/FrameView.cpp:1064
> +    bool disabled;

Rename to cachedOffsetDisabled.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:362
> +                // There is only one layer to update, it is not worth using |cachedOffset| since
> +                // we are not sure the value will be used.
> +                static LayoutPoint* disabledCachedOffset = 0;
> +                m_layer->updateLayerPositions(disabledCachedOffset);

I don't understand this change.

> Source/WebCore/rendering/RenderLayer.cpp:264
> +    // prevent the optimization to work.

"from working" would read better than "to work"

> Source/WebCore/rendering/RenderLayer.h:311
> +    void updateLayerPositions(LayoutPoint* cachedOffset, UpdateLayerPositionsFlags = defaultFlags);

Allow the cachedOffset to be 0 still and you wouldn't have to change RenderBoxModelObject.

[Julien Chaffraix]

Comment on attachment 105208 [details]
Version 2: Should fix windows bot.

View in context: https://bugs.webkit.org/attachment.cgi?id=105208&action=review

Thanks for looking at this change, some comments inlined. The rest will be changed.

>> Source/WebCore/rendering/RenderBoxModelObject.cpp:362
>> +                m_layer->updateLayerPositions(disabledCachedOffset);
> 
> I don't understand this change.

gcc was complaining about this line because it would interpret 0 as an integer. Let me double check if it is needed and if that's the case, the comment will be amended (see below as to why I want to keep this change).

>> Source/WebCore/rendering/RenderLayer.h:311
>> +    void updateLayerPositions(LayoutPoint* cachedOffset, UpdateLayerPositionsFlags = defaultFlags);
> 
> Allow the cachedOffset to be 0 still and you wouldn't have to change RenderBoxModelObject.

I disagree here. The cachedOffset optimization is important and we really don't want people to start disabling it by mistake or at least without an explanation as to why. Let me add a comment about for the record.

[Julien Chaffraix]

Created attachment 105408 [details]
Version 3: Integrating Dave's comments.

[WebKit Review Bot]

Attachment 105408 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/rendering/RenderLayer.h:641:  Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Julien Chaffraix]

Created attachment 105673 [details]
Version 4: Correct a bug in the change, added a test case for that.

[WebKit Review Bot]

Attachment 105673 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/rendering/RenderLayer.h:641:  Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Julien Chaffraix]

> If any of these errors are false positives, please file a bug against check-webkit-style.

FWIW filed bug 67502 about the false positive style issue.

[Simon Fraser (smfr)]

Comment on attachment 105673 [details]
Version 4: Correct a bug in the change, added a test case for that.

View in context: https://bugs.webkit.org/attachment.cgi?id=105673&action=review

> LayoutTests/fast/layers/crash-RenderLayer-update-positions.html:1
> +<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd">

Just <!DOCTYPE html> please.

> LayoutTests/fast/layers/crash-RenderLayer-update-positions.html:39
> +                <script>
> +                    if (window.layoutTestController) {
> +                        layoutTestController.dumpAsText();
> +                        layoutTestController.waitUntilDone();
> +                    }
> +
> +                    function crash() {
> +                        var dimensionsContainer = document.getElementById("crasher");
> +                        dimensionsContainer.setAttribute("class", "");
> +                        if (window.layoutTestController)
> +                            layoutTestController.notifyDone();
> +                    }
> +                    // Asynchronously change the style to force a style recalc later.
> +                    window.setTimeout(crash, 0);
> +                </script>

Is it necessary to embed the script here? Seems odd. Also, a JS function called crash() is weird, and the test should refer to asserting, not crashing.

> Source/WebCore/rendering/RenderLayer.cpp:256
> +LayoutPoint RenderLayer::cachedOffsetFromRoot(bool& cachedOffsetDisabled) const

The term 'cached' in the method name is misleading; the offset hasn't been cached, it's being computed.

> Source/WebCore/rendering/RenderLayer.cpp:274
> +    for (const RenderLayer* parentLayer = parent(); parentLayer; rootLayer = parentLayer, parentLayer = parentLayer->parent()) {
> +        cachedOffsetDisabled = parentLayer->shouldDisableOffsetCache();
> +        if (cachedOffsetDisabled)
> +            return LayoutPoint();
> +    }
> +    ASSERT(rootLayer == root());
> +
> +    LayoutPoint offset;
> +    parent()->convertToLayerCoords(rootLayer, offset);

There are two ancestor walks here. Can they be combined?

> Source/WebCore/rendering/RenderLayer.cpp:1147
> +        LayoutPoint cachedOffset = parentCachedOffset;
> +        // updateLayerPositions depends on hasLayer() already being false for proper layout.
> +        current->updateLayerPositions(cachedOffsetDisabled ? 0 : &cachedOffset);

This is confusing. parentCachedOffset is the offset of |this|, yet you're passiong that offset down for a child? What about the this->child offset?

> Source/WebCore/rendering/RenderLayer.h:313
> +    void updateLayerPositions(LayoutPoint* cachedOffset, UpdateLayerPositionsFlags = defaultFlags);

I think you should give cachedOffset a default value of 0.

> Source/WebCore/rendering/RenderLayer.h:642
> +    bool shouldDisableOffsetCache() const
> +    {
> +        return renderer()->hasColumns() || renderer()->hasTransform() || isComposited()
> +#if ENABLE(SVG)
> +            || renderer()->isSVGRoot()
> +#endif
> +            ;
> +    }

I think the name of this method could be broadened. It really means "is it OK to call convertToLayerCoords() across this layer". There are probably other places in the code we could call this.

[Julien Chaffraix]

Comment on attachment 105673 [details]
Version 4: Correct a bug in the change, added a test case for that.

View in context: https://bugs.webkit.org/attachment.cgi?id=105673&action=review

>> LayoutTests/fast/layers/crash-RenderLayer-update-positions.html:39
>> +                </script>
> 
> Is it necessary to embed the script here? Seems odd. Also, a JS function called crash() is weird, and the test should refer to asserting, not crashing.

I am pretty sure the script needs to be embedded here to get the repro. Let me double check if it can be moved.

>> Source/WebCore/rendering/RenderLayer.cpp:256
>> +LayoutPoint RenderLayer::cachedOffsetFromRoot(bool& cachedOffsetDisabled) const
> 
> The term 'cached' in the method name is misleading; the offset hasn't been cached, it's being computed.

How about computeOffsetFromRoot?

>> Source/WebCore/rendering/RenderLayer.cpp:274
>> +    parent()->convertToLayerCoords(rootLayer, offset);
> 
> There are two ancestor walks here. Can they be combined?

It may be possible. I don't think it will be easy though as convertToLayerCoords may use localToAbsolute which uses the RenderTree to do the conversion. If you don't mind, I would rather push that to another bug.

>> Source/WebCore/rendering/RenderLayer.cpp:1147
>> +        current->updateLayerPositions(cachedOffsetDisabled ? 0 : &cachedOffset);
> 
> This is confusing. parentCachedOffset is the offset of |this|, yet you're passiong that offset down for a child? What about the this->child offset?

What you are passing to updateLayerPositions is actually the parentOffset. If you look at updateLayerPositions, after computing our relative offset, we update cachedOffset (which is the parent's cachedOffset) to account for the child offset from its parent.

>> Source/WebCore/rendering/RenderLayer.h:313
>> +    void updateLayerPositions(LayoutPoint* cachedOffset, UpdateLayerPositionsFlags = defaultFlags);
> 
> I think you should give cachedOffset a default value of 0.

I would rather not do that. Such a change would make it easy to forget this optimization which is not something we want.

>> Source/WebCore/rendering/RenderLayer.h:642
>> +    }
> 
> I think the name of this method could be broadened. It really means "is it OK to call convertToLayerCoords() across this layer". There are probably other places in the code we could call this.

Most functions just call convertToLayerCoords regardless of what shouldDisabledOffsetCache would return. There is also 2 cases where we are actually going against your definition: updateLayerPositions (the branch when cachedOffset is 0) and paintChildLayerIntoColumns. Unless I am missing something, the code seems to diverge from your definition. I can mark the call sites if you think this is needed.

[Julien Chaffraix]

Created attachment 106197 [details]
Version 5: Addressing Simon's comments.

[WebKit Review Bot]

Attachment 106197 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/rendering/RenderLayer.h:643:  Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 106197 [details]
Version 5: Addressing Simon's comments.

View in context: https://bugs.webkit.org/attachment.cgi?id=106197&action=review

> Source/WebCore/page/FrameView.cpp:1094
> +    bool cachedOffsetDisabled;
> +    LayoutPoint cachedOffset = layer->computeOffsetFromRoot(cachedOffsetDisabled);

cachedOffsetDisabled now doesn't read quite right. It's more like 'was able to compute offset'. Maybe 'haveLayerOffset'?

> Source/WebCore/rendering/RenderLayer.cpp:278
> +void RenderLayer::updateLayerPositions(LayoutPoint* cachedOffset, UpdateLayerPositionsFlags flags)

I think cachedOffset should be renamed here to offsetFromRoot (or parentOffsetFromRoot maybe).

> Source/WebCore/rendering/RenderLayer.cpp:1149
> +        // updateLayerPositions depends on hasLayer() already being false for proper layout.

Should that be asserted?

> Source/WebCore/rendering/RenderLayer.h:314
> +    void updateLayerPositions(LayoutPoint* cachedOffset, UpdateLayerPositionsFlags = defaultFlags);

Rename cachedOffset

> Source/WebCore/rendering/RenderLayer.h:637
> +    bool convertToLayerCoordsIsDisabled() const

This is really "is the layer position affected by things that RenderLayer doesn't know about". I think the sense of the method should be flipped ("okToUseConvertToLayerCoords), but I can't think of a better name.

[Julien Chaffraix]

Comment on attachment 106197 [details]
Version 5: Addressing Simon's comments.

View in context: https://bugs.webkit.org/attachment.cgi?id=106197&action=review

>> Source/WebCore/page/FrameView.cpp:1094
>> +    LayoutPoint cachedOffset = layer->computeOffsetFromRoot(cachedOffsetDisabled);
> 
> cachedOffsetDisabled now doesn't read quite right. It's more like 'was able to compute offset'. Maybe 'haveLayerOffset'?

Fine by me (however the 3rd person - hasLayerOffset - sounds better).

>> Source/WebCore/rendering/RenderLayer.cpp:278
>> +void RenderLayer::updateLayerPositions(LayoutPoint* cachedOffset, UpdateLayerPositionsFlags flags)
> 
> I think cachedOffset should be renamed here to offsetFromRoot (or parentOffsetFromRoot maybe).

I would go for offsetFromRoot as the meaning of the variable change inside the function.

>> Source/WebCore/rendering/RenderLayer.cpp:1149
>> +        // updateLayerPositions depends on hasLayer() already being false for proper layout.
> 
> Should that be asserted?

Sure, let me see if it is hit.

>> Source/WebCore/rendering/RenderLayer.h:637
>> +    bool convertToLayerCoordsIsDisabled() const
> 
> This is really "is the layer position affected by things that RenderLayer doesn't know about". I think the sense of the method should be flipped ("okToUseConvertToLayerCoords), but I can't think of a better name.

canUseConvertToLayerCoords would match our usual naming and get the same idea. Any objection?

[Julien Chaffraix]

Created attachment 106217 [details]
Version 6: Addressing Simon's latest comments.

[WebKit Review Bot]

Attachment 106217 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/rendering/RenderLayer.h:644:  Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Julien Chaffraix]

Comment on attachment 106217 [details]
Version 6: Addressing Simon's latest comments.

Thanks Simon!

[WebKit Review Bot]

Comment on attachment 106217 [details]
Version 6: Addressing Simon's latest comments.

Clearing flags on attachment: 106217

Committed r94464: <http://trac.webkit.org/changeset/94464>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.