- Revision
- 94464
- Author
- [email protected]
- Date
- 2011-09-02 17:07:44 -0700 (Fri, 02 Sep 2011)
Log Message
Enable RenderLayer::updateLayerPosition's cachedOffset optimization for more cases
https://bugs.webkit.org/show_bug.cgi?id=66901
Reviewed by Simon Fraser.
Source/WebCore:
Test: fast/layers/assert-RenderLayer-update-positions.html
Also covered by existing tests under the new ASSERT.
This change extends the range of callers making use of the cachedOffset optimization.
Most callers did not make use of cachedOffset as it did not work when called on a subtree.
This limitation is now gone thus we can enable it more widely.
The semantics of the optimization are changed a bit as we now return if it is enabled whereas
the old code would check if it was *disabled*. Also there were some renames done to match more
closely what was going on (s/cachedOffset/offsetFromRoot/ and s/cachedOffsetDisabled/hasLayerOffset/).
Note that this is an optimistic optimization: if cachedOffset is not used, then we have
done at least an extra traversal up to the root. I have found it to be a wash on file
cycler (alexa) but to be a nice improvement (~20%) on some table benchmarks (modifying
a cell, scrolling).
* page/FrameView.cpp:
(WebCore::FrameView::layout): Extended the use of cachedOffset to subtree layouts.
* rendering/RenderBoxModelObject.cpp:
(WebCore::RenderBoxModelObject::styleDidChange): Forbid the use cachedOffset in this
case as we have only a single layer to update.
* rendering/RenderLayer.cpp:
(WebCore::RenderLayer::computeOffsetFromRoot): Added this function to get the offset from the root
layer at a certain point in the RenderLayer's tree. It gets the root layer's checking if no layer
in between would prevent convertToLayerCoords to work and return the position relative to
this layer.
(WebCore::RenderLayer::updateLayerPositions): Added a new ASSERT to make sure our cachedOffset
is always fine. Also added a comment about calling convertToLayerCoords.
(WebCore::RenderLayer::removeOnlyThisLayer): Added cachedOffset here too as we may have to
update several layers. We save the offset prior to being removed from the hierarchy for
correctness.
(WebCore::RenderLayer::paintChildLayerIntoColumns): Added a comment here about calling convertToLayerCoords.
* rendering/RenderLayer.h: Swapped the argument in updateLayerPositions to make
cachedOffset a mandatory field. Patched all the callers.
(WebCore::RenderLayer::canUseConvertToLayerCoords): Added this helper method to know when a
renderer prevents convertToLayerCoords from working. Added some FIXME around suspicious use
of convertToLayerCoords.
LayoutTests:
* fast/layers/crash-RenderLayer-update-positions-expected.txt: Added.
* fast/layers/crash-RenderLayer-update-positions.html: Added.
Test that some changes in the layers does not cause an ASSERT failure.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (94463 => 94464)
--- trunk/LayoutTests/ChangeLog 2011-09-02 23:58:05 UTC (rev 94463)
+++ trunk/LayoutTests/ChangeLog 2011-09-03 00:07:44 UTC (rev 94464)
@@ -1,3 +1,14 @@
+2011-09-02 Julien Chaffraix <[email protected]>
+
+ Enable RenderLayer::updateLayerPosition's cachedOffset optimization for more cases
+ https://bugs.webkit.org/show_bug.cgi?id=66901
+
+ Reviewed by Simon Fraser.
+
+ * fast/layers/crash-RenderLayer-update-positions-expected.txt: Added.
+ * fast/layers/crash-RenderLayer-update-positions.html: Added.
+ Test that some changes in the layers does not cause an ASSERT failure.
+
2011-09-02 Dan Bernstein <[email protected]>
<rdar://problem/9755843> anonymous RenderMathMLOperator sets itself as the renderer of its parent mfenced node
Added: trunk/LayoutTests/fast/layers/assert-RenderLayer-update-positions-expected.txt (0 => 94464)
--- trunk/LayoutTests/fast/layers/assert-RenderLayer-update-positions-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/layers/assert-RenderLayer-update-positions-expected.txt 2011-09-03 00:07:44 UTC (rev 94464)
@@ -0,0 +1 @@
+Test for bug 66901. The test passes if it does not ASSERT.
Added: trunk/LayoutTests/fast/layers/assert-RenderLayer-update-positions.html (0 => 94464)
--- trunk/LayoutTests/fast/layers/assert-RenderLayer-update-positions.html (rev 0)
+++ trunk/LayoutTests/fast/layers/assert-RenderLayer-update-positions.html 2011-09-03 00:07:44 UTC (rev 94464)
@@ -0,0 +1,45 @@
+<!DOCTYPE html>
+<html>
+<head>
+ <style>
+ .no-js {
+ position: relative;
+ }
+
+ .no-js h3 {
+ position: absolute;
+ }
+ </style>
+</head>
+<body>
+<div>
+ <div>
+ <div style="position: relative">
+ <div id="assertTrigger" class="no-js">
+ <div>
+ <div>
+ <h3>Test for bug 66901. The test passes if it does not <span>ASSERT.</span></h3>
+ </div>
+ </div>
+ <script>
+ if (window.layoutTestController) {
+ layoutTestController.dumpAsText();
+ layoutTestController.waitUntilDone();
+ }
+
+ function assert() {
+ var dimensionsContainer = document.getElementById("assertTrigger");
+ dimensionsContainer.setAttribute("class", "");
+ if (window.layoutTestController)
+ layoutTestController.notifyDone();
+ }
+
+ // Asynchronously change the style to force a style recalc later.
+ window.setTimeout(assert, 0);
+ </script>
+ </div>
+ </div>
+ </div>
+</div>
+</body>
+</html>
Property changes on: trunk/LayoutTests/fast/layers/assert-RenderLayer-update-positions.html
___________________________________________________________________
Added: svn:executable
Modified: trunk/Source/WebCore/ChangeLog (94463 => 94464)
--- trunk/Source/WebCore/ChangeLog 2011-09-02 23:58:05 UTC (rev 94463)
+++ trunk/Source/WebCore/ChangeLog 2011-09-03 00:07:44 UTC (rev 94464)
@@ -1,3 +1,55 @@
+2011-09-02 Julien Chaffraix <[email protected]>
+
+ Enable RenderLayer::updateLayerPosition's cachedOffset optimization for more cases
+ https://bugs.webkit.org/show_bug.cgi?id=66901
+
+ Reviewed by Simon Fraser.
+
+ Test: fast/layers/assert-RenderLayer-update-positions.html
+ Also covered by existing tests under the new ASSERT.
+
+ This change extends the range of callers making use of the cachedOffset optimization.
+
+ Most callers did not make use of cachedOffset as it did not work when called on a subtree.
+ This limitation is now gone thus we can enable it more widely.
+
+ The semantics of the optimization are changed a bit as we now return if it is enabled whereas
+ the old code would check if it was *disabled*. Also there were some renames done to match more
+ closely what was going on (s/cachedOffset/offsetFromRoot/ and s/cachedOffsetDisabled/hasLayerOffset/).
+
+ Note that this is an optimistic optimization: if cachedOffset is not used, then we have
+ done at least an extra traversal up to the root. I have found it to be a wash on file
+ cycler (alexa) but to be a nice improvement (~20%) on some table benchmarks (modifying
+ a cell, scrolling).
+
+ * page/FrameView.cpp:
+ (WebCore::FrameView::layout): Extended the use of cachedOffset to subtree layouts.
+ * rendering/RenderBoxModelObject.cpp:
+ (WebCore::RenderBoxModelObject::styleDidChange): Forbid the use cachedOffset in this
+ case as we have only a single layer to update.
+
+ * rendering/RenderLayer.cpp:
+ (WebCore::RenderLayer::computeOffsetFromRoot): Added this function to get the offset from the root
+ layer at a certain point in the RenderLayer's tree. It gets the root layer's checking if no layer
+ in between would prevent convertToLayerCoords to work and return the position relative to
+ this layer.
+
+ (WebCore::RenderLayer::updateLayerPositions): Added a new ASSERT to make sure our cachedOffset
+ is always fine. Also added a comment about calling convertToLayerCoords.
+
+ (WebCore::RenderLayer::removeOnlyThisLayer): Added cachedOffset here too as we may have to
+ update several layers. We save the offset prior to being removed from the hierarchy for
+ correctness.
+
+ (WebCore::RenderLayer::paintChildLayerIntoColumns): Added a comment here about calling convertToLayerCoords.
+
+ * rendering/RenderLayer.h: Swapped the argument in updateLayerPositions to make
+ cachedOffset a mandatory field. Patched all the callers.
+
+ (WebCore::RenderLayer::canUseConvertToLayerCoords): Added this helper method to know when a
+ renderer prevents convertToLayerCoords from working. Added some FIXME around suspicious use
+ of convertToLayerCoords.
+
2011-08-30 Matthew Delaney <[email protected]>
Read out of bounds in sUnpremultiplyData_RGBA8888 / ImageBufferData::getData
Modified: trunk/Source/WebCore/page/FrameView.cpp (94463 => 94464)
--- trunk/Source/WebCore/page/FrameView.cpp 2011-09-02 23:58:05 UTC (rev 94463)
+++ trunk/Source/WebCore/page/FrameView.cpp 2011-09-03 00:07:44 UTC (rev 94464)
@@ -1090,14 +1090,15 @@
// Now update the positions of all layers.
beginDeferredRepaints();
- LayoutPoint cachedOffset;
+ bool hasLayerOffset;
+ LayoutPoint offsetFromRoot = layer->computeOffsetFromRoot(hasLayerOffset);
if (m_doFullRepaint)
root->view()->repaint(); // FIXME: This isn't really right, since the RenderView doesn't fully encompass the visibleContentRect(). It just happens
// to work out most of the time, since first layouts and printing don't have you scrolled anywhere.
- layer->updateLayerPositions((m_doFullRepaint ? 0 : RenderLayer::CheckForRepaint)
+ layer->updateLayerPositions(hasLayerOffset ? &offsetFromRoot : 0,
+ (m_doFullRepaint ? 0 : RenderLayer::CheckForRepaint)
| RenderLayer::IsCompositingUpdateRoot
- | RenderLayer::UpdateCompositingLayers,
- subtree ? 0 : &cachedOffset);
+ | RenderLayer::UpdateCompositingLayers);
endDeferredRepaints();
#if USE(ACCELERATED_COMPOSITING)
Modified: trunk/Source/WebCore/rendering/RenderBoxModelObject.cpp (94463 => 94464)
--- trunk/Source/WebCore/rendering/RenderBoxModelObject.cpp 2011-09-02 23:58:05 UTC (rev 94463)
+++ trunk/Source/WebCore/rendering/RenderBoxModelObject.cpp 2011-09-03 00:07:44 UTC (rev 94464)
@@ -356,7 +356,9 @@
m_layer->insertOnlyThisLayer();
if (parent() && !needsLayout() && containingBlock()) {
m_layer->setNeedsFullRepaint();
- m_layer->updateLayerPositions();
+ // There is only one layer to update, it is not worth using |cachedOffset| since
+ // we are not sure the value will be used.
+ m_layer->updateLayerPositions(0);
}
}
} else if (layer() && layer()->parent()) {
Modified: trunk/Source/WebCore/rendering/RenderLayer.cpp (94463 => 94464)
--- trunk/Source/WebCore/rendering/RenderLayer.cpp 2011-09-02 23:58:05 UTC (rev 94463)
+++ trunk/Source/WebCore/rendering/RenderLayer.cpp 2011-09-03 00:07:44 UTC (rev 94464)
@@ -253,45 +253,75 @@
#endif
}
-void RenderLayer::updateLayerPositions(UpdateLayerPositionsFlags flags, LayoutPoint* cachedOffset)
+LayoutPoint RenderLayer::computeOffsetFromRoot(bool& hasLayerOffset) const
{
+ hasLayerOffset = true;
+
+ if (!parent())
+ return LayoutPoint();
+
+ // This is similar to root() but we check if an ancestor layer would
+ // prevent the optimization from working.
+ const RenderLayer* rootLayer = 0;
+ for (const RenderLayer* parentLayer = parent(); parentLayer; rootLayer = parentLayer, parentLayer = parentLayer->parent()) {
+ hasLayerOffset = parentLayer->canUseConvertToLayerCoords();
+ if (!hasLayerOffset)
+ return LayoutPoint();
+ }
+ ASSERT(rootLayer == root());
+
+ LayoutPoint offset;
+ parent()->convertToLayerCoords(rootLayer, offset);
+ return offset;
+}
+
+void RenderLayer::updateLayerPositions(LayoutPoint* offsetFromRoot, UpdateLayerPositionsFlags flags)
+{
+#ifndef NDEBUG
+ if (offsetFromRoot) {
+ bool hasLayerOffset;
+ LayoutPoint computedOffsetFromRoot = computeOffsetFromRoot(hasLayerOffset);
+ ASSERT(hasLayerOffset);
+ ASSERT(*offsetFromRoot == computedOffsetFromRoot);
+ }
+#endif
+
updateLayerPosition(); // For relpositioned layers or non-positioned layers,
// we need to keep in sync, since we may have shifted relative
// to our parent layer.
- LayoutPoint oldCachedOffset;
- if (cachedOffset) {
+ LayoutPoint oldOffsetFromRoot;
+ if (offsetFromRoot) {
// We can't cache our offset to the repaint container if the mapping is anything more complex than a simple translation
- bool disableOffsetCache = renderer()->hasColumns() || renderer()->hasTransform() || isComposited();
-#if ENABLE(SVG)
- disableOffsetCache = disableOffsetCache || renderer()->isSVGRoot();
-#endif
- if (disableOffsetCache)
- cachedOffset = 0; // If our cached offset is invalid make sure it's not passed to any of our children
+ if (!canUseConvertToLayerCoords())
+ offsetFromRoot = 0; // If our cached offset is invalid make sure it's not passed to any of our children
else {
- oldCachedOffset = *cachedOffset;
+ oldOffsetFromRoot = *offsetFromRoot;
// Frequently our parent layer's renderer will be the same as our renderer's containing block. In that case,
// we just update the cache using our offset to our parent (which is m_topLeft). Otherwise, regenerated cached
// offsets to the root from the render tree.
if (!m_parent || m_parent->renderer() == renderer()->containingBlock())
- cachedOffset->move(m_topLeft.x(), m_topLeft.y()); // Fast case
+ offsetFromRoot->move(m_topLeft.x(), m_topLeft.y()); // Fast case
else {
LayoutPoint offset;
convertToLayerCoords(root(), offset);
- *cachedOffset = offset;
+ *offsetFromRoot = offset;
}
}
}
LayoutPoint offset;
- if (cachedOffset) {
- offset = *cachedOffset;
+ if (offsetFromRoot) {
+ offset = *offsetFromRoot;
#ifndef NDEBUG
- LayoutPoint nonCachedOffset;
- convertToLayerCoords(root(), nonCachedOffset);
- ASSERT(offset == nonCachedOffset);
+ LayoutPoint computedOffsetFromRoot;
+ convertToLayerCoords(root(), computedOffsetFromRoot);
+ ASSERT(offset == computedOffsetFromRoot);
#endif
- } else
+ } else {
+ // FIXME: It looks suspicious to call convertToLayerCoords here
+ // as canUseConvertToLayerCoords may be true for an ancestor layer.
convertToLayerCoords(root(), offset);
+ }
positionOverflowControls(toSize(offset));
updateVisibilityStatus();
@@ -344,7 +374,7 @@
flags |= UpdatePagination;
for (RenderLayer* child = firstChild(); child; child = child->nextSibling())
- child->updateLayerPositions(flags, cachedOffset);
+ child->updateLayerPositions(offsetFromRoot, flags);
#if USE(ACCELERATED_COMPOSITING)
if ((flags & UpdateCompositingLayers) && isComposited())
@@ -355,8 +385,8 @@
if (m_marquee)
m_marquee->updateMarqueePosition();
- if (cachedOffset)
- *cachedOffset = oldCachedOffset;
+ if (offsetFromRoot)
+ *offsetFromRoot = oldOffsetFromRoot;
}
LayoutRect RenderLayer::repaintRectIncludingDescendants() const
@@ -1101,6 +1131,8 @@
// Remove us from the parent.
RenderLayer* parent = m_parent;
RenderLayer* nextSib = nextSibling();
+ bool hasLayerOffset;
+ const LayoutPoint offsetFromRootBeforeMove = computeOffsetFromRoot(hasLayerOffset);
parent->removeChild(this);
if (reflection())
@@ -1113,7 +1145,10 @@
removeChild(current);
parent->addChild(current, nextSib);
current->setNeedsFullRepaint();
- current->updateLayerPositions(); // Depends on hasLayer() already being false for proper layout.
+ LayoutPoint offsetFromRoot = offsetFromRootBeforeMove;
+ // updateLayerPositions depends on hasLayer() already being false for proper layout.
+ ASSERT(!renderer()->hasLayer());
+ current->updateLayerPositions(hasLayerOffset ? &offsetFromRoot : 0);
current = next;
}
@@ -2799,6 +2834,8 @@
return;
LayoutPoint layerOffset;
+ // FIXME: It looks suspicious to call convertToLayerCoords here
+ // as canUseConvertToLayerCoords is true for this layer.
columnBlock->layer()->convertToLayerCoords(rootLayer, layerOffset);
bool isHorizontal = columnBlock->style()->isHorizontalWritingMode();
Modified: trunk/Source/WebCore/rendering/RenderLayer.h (94463 => 94464)
--- trunk/Source/WebCore/rendering/RenderLayer.h 2011-09-02 23:58:05 UTC (rev 94463)
+++ trunk/Source/WebCore/rendering/RenderLayer.h 2011-09-03 00:07:44 UTC (rev 94464)
@@ -308,7 +308,10 @@
UpdatePagination = 1 << 3
};
typedef unsigned UpdateLayerPositionsFlags;
- void updateLayerPositions(UpdateLayerPositionsFlags = CheckForRepaint | IsCompositingUpdateRoot | UpdateCompositingLayers, LayoutPoint* cachedOffset = 0);
+ static const UpdateLayerPositionsFlags defaultFlags = CheckForRepaint | IsCompositingUpdateRoot | UpdateCompositingLayers;
+ // Providing |cachedOffset| prevents a outlineBoxForRepaint from walking back to the root for each layer in our subtree.
+ // This is an optimistic optimization that is not guaranteed to succeed.
+ void updateLayerPositions(LayoutPoint* offsetFromRoot, UpdateLayerPositionsFlags = defaultFlags);
void updateTransform();
@@ -401,6 +404,9 @@
void updateHoverActiveState(const HitTestRequest&, HitTestResult&);
+ // WARNING: This method returns the offset for the parent as this is what updateLayerPositions expects.
+ LayoutPoint computeOffsetFromRoot(bool& hasLayerOffset) const;
+
// Return a cached repaint rect, computed relative to the layer renderer's containerForRepaint.
LayoutRect repaintRect() const { return m_repaintRect; }
LayoutRect repaintRectIncludingDescendants() const;
@@ -628,6 +634,16 @@
LayoutUnit overflowLeft() const;
LayoutUnit overflowRight() const;
+ bool canUseConvertToLayerCoords() const
+ {
+ // These RenderObject have an impact on their layers' without them knowing about it.
+ return !renderer()->hasColumns() && !renderer()->hasTransform() && !isComposited()
+#if ENABLE(SVG)
+ && !renderer()->isSVGRoot()
+#endif
+ ;
+ }
+
protected:
RenderBoxModelObject* m_renderer;
