Bug 143432 - Free up some bits in RenderObject by moving rarely used bits into a side table
Summary: Free up some bits in RenderObject by moving rarely used bits into a side table
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-05 18:01 PDT by Simon Fraser (smfr)
Modified: 2016-03-01 10:28 PST (History)
6 users (show)

See Also:


Attachments
Patch (14.04 KB, patch)
2015-04-05 18:17 PDT, Simon Fraser (smfr)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2015-04-05 18:01:36 PDT
Free up some bits in RenderObject by moving rarely used bits into a side table
Comment 1 Simon Fraser (smfr) 2015-04-05 18:17:21 PDT
Created attachment 250184 [details]
Patch
Comment 2 Darin Adler 2015-04-05 18:22:09 PDT
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.
Comment 3 Simon Fraser (smfr) 2015-04-05 18:42:15 PDT
(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.
Comment 4 Simon Fraser (smfr) 2015-04-05 20:01:14 PDT
https://trac.webkit.org/r182373
Comment 5 Antti Koivisto 2016-03-01 07:10:43 PST
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.
Comment 6 Simon Fraser (smfr) 2016-03-01 10:28:39 PST
When I did this I would have moved the bits into RenderElement if that had been possible.