Bug 66901 - Enable RenderLayer::updateLayerPosition's cachedOffset optimization for more cases
Summary: Enable RenderLayer::updateLayerPosition's cachedOffset optimization for more ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Julien Chaffraix
URL:
Keywords:
Depends on:
Blocks: 67548
  Show dependency treegraph
 
Reported: 2011-08-24 15:20 PDT by Julien Chaffraix
Modified: 2011-09-02 18:15 PDT (History)
4 users (show)

See Also:


Attachments
Proposed change: Extend the use of cachedOffset. (9.95 KB, patch)
2011-08-24 19:02 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Version 2: Should fix windows bot. (9.96 KB, patch)
2011-08-25 10:19 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Version 3: Integrating Dave's comments. (10.23 KB, patch)
2011-08-26 14:23 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Version 4: Correct a bug in the change, added a test case for that. (13.40 KB, patch)
2011-08-30 12:52 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Version 5: Addressing Simon's comments. (14.67 KB, patch)
2011-09-02 14:22 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Version 6: Addressing Simon's latest comments. (16.73 KB, patch)
2011-09-02 16:16 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Chaffraix 2011-08-24 15:20:29 PDT
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.
Comment 1 Julien Chaffraix 2011-08-24 19:02:41 PDT
Created attachment 105117 [details]
Proposed change: Extend the use of cachedOffset.
Comment 2 WebKit Review Bot 2011-08-24 19:04:28 PDT
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.
Comment 3 Julien Chaffraix 2011-08-25 10:04:43 PDT
> 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.
Comment 4 Julien Chaffraix 2011-08-25 10:19:45 PDT
Created attachment 105208 [details]
Version 2: Should fix windows bot.
Comment 5 WebKit Review Bot 2011-08-25 10:25:10 PDT
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.
Comment 6 Dave Hyatt 2011-08-26 13:26:38 PDT
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.
Comment 7 Julien Chaffraix 2011-08-26 13:46:17 PDT
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.
Comment 8 Julien Chaffraix 2011-08-26 14:23:54 PDT
Created attachment 105408 [details]
Version 3: Integrating Dave's comments.
Comment 9 WebKit Review Bot 2011-08-26 14:25:42 PDT
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.
Comment 10 Julien Chaffraix 2011-08-30 12:52:44 PDT
Created attachment 105673 [details]
Version 4: Correct a bug in the change, added a test case for that.
Comment 11 WebKit Review Bot 2011-08-30 12:54:40 PDT
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.
Comment 12 Julien Chaffraix 2011-09-02 10:36:55 PDT
> 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.
Comment 13 Simon Fraser (smfr) 2011-09-02 10:37:57 PDT
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.
Comment 14 Julien Chaffraix 2011-09-02 11:27:08 PDT
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.
Comment 15 Julien Chaffraix 2011-09-02 14:22:22 PDT
Created attachment 106197 [details]
Version 5: Addressing Simon's comments.
Comment 16 WebKit Review Bot 2011-09-02 14:25:20 PDT
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.
Comment 17 Simon Fraser (smfr) 2011-09-02 14:46:35 PDT
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.
Comment 18 Julien Chaffraix 2011-09-02 15:03:34 PDT
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?
Comment 19 Julien Chaffraix 2011-09-02 16:16:01 PDT
Created attachment 106217 [details]
Version 6: Addressing Simon's latest comments.
Comment 20 WebKit Review Bot 2011-09-02 16:19:40 PDT
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.
Comment 21 Julien Chaffraix 2011-09-02 16:23:18 PDT
Comment on attachment 106217 [details]
Version 6: Addressing Simon's latest comments.

Thanks Simon!
Comment 22 WebKit Review Bot 2011-09-02 17:06:52 PDT
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>
Comment 23 WebKit Review Bot 2011-09-02 17:06:57 PDT
All reviewed patches have been landed.  Closing bug.