Bug 90342

Created attachment 150323 [details] First cut, not ready for review.

Created attachment 352531 [details] Patch

Created attachment 352572 [details] Patch

Created attachment 352711 [details] Patch

Created attachment 352869 [details] Patch

Created attachment 352870 [details] Patch

Created attachment 354091 [details] Patch

Created attachment 354390 [details] Patch

Created attachment 354396 [details] Patch

Created attachment 354400 [details] Patch

Created attachment 354406 [details] Patch

Comment on attachment 354406 [details] Patch Attachment 354406 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9931946 New failing tests: compositing/video/video-clip-change-src.html

Created attachment 354421 [details] Archive of layout-test-results from ews104 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6

Created attachment 354456 [details] Patch

Created attachment 354475 [details] Patch

Created attachment 354491 [details] Patch

Attachment 354491 [details] did not pass style-queue: ERROR: Source/WebCore/rendering/RenderLayer.h:397: Line contains only semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] Total errors found: 1 in 25 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 354508 [details] Patch

Attachment 354508 [details] did not pass style-queue: ERROR: Source/WebCore/rendering/RenderLayer.h:397: Line contains only semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] Total errors found: 1 in 29 files If any of these errors are false positives, please file a bug against check-webkit-style.

*** Bug 81239 has been marked as a duplicate of this bug. ***

Comment on attachment 354508 [details] Patch Attachment 354508 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/9950641 New failing tests: compositing/filters/sw-layer-overlaps-hw-shadow.html

Created attachment 354510 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6

Failing test just needs iOS results.

Comment on attachment 354508 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354508&action=review r=me, nice! > Source/WebCore/ChangeLog:33 > + These changes fix a correctness issue where transform changes now trigger overlap re-evaluation, which causes > + more layer geometry updates than before. This regressed the MotionMark "Focus" test, when geometry updates > + triggered layer resizes as the filter blur radius changed, which then triggered repaints. This is fixed by > + excluding composited filters from the composited bounds (but still taking them into account for overlap). Do we have some performance tests that progress? > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2162 > + if (madeLayer) { > updateBackdropFiltersRect(); > + noteSublayersChanged(DontScheduleFlush); > + } Why not schedule a flush? > Source/WebCore/rendering/RenderLayer.cpp:279 > , m_forcedStackingContext(rendererLayerModelObject.isMedia()) > , m_zOrderListsDirty(false) > , m_normalFlowListDirty(true) > + , m_hadNegativeZOrderList(false) > , m_inResizeMode(false) > , m_scrollDimensionsDirty(true) > , m_hasSelfPaintingLayerDescendant(false) Would be nice to turn these into normal bools (or OptionSets) at some point. Pretty sure memory optimization from bitfields is not meaningful. > Source/WebCore/rendering/RenderLayer.cpp:617 > + RenderLayer* layer = parent(); auto* > Source/WebCore/rendering/RenderLayer.h:197 > + enum CompositingStateFlag { You might consider making this 'enum class CompositingState', 'CompositingFlag' or just 'Compositing' and dropping the then redundant word 'Compositing' from the value names. > Source/WebCore/rendering/RenderLayer.h:256 > + bool needsPostLayoutCompositingUpdate() const { return m_compositingDirtyBits.contains(NeedsPostLayoutCompositingUpdate); } Not sure how all these accessors are better than making the enum public and exposing m_compositingDirtyBits via const accessor. They are not used that much. > Source/WebCore/rendering/RenderLayer.h:261 > + void setNeedsPostLayoutCompositingUpdate() > + { > + m_compositingDirtyBits.add(NeedsPostLayoutCompositingUpdate); > + setAncestorsHaveCompositingDirtyFlag(HasDescendantNeedingCompositingRequirementsTraversal); > + } Do all these really need to be in the header? It is already big. > Source/WebCore/rendering/RenderLayerBacking.cpp:629 > } else > m_artificiallyInflatedBounds = false; > > - setCompositedBounds(layerBounds); > + return setCompositedBounds(layerBounds); Could m_artificiallyInflatedBounds change even when the computed bounds stay the same? Should we return true in that case? > Source/WebCore/rendering/RenderLayerBacking.cpp:661 > +// This can only update things that don't require up-to-date layout. > +void RenderLayerBacking::updateConfigurationAfterStyleChange() updateConfigurationAfterStyleChangeWithoutRequiringUpToDateLayout? :D > Source/WebCore/rendering/RenderLayerCompositor.cpp:227 > + , fullPaintOrderTraversalRequired(false) > + , descendantsRequireCompositingUpdate(false) > , ancestorHasTransformAnimation(false) These could be switched to modern initialization. > Source/WebCore/rendering/RenderLayerCompositor.cpp:657 > +// Returns true on a successful update. > bool RenderLayerCompositor::updateCompositingLayers(CompositingUpdateType updateType, RenderLayer* updateRoot) enum return value would avoid the comment. > Source/WebCore/rendering/RenderLayerCompositor.cpp:661 > + UNUSED_PARAM(updateType); > + UNUSED_PARAM(updateRoot); Neither of these parameters is unused. > Source/WebCore/rendering/RenderLayerCompositor.cpp:691 > + // Scrolling can affect overlap. FIXME: avoid for page scrolling. > + updateRoot->setDescendantsNeedCompositingRequirementsTraversal(); Sounds like a big FIXME! > Source/WebCore/rendering/RenderLayerCompositor.cpp:697 > + if (!updateRoot->hasDescendantNeedingCompositingRequirementsTraversal() && !m_compositing) { > + LOG_WITH_STREAM(Compositing, stream << " no compositing work to do"); > return false; > + } Why is this case considered 'unsuccessful'... > Source/WebCore/rendering/RenderLayerCompositor.cpp:702 > + if (!updateRoot->needsAnyCompositingTraversal()) { > + LOG_WITH_STREAM(Compositing, stream << " updateRoot has no dirty child and doesn't need update"); > + return true; > + } while this is considered 'successful'. > Source/WebCore/rendering/RenderLayerCompositor.cpp:802 > + compositingState.fullPaintOrderTraversalRequired |= layer.needsCompositingRequirementsTraversal(); > + compositingState.descendantsRequireCompositingUpdate |= layer.descendantsNeedCompositingRequirementsTraversal(); Using bit operators like this is slightly ugly. > Source/WebCore/rendering/RenderLayerCompositor.cpp:1019 > +// We have to traverse unchanged layers to fill in the overlap map. > +void RenderLayerCompositor::traverseUnchangedSubtree(RenderLayer* ancestorLayer, RenderLayer& layer, OverlapMap& overlapMap, CompositingState& compositingState, bool& descendantHas3DTransform) Can results of this traversal be cached? Presumably the results don't change if the layers are unchanged. > Source/WebCore/rendering/RenderLayerCompositor.cpp:1252 > +void RenderLayerCompositor::layerBecameNonComposited(const RenderLayer& layer) > +{ > + // Inform the inspector that the given RenderLayer was destroyed. > + InspectorInstrumentation::renderLayerDestroyed(&page(), layer); Something here seems like a lie. Was the layer destroyed or did it just become non-composited? > Source/WebCore/rendering/RenderLayerCompositor.h:142 > + struct CompositingQueryData { This seems to introduce concept of "compositing query" but there are no functions or anything else referencing this concept, so it is bit difficult to understand how it is used. > Source/WebCore/rendering/RenderLayerCompositor.h:144 > + RenderLayer::ViewportConstrainedNotCompositedReason nonCompositedForPositionReason { RenderLayer::NoNotCompositedReason }; NoNotCompositedReason :( > Source/WebCore/rendering/RenderLayerCompositor.h:351 > + bool needsToBeComposited(const RenderLayer&, CompositingQueryData&) const; > // Whether the layer has an intrinsic need for compositing layer. > - bool requiresCompositingLayer(const RenderLayer&, RenderLayer::ViewportConstrainedNotCompositedReason* = nullptr) const; > + bool requiresCompositingLayer(const RenderLayer&, CompositingQueryData&) const; It is bit unclear if CompositingQueryData is an input, output or both for number of functions that take it. Could it be a (perhaps optional) return value when it is a pure output? Many clients initialize it in stack and invoke some function without doing anything between. > Source/WebCore/rendering/RenderLayerCompositor.h:379 > + enum class UpdateLevel { > + AllDescendants = 1 << 0, > + CompositedChildren = 1 << 1, > + }; > // Recurses down the tree, parenting descendant compositing layers and collecting an array of child layers for the current compositing layer. > - void rebuildCompositingLayerTree(RenderLayer&, Vector<Ref<GraphicsLayer>>& childGraphicsLayersOfEnclosingLayer, int depth); > + void updateBackingAndHierarchy(RenderLayer&, Vector<Ref<GraphicsLayer>>& childGraphicsLayersOfEnclosingLayer, OptionSet<UpdateLevel> = { }, int depth = 0); Why OptionSet? Doesn't 'AllDescendants' also cover 'CompositedChildren'? The logic might read better with a regular 3-value enum.

Comment on attachment 354508 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354508&action=review > Source/WebCore/rendering/RenderLayer.cpp:690 > + if (hasNegativeZOrderList != m_hadNegativeZOrderList) { > + m_hadNegativeZOrderList = hasNegativeZOrderList; The bit seems the reflect the current state so maybe m_has instead of m_had? > Source/WebCore/rendering/RenderLayer.h:1180 > + OptionSet<CompositingStateFlag> m_compositingDirtyBits; Are these "state flags" or "dirty bits"? It would be nice if the type name and the field name matched.

(In reply to Antti Koivisto from comment #97) > Comment on attachment 354508 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=354508&action=review > > r=me, nice! > > > Source/WebCore/ChangeLog:33 > > + These changes fix a correctness issue where transform changes now trigger overlap re-evaluation, which causes > > + more layer geometry updates than before. This regressed the MotionMark "Focus" test, when geometry updates > > + triggered layer resizes as the filter blur radius changed, which then triggered repaints. This is fixed by > > + excluding composited filters from the composited bounds (but still taking them into account for overlap). > > Do we have some performance tests that progress? Yes, MotionMark shows it on the Focus test. > > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2162 > > + if (madeLayer) { > > updateBackdropFiltersRect(); > > + noteSublayersChanged(DontScheduleFlush); > > + } > > Why not schedule a flush? We're already inside a flush here. > > Source/WebCore/rendering/RenderLayer.cpp:279 > > , m_forcedStackingContext(rendererLayerModelObject.isMedia()) > > , m_zOrderListsDirty(false) > > , m_normalFlowListDirty(true) > > + , m_hadNegativeZOrderList(false) > > , m_inResizeMode(false) > > , m_scrollDimensionsDirty(true) > > , m_hasSelfPaintingLayerDescendant(false) > > Would be nice to turn these into normal bools (or OptionSets) at some point. > Pretty sure memory optimization from bitfields is not meaningful. Some pages can have thousands of RenderLayers, so I think some amount of space optimization is worthwhile. C++17 (?) gives us initializers for bitfields. > > Source/WebCore/rendering/RenderLayer.h:197 > > + enum CompositingStateFlag { > > You might consider making this 'enum class CompositingState', > 'CompositingFlag' or just 'Compositing' and dropping the then redundant word > 'Compositing' from the value names. > > > Source/WebCore/rendering/RenderLayer.h:256 > > + bool needsPostLayoutCompositingUpdate() const { return m_compositingDirtyBits.contains(NeedsPostLayoutCompositingUpdate); } > > Not sure how all these accessors are better than making the enum public and > exposing m_compositingDirtyBits via const accessor. They are not used that > much. I tried it the other way, but this way makes the code using the accessors more compact. Maybe grouping all the accessors together would make this header less noisy. > > Source/WebCore/rendering/RenderLayer.h:261 > > + void setNeedsPostLayoutCompositingUpdate() > > + { > > + m_compositingDirtyBits.add(NeedsPostLayoutCompositingUpdate); > > + setAncestorsHaveCompositingDirtyFlag(HasDescendantNeedingCompositingRequirementsTraversal); > > + } > > Do all these really need to be in the header? It is already big. I shrunk things down a bit. > > Source/WebCore/rendering/RenderLayerBacking.cpp:629 > > } else > > m_artificiallyInflatedBounds = false; > > > > - setCompositedBounds(layerBounds); > > + return setCompositedBounds(layerBounds); > > Could m_artificiallyInflatedBounds change even when the computed bounds stay > the same? Should we return true in that case? I think it's OK; all that matters whether the composited bounds changed (whether or not they were inflated). > > Source/WebCore/rendering/RenderLayerCompositor.cpp:227 > > + , fullPaintOrderTraversalRequired(false) > > + , descendantsRequireCompositingUpdate(false) > > , ancestorHasTransformAnimation(false) > > These could be switched to modern initialization. Done > > Source/WebCore/rendering/RenderLayerCompositor.cpp:691 > > + // Scrolling can affect overlap. FIXME: avoid for page scrolling. > > + updateRoot->setDescendantsNeedCompositingRequirementsTraversal(); > > Sounds like a big FIXME! Yeah, filed bug 191546 to track. > > Source/WebCore/rendering/RenderLayerCompositor.cpp:697 > > + if (!updateRoot->hasDescendantNeedingCompositingRequirementsTraversal() && !m_compositing) { > > + LOG_WITH_STREAM(Compositing, stream << " no compositing work to do"); > > return false; > > + } > > Why is this case considered 'unsuccessful'... Fixed. > > Source/WebCore/rendering/RenderLayerCompositor.cpp:802 > > + compositingState.fullPaintOrderTraversalRequired |= layer.needsCompositingRequirementsTraversal(); > > + compositingState.descendantsRequireCompositingUpdate |= layer.descendantsNeedCompositingRequirementsTraversal(); > > Using bit operators like this is slightly ugly. Is there a nicer way? > > Source/WebCore/rendering/RenderLayerCompositor.cpp:1019 > > +// We have to traverse unchanged layers to fill in the overlap map. > > +void RenderLayerCompositor::traverseUnchangedSubtree(RenderLayer* ancestorLayer, RenderLayer& layer, OverlapMap& overlapMap, CompositingState& compositingState, bool& descendantHas3DTransform) > > Can results of this traversal be cached? Presumably the results don't change > if the layers are unchanged. We'd have to keep the overlap map around, and then know how to dirty it. > > > Source/WebCore/rendering/RenderLayerCompositor.cpp:1252 > > +void RenderLayerCompositor::layerBecameNonComposited(const RenderLayer& layer) > > +{ > > + // Inform the inspector that the given RenderLayer was destroyed. > > + InspectorInstrumentation::renderLayerDestroyed(&page(), layer); > > Something here seems like a lie. Was the layer destroyed or did it just > become non-composited? Agreed. Filed bug 191547. > > Source/WebCore/rendering/RenderLayerCompositor.h:142 > > + struct CompositingQueryData { > > This seems to introduce concept of "compositing query" but there are no > functions or anything else referencing this concept, so it is bit difficult > to understand how it is used. Yeah. The "query" is the "do you need to composite" question. This struct really just wraps up a bunch of in and out params to these functions. I'll rename this to RequiresCompositingData. > > Source/WebCore/rendering/RenderLayerCompositor.h:144 > > + RenderLayer::ViewportConstrainedNotCompositedReason nonCompositedForPositionReason { RenderLayer::NoNotCompositedReason }; > > NoNotCompositedReason :( Not new :| > > Source/WebCore/rendering/RenderLayerCompositor.h:351 > > + bool needsToBeComposited(const RenderLayer&, CompositingQueryData&) const; > > // Whether the layer has an intrinsic need for compositing layer. > > - bool requiresCompositingLayer(const RenderLayer&, RenderLayer::ViewportConstrainedNotCompositedReason* = nullptr) const; > > + bool requiresCompositingLayer(const RenderLayer&, CompositingQueryData&) const; > > It is bit unclear if CompositingQueryData is an input, output or both for > number of functions that take it. Could it be a (perhaps optional) return > value when it is a pure output? Many clients initialize it in stack and > invoke some function without doing anything between. It has both inputs and outputs. > > Source/WebCore/rendering/RenderLayerCompositor.h:379 > > + enum class UpdateLevel { > > + AllDescendants = 1 << 0, > > + CompositedChildren = 1 << 1, > > + }; > > // Recurses down the tree, parenting descendant compositing layers and collecting an array of child layers for the current compositing layer. > > - void rebuildCompositingLayerTree(RenderLayer&, Vector<Ref<GraphicsLayer>>& childGraphicsLayersOfEnclosingLayer, int depth); > > + void updateBackingAndHierarchy(RenderLayer&, Vector<Ref<GraphicsLayer>>& childGraphicsLayersOfEnclosingLayer, OptionSet<UpdateLevel> = { }, int depth = 0); > > Why OptionSet? Doesn't 'AllDescendants' also cover 'CompositedChildren'? The > logic might read better with a regular 3-value enum. AllDescendants trumps CompositedChildren, yes. With a 3-state, there would be more if-then code when toggling CompositedChildren.

(In reply to Antti Koivisto from comment #98) > Comment on attachment 354508 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=354508&action=review > > > Source/WebCore/rendering/RenderLayer.cpp:690 > > + if (hasNegativeZOrderList != m_hadNegativeZOrderList) { > > + m_hadNegativeZOrderList = hasNegativeZOrderList; > > The bit seems the reflect the current state so maybe m_has instead of m_had? It's "had" in the sense that last time we updated z-order lists, we saw negative z children, but after lists are dirtied (which clears them), we are in a limbo state. > > Source/WebCore/rendering/RenderLayer.h:1180 > > + OptionSet<CompositingStateFlag> m_compositingDirtyBits; > > Are these "state flags" or "dirty bits"? It would be nice if the type name > and the field name matched. It's OptionSet<Compositing> m_compositingDirtyBits now. "state" implies information about the steady state of the layer; these are dirty bits.

> > Using bit operators like this is slightly ugly. > > Is there a nicer way? Both if() and operator|| avoid unnecessary evaluation of the right side (though it doesn't matter in practice here).

This breaks release builds with logging enabled. I've noticed a few times now that there's some confusion about when to use if !LOG_DISABLED, ifndef NDEBUG and now in this patch if ENABLE(TREE_DEBUGGING). We need a bot building release with logging enabled to catch these imo, I know I'm not the only one building like this, because full debug builds are too slow on my hardware :). Could someone have a look at 191583 where I've applied some build fixes for release+logging