Bug 74646 – sizeof(RenderObject) is 32 instead of 24 on Windows

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Ryosuke Niwa]

Unlike gcc and clang, MSVC pads each consecutive member variables of the same type
in bitfields. e.g. if you have:
sturct AB {
unsigned m_1 : 31;
bool m_2 : 1;
}
then MSVC pads m_1 and allocates sizeof(unsigned) * 2 for AB whereas gcc and clang
only allocate sizeof(unsigned) * 1 for AB.

As a result, RenderObject consumes 44 bytes instead of 32 bytes when compiled on Windows by cl.exe.

[Ryosuke Niwa]

Created attachment 119498 [details]
work in progress

This one wraps bitfields in new struct with accessors. This mitigates the implicit coercion problem Darin pointed out. I used macro because the idea of repeating the same code 31 times was just absurd.

[Ryosuke Niwa]

Created attachment 119520 [details]
The patch that fixes all

[Ryosuke Niwa]

On my second thought, none of the existing callers does dangerous assignment so we could just make them all unsigned. However, that doesn't prevent people from making the said coercion mistakes in the future. I'm fine with either approach.

[Darin Adler]

Comment on attachment 119520 [details]
The patch that fixes all

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

Looks OK.

> Source/WebCore/rendering/RenderObject.h:899
> +#define ADD_BOOLEAN_BITFIELD(name, Name) \

Should probably #undef ADD_BOOLEAN_BITFIELD after the code that uses it since it’s in a header.

> Source/WebCore/rendering/RenderObject.h:904
> +        ALWAYS_INLINE bool name() const { return m_##name; }\
> +        ALWAYS_INLINE void set##Name(bool name) { m_##name = name; }\

I think ALWAYS_INLINE is overkill. Normal inlining should be OK.

> Source/WebCore/rendering/RenderObject.h:973
> +        // These bitfields are moved here from subclasses to pack them together
> +        // from RenderBlock

This “from RenderBlock” looks like a continuation of the comment on the line before. A period would make it better. A blank line also would help.

[Darin Adler]

(In reply to comment #3)
> On my second thought, none of the existing callers does dangerous assignment so we could just make them all unsigned. However, that doesn't prevent people from making the said coercion mistakes in the future. I'm fine with either approach.

I also am fine with either approach.

[Ryosuke Niwa]

Comment on attachment 119520 [details]
The patch that fixes all

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

>> Source/WebCore/rendering/RenderObject.h:973
>> +        // from RenderBlock
> 
> This “from RenderBlock” looks like a continuation of the comment on the line before. A period would make it better. A blank line also would help.

As far as I understand it, "from RenderBlock" is part of the sentence.

[Ryosuke Niwa]

Comment on attachment 119520 [details]
The patch that fixes all

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

>> Source/WebCore/rendering/RenderObject.h:899
>> +#define ADD_BOOLEAN_BITFIELD(name, Name) \
> 
> Should probably #undef ADD_BOOLEAN_BITFIELD after the code that uses it since it’s in a header.

Done.

>> Source/WebCore/rendering/RenderObject.h:904
>> +        ALWAYS_INLINE void set##Name(bool name) { m_##name = name; }\
> 
> I think ALWAYS_INLINE is overkill. Normal inlining should be OK.

Okay. Removed.

[Ryosuke Niwa]

Yet another approach is to make setters/getters on RenderObject be friends of RenderObjectBitfields instead of having getters and setters on RenderObjectBitfields itself. This reduces the code redirections and will enforce that m_bitfields is never accessed directly.

[WebKit Review Bot]

Comment on attachment 119520 [details]
The patch that fixes all

Attachment 119520 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10895483

New failing tests:
animations/animation-controller-drt-api.html
animations/3d/transform-origin-vs-functions.html
accessibility/aria-checkbox-checked.html
http/tests/appcache/access-via-redirect.php
http/tests/appcache/auth.html
animations/animation-css-rule-types.html
animations/3d/matrix-transform-type-animation.html
animations/3d/state-at-end-event-transform.html
http/tests/appcache/credential-url.html
http/tests/appcache/crash-when-navigating-away-then-back.html
accessibility/anonymous-render-block-in-continuation-causes-crash.html
accessibility/adjacent-continuations-cause-assertion-failure.html
animations/additive-transform-animations.html
animations/3d/change-transform-in-end-event.html
animations/3d/replace-filling-transform.html
http/tests/appcache/cyrillic-uri.html
accessibility/anchor-linked-anonymous-block-crash.html
animations/animation-direction-normal.html
accessibility/aria-checkbox-sends-notification.html
animations/animation-add-events-in-handler.html

[Darin Adler]

Comment on attachment 119520 [details]
The patch that fixes all

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

>>> Source/WebCore/rendering/RenderObject.h:973
>>> +        // from RenderBlock
>> 
>> This “from RenderBlock” looks like a continuation of the comment on the line before. A period would make it better. A blank line also would help.
> 
> As far as I understand it, "from RenderBlock" is part of the sentence.

Here are the three reasons I did not think it was part of the sentence.

1) There is no period after RenderBlock.
2) This has a similar format to the “for RootInlineBox” comment in InlineBox.h.
3) The phrase “pack them together from RenderBlock” is nonsense.

[Ryosuke Niwa]

Turns out that the comment is out-dated. They're moved from RenderBlock:
http://trac.webkit.org/changeset/40312/trunk/WebCore/rendering/RenderObject.h

and RenderFlowThread:
http://trac.webkit.org/changeset/96408/trunk/Source/WebCore/rendering/RenderObject.h

I'll update the comment.

[Ryosuke Niwa]

Comment on attachment 119520 [details]
The patch that fixes all

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

> Source/WebCore/rendering/RenderObject.cpp:666
> +    setPreferredLogicalWidthsDirty(b);

Obviously, this would result in a recursive call :(

Will fix before l landing it.

> Source/WebCore/rendering/RenderObject.cpp:683
> -        o->m_preferredLogicalWidthsDirty = true;
> +        o->setPreferredLogicalWidthsDirty(true);

Also this one is calling the wrong function.

> Source/WebCore/rendering/RenderObject.h:1037
> +    bool alreadyNeededLayout = needsLayout();
> +    setNeedsLayout(b);

And this one.

[Ryosuke Niwa]

Comment on attachment 119520 [details]
The patch that fixes all

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

> Source/WebCore/rendering/RenderObject.h:488
> +        return m_bitfields.needsLayout() || normalChildNeedsLayout() || posChildNeedsLayout()
> +            || needsSimplifiedNormalFlowLayout() || needsPositionedMovementLayout();

These

> Source/WebCore/rendering/RenderObject.h:496
> +        return needsPositionedMovementLayout() && !needsLayout() && !normalChildNeedsLayout()
> +            && !posChildNeedsLayout() && !needsSimplifiedNormalFlowLayout();

And these also need m_bitfilds. at the front :(  Don't know why I was so careless.

[Ryosuke Niwa]

Committed r103020: <http://trac.webkit.org/changeset/103020>

[Tony Chang]

It looks like this reduced the memory by 1-5% on the various Chromium Win page cyclers:

http://build.chromium.org/f/chromium/perf/vista-release-webkit-latest/moz/report.html?history=150&rev=114800&graph=vm_peak_r

http://build.chromium.org/f/chromium/perf/vista-release-webkit-latest/intl1/report.html?history=150&rev=114800&graph=vm_peak_r

http://build.chromium.org/f/chromium/perf/vista-release-webkit-latest/intl2/report.html?history=150&rev=114800&graph=vm_peak_r

[Ryosuke Niwa]

(In reply to comment #15)
> It looks like this reduced the memory by 1-5% on the various Chromium Win page cyclers:
> 
> http://build.chromium.org/f/chromium/perf/vista-release-webkit-latest/moz/report.html?history=150&rev=114800&graph=vm_peak_r
> 
> http://build.chromium.org/f/chromium/perf/vista-release-webkit-latest/intl1/report.html?history=150&rev=114800&graph=vm_peak_r
> 
> http://build.chromium.org/f/chromium/perf/vista-release-webkit-latest/intl2/report.html?history=150&rev=114800&graph=vm_peak_r

Very nice!