Bug 156846 – RenderStyle should not be reference counted

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Antti Koivisto]

RenderStyle refcounts its substructures. We no longer share RenderStyle objects between renderers so there is no reason to refcount the RenderStyles themselves too. Making it a non-refcounted type clarifies ownership relations, reduces branchiness and saves some memory.

[Antti Koivisto]

Created attachment 276909 [details]
wip

[Build Bot]

Comment on attachment 276909 [details]
wip

Attachment 276909 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1195955

New failing tests:
animations/keyframes-comma-separated.html

[Build Bot]

Created attachment 276911 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 276909 [details]
wip

Attachment 276909 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1195975

New failing tests:
animations/keyframes-comma-separated.html

[Build Bot]

Created attachment 276912 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 276909 [details]
wip

Attachment 276909 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1196008

New failing tests:
animations/keyframes-comma-separated.html

[Build Bot]

Created attachment 276914 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Antti Koivisto]

Created attachment 276915 [details]
wip

[Antti Koivisto]

Created attachment 276935 [details]
patch

[Antti Koivisto]

Created attachment 276944 [details]
patch

[Darin Adler]

Comment on attachment 276944 [details]
patch

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2268
> +static inline RenderStyle* computeRenderStyleForProperty(Node* styledNode, PseudoId pseudoElementSpecifier, CSSPropertyID propertyID, std::unique_ptr<RenderStyle>& ownedStyle)

Maybe we should make a class template that supports this use in the future. A union of unique_ptr with a raw pointer that can hold either an owned or unowned pointer. Or we can make a smaller implementation that uses a boolean. Would be nice to not have to add an extra local variable and argument for cases like this. Might even be a way to do this with a unique_ptr and a custom deleter; not sure if the deleter can have 1 bit of state.

[Antti Koivisto]

Created attachment 277143 [details]
patch

[WebKit Commit Bot]

Comment on attachment 277143 [details]
patch

Clearing flags on attachment: 277143

Committed r199964: <http://trac.webkit.org/changeset/199964>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 277143 [details]
patch

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

This makes me wonder why these are heap allocated. Maybe at some point should passing RenderStyle&& and moving them instead of using objects on the heap and moving unique_ptrs around. I guess they are kind of big, though.

> Source/WebCore/css/MediaQueryEvaluator.cpp:88
>  MediaQueryEvaluator::MediaQueryEvaluator(const String& acceptedMediaType, Frame* frame, RenderStyle* style)

Seems like the argument should be RenderStyle&.

> Source/WebCore/css/MediaQueryEvaluator.cpp:91
> +    , m_style(WTFMove(style))

Doesn’t make sense since both style and m_style are a raw pointer. I imagine that was different in an earlier point of the development of this patch.

> Source/WebCore/css/StyleResolver.h:135
>  struct ElementStyle {
> -    ElementStyle(Ref<RenderStyle>&& renderStyle, std::unique_ptr<Style::Relations> relations = { })
> +    ElementStyle(std::unique_ptr<RenderStyle> renderStyle, std::unique_ptr<Style::Relations> relations = { })
>          : renderStyle(WTFMove(renderStyle))
>          , relations(WTFMove(relations))
>      { }
>  
> -    Ref<RenderStyle> renderStyle;
> +    std::unique_ptr<RenderStyle> renderStyle;
>      std::unique_ptr<Style::Relations> relations;
>  };

I suspect this might work better without the constructor, using aggregate syntax to construct this when needed.

> Source/WebCore/dom/Document.cpp:2131
> +    std::unique_ptr<RenderStyle> pageStyle(ensureStyleResolver().styleForPage(pageIndex));

Maybe auto instead?

> Source/WebCore/dom/Document.cpp:2137
> +    std::unique_ptr<RenderStyle> style = ensureStyleResolver().styleForPage(pageIndex);

Maybe auto instead?

> Source/WebCore/dom/PseudoElement.cpp:125
> +        std::unique_ptr<RenderStyle> createdStyle = RenderStyle::createStyleInheritingFromPseudoStyle(renderer.style());

Maybe auto instead?

> Source/WebCore/page/animation/AnimationBase.h:142
> +    virtual void getAnimatedStyle(std::unique_ptr<RenderStyle>& /*animatedStyle*/) = 0;

Why doesn’t this function use a return value?

> Source/WebCore/rendering/RenderButton.cpp:115
>  void RenderButton::setupInnerStyle(RenderStyle* innerStyle) 

Clearly this function should take a reference rather than a pointer.

> Source/WebCore/rendering/RenderElement.cpp:1550
> +    std::unique_ptr<RenderStyle> result = getUncachedPseudoStyle(PseudoStyleRequest(pseudo), parentStyle);

Maybe auto instead?

> Source/WebCore/rendering/RenderElement.cpp:1588
> +    if (std::unique_ptr<RenderStyle> pseudoStyle = selectionPseudoStyle()) {

Maybe auto instead?

> Source/WebCore/rendering/RenderElement.cpp:1633
> +    std::unique_ptr<RenderStyle> pseudoStyle = selectionPseudoStyle();

Maybe auto instead?

> Source/WebCore/rendering/RenderLayerCompositor.cpp:519
> +    std::unique_ptr<RenderStyle> scrollbarStyle = static_cast<RenderScrollbar*>(scrollbar)->getScrollbarPseudoStyle(ScrollbarBGPart, SCROLLBAR);

Maybe auto instead?

> Source/WebCore/rendering/RenderNamedFlowFragment.cpp:467
> +        std::unique_ptr<RenderStyle> objectRegionStyle = RenderStyle::clone(&object->style());
> +        std::unique_ptr<RenderStyle> objectOriginalStyle = RenderStyle::clone(objectPair.value.style.get());

Maybe auto instead?

> Source/WebCore/rendering/RenderScrollbar.cpp:146
> +    std::unique_ptr<RenderStyle> result = owningRenderer()->getUncachedPseudoStyle(PseudoStyleRequest(pseudoId, this, partType), &owningRenderer()->style());

Maybe auto instead?

> Source/WebCore/rendering/RenderScrollbar.cpp:216
> +    std::unique_ptr<RenderStyle> partStyle = getScrollbarPseudoStyle(partType, pseudoForScrollbarPart(partType));

Maybe auto instead?

> Source/WebCore/rendering/style/RenderStyle.h:478
> +#if !ASSERT_DISABLED
> +    bool m_deletionHasBegun { false };
> +#endif

Seems like this could be private.

Also, neat that we have this in RefCounted, but do we really need it in RenderStyle?

[Antti Koivisto]

(In reply to comment #15)
> Comment on attachment 277143 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=277143&action=review
> 
> This makes me wonder why these are heap allocated. Maybe at some point
> should passing RenderStyle&& and moving them instead of using objects on the
> heap and moving unique_ptrs around. I guess they are kind of big, though.

The plan is to move to RenderStyle&& in at least some places. One obvious thing we can do now is to inline RenderStyle to RenderElement, eliminating an allocation and indirection per renderer.

[Antti Koivisto]

> > Source/WebCore/rendering/style/RenderStyle.h:478
> > +#if !ASSERT_DISABLED
> > +    bool m_deletionHasBegun { false };
> > +#endif
> 
> Seems like this could be private.
> 
> Also, neat that we have this in RefCounted, but do we really need it in
> RenderStyle?

I just wanted to check I wasn't making mistakes. It might be unnecessary to keep it around.