Bug 143432 – Free up some bits in RenderObject by moving rarely used bits into a side table

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Simon Fraser (smfr)]

Free up some bits in RenderObject by moving rarely used bits into a side table

[Simon Fraser (smfr)]

Created attachment 250184 [details]
Patch

[Darin Adler]

Comment on attachment 250184 [details]
Patch

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

No measurable performance cost?

> Source/WebCore/rendering/RenderObject.cpp:2507
> +    if (b) {
> +        ensureRareData().setIsDragging(true);
> +        return;
> +    }
> +    if (hasRareData())
> +        ensureRareData().setIsDragging(false);

Another way to write this is:

    if (b || hasRareData())
        ensureRareData().setIsDragging(b);
I slightly prefer that version. I also slightly prefer a word for the argument such as "isDragging" rather than just "b".

> Source/WebCore/rendering/RenderObject.cpp:2520
> +RenderObject::RareDataHash& RenderObject::rareDataMap()

I suggest making this a file-local thing rather than a static class member; that way it doesn’t have to be in the header at all.

> Source/WebCore/rendering/RenderObject.h:1017
> +            : m_isDragging(false)
> +            , m_hasReflection(false)

If these are going to be public data members, then I suggest we name them without the "m_".

> Source/WebCore/rendering/RenderObject.h:1030
> +    void clearRareData();

I think the right name here is removeRareData.

[Simon Fraser (smfr)]

(In reply to comment #2)
> Comment on attachment 250184 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250184&action=review
> 
> No measurable performance cost?

I didn't measure, but I think it's unlikely. setIsDragging(true) and setIsReflection(true) are rarely called, and the setters with false, and getters when there is no rare data should be as cheap as before.

> > Source/WebCore/rendering/RenderObject.cpp:2507
> > +    if (b) {
> > +        ensureRareData().setIsDragging(true);
> > +        return;
> > +    }
> > +    if (hasRareData())
> > +        ensureRareData().setIsDragging(false);
> 
> Another way to write this is:
> 
>     if (b || hasRareData())
>         ensureRareData().setIsDragging(b);
> I slightly prefer that version. I also slightly prefer a word for the
> argument such as "isDragging" rather than just "b".

Done.

> > Source/WebCore/rendering/RenderObject.cpp:2520
> > +RenderObject::RareDataHash& RenderObject::rareDataMap()
> 
> I suggest making this a file-local thing rather than a static class member;
> that way it doesn’t have to be in the header at all.

Sadly doing that means that RenderObjectRareData can no longer be private to RenderObject, unless I'm missing something.

> > Source/WebCore/rendering/RenderObject.h:1017
> > +            : m_isDragging(false)
> > +            , m_hasReflection(false)
> 
> If these are going to be public data members, then I suggest we name them
> without the "m_".

The macro makes them private.

> > Source/WebCore/rendering/RenderObject.h:1030
> > +    void clearRareData();
> 
> I think the right name here is removeRareData.

Done.

[Simon Fraser (smfr)]

https://trac.webkit.org/r182373

[Antti Koivisto]

We have been trying to make RenderObject less of a general dumping ground so this is somewhat unfortunate. If we need this I think this should have been RenderElementRareData instead. 

The few bits currently here don't really justify the existence of the type. There are plenty of free bits in RenderElement and any RenderObject bits that don't apply to RenderTexts are easily moved there.

[Simon Fraser (smfr)]

When I did this I would have moved the bits into RenderElement if that had been possible.