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.
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.
Created attachment 119520 [details] The patch that fixes all
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.
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.
(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.
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.
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.
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.
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
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.
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.
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.
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.
Committed r103020: <http://trac.webkit.org/changeset/103020>
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
(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!