Bug 89828 – Optimize RenderGeometryMap mappings gathering

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Antti Koivisto]

RenderGeometryMap currently gathers mappings by climbing the rendering tree. This is slow and can produce large number of mapping steps. In the common case we already have the child layer coordinates available in the layer tree and we can just use that.

[Antti Koivisto]

<rdar://problem/10559009>

[Antti Koivisto]

Created attachment 149194 [details]
patch

[WebKit Review Bot]

Attachment 149194 [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:483:  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.

[Simon Fraser (smfr)]

Comment on attachment 149194 [details]
patch

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

> Source/WebCore/rendering/RenderGeometryMap.cpp:182
> +    if (ancestorLayer && layer->canUseConvertToLayerCoords() && renderer->style()->position() != FixedPosition) {

Doesn't this need to check canUseConvertToLayerCoords() on all layers between layer and ancestorLayer? There's no guarantee that ancestorLayer is an immediate ancestor.

> Source/WebCore/rendering/RenderGeometryMap.h:52
> +    // Called by code walking the layer tree.
> +    void pushMappingsToAncestor(const RenderLayer*, const RenderLayer* ancestorLayer);
> +    void popMappingsToAncestor(const RenderLayer*);

I'd like to keep versions that are renderer-based, so we can call them from absoluteRects()/absoluteQuads() at some point (we already know of cases where these are a bottleneck).

[Simon Fraser (smfr)]

Also, I think we should try to wean ourselves off of convertToLayerCoords in the long term, since it would be better to have one way to do coordinate math that always "just works", rather than checking whether it's OK to call it.

[Antti Koivisto]

(In reply to comment #4)
> Doesn't this need to check canUseConvertToLayerCoords() on all layers between layer and ancestorLayer? There's no guarantee that ancestorLayer is an immediate ancestor.

Sounds likely. Can you think of a way to test that case? The patch is logic pretty well tested due to 

    ASSERT(enclosingIntRect(rendererMappedResult) == enclosingIntRect(FloatQuad(result).boundingBox()));

in RenderGeometryMap::absoluteRect() and we don't hit the case in layout tests. 

> I'd like to keep versions that are renderer-based, so we can call them from absoluteRects()/absoluteQuads() at some point (we already know of cases where these are a bottleneck).

We don't usually leave around dead code. It is trivial to add back later. Are you planning to use it soon?

[Antti Koivisto]

(In reply to comment #5)
> Also, I think we should try to wean ourselves off of convertToLayerCoords in the long term, since it would be better to have one way to do coordinate math that always "just works", rather than checking whether it's OK to call it.

Sounds very sensible, this is not nice at all. The new scheme will need some sort of caching though, just walking up the rendering tree is pretty slow.

[Antti Koivisto]

Created attachment 149206 [details]
updated patch

- check that all layers between the current and the ancestor have canUseConvertToLayerCoords().
- put the renderer version of pushMappingsToAncestor back for future use

[WebKit Review Bot]

Attachment 149206 [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:483:  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.

[Simon Fraser (smfr)]

Comment on attachment 149206 [details]
updated patch

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

> Source/WebCore/rendering/RenderGeometryMap.cpp:190
> +        canConvertInLayerTree = current->canUseConvertToLayerCoords();

Shouldn't this be &= ?

[Antti Koivisto]

(In reply to comment #10)
> Shouldn't this be &= ?

No need, we bail out from the loop if the variable turns false.

[Antti Koivisto]

http://trac.webkit.org/changeset/121124

[Antti Koivisto]

http://trac.webkit.org/changeset/121130 fixes an assert with fast/block/inline-children-root-linebox-crash.html

[Simon Fraser (smfr)]

This change caused RenderGeometryMap::pushView() to no longer get called. This might have broken compositing overlap testing inside of position:fixed, I'm not sure.

[Simon Fraser (smfr)]

        push(renderer, toLayoutSize(layerOffset), /*accumulatingTransform*/ true, /*isNonUniform*/ false, /*isFixedPosition*/ false, /*hasTransform*/ false);

accumulatingTransform should also be false; that refers to preserve-3d.

[Simon Fraser (smfr)]

...and I think we need to check for position:fixed on intermediate layers.

[Antti Koivisto]

The assert in absoluteRect should verify that the results are identical to the old code across all our tests. I wonder if those cases don't occur for some reason? Or is the test coverage just weak?

[Simon Fraser (smfr)]

We'd hae to have a test that has compositing layers inside position:fixed, with a scrolled page. We might not. Or maybe the breakage is benign so far (but I'm making some changes that hit it).

[Antti Koivisto]

The top level view will use pushView() due to the ancestorLayer test. I think the problems would be with scrolled subframes. Not sure if canUseConvertToLayerCoords test catches them.

[Simon Fraser (smfr)]

pushView was never hit when using the convertToLayerCoords path.

[Antti Koivisto]

(In reply to comment #20)
> pushView was never hit when using the convertToLayerCoords path.

Correct. The idea is to not take convertToLayerCoords path for the RenderViews.

[Simon Fraser (smfr)]

Yeah. Push the RenderView alone, then push layer offsets from it.

[Simon Fraser (smfr)]

The patch in bug 90342 has some relevant RenderGeometryMap changes (assertions and fixes), but not a fix for the pushView issue.

[Simon Fraser (smfr)]

This caused bug 90919.