For core classes like these we should COMPILE_ASSERT their size. We do this for many, but not all of these classes. On Thu, Mar 29, 2012 at 1:21 AM, Ryosuke Niwa <rniwa@webkit.org> wrote:

Hi,

Unlike gcc and clang, MSVC pads each consecutive member variables of the same type in bitfields. e.g. if you have: struct 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.

This is not a compiler bug. It's a spec. compliant behavior, and may in fact have a better run-time performance in some situations. However, for core objects like RenderObject and InlineBox, allocating extra 4-8 bytes per each object is prohibitory expensive.

In such cases, please use the same POD type for all bitfield member variables. (Storage class classifiers and variable qualifiers seem to have no effect on how variables are packed; e.g. mutable, const, etc...). For example, MSVC will allocate sizeof(unsigned) * 1 for AB if we rewrite the above code as: struct AB {     unsigned m_1 : 31;     unsigned m_2 : 1; }

When you're making this change, be sure to audit all code that assigns a non-boolean value to m_2 because implicit type coercion into boolean is no longer in effect. For example,

AB ab; ab.m_2 = 2; puts(ab.m_2 ? "true" : "false");

will print "true" before the change and will print "false" after the change. An easy way to ensure you've audited all such code is to add getters and setters for all bitfield member variables or wrap them in a special structure or class as done in http://trac.webkit.org/changeset/103353 and http://trac.webkit.org/changeset/104254.

Best regards, Ryosuke Niwa Software Engineer Google Inc.

_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Hi all, I'm going to resurrect this thread from 2012 and suggest that we introduce a new coding style guide as it came up in https://webkit.org/b/216069. It looks like we added the rule to only use signed or unsigned as the underlying type of bit fields to our style checker in https://trac.webkit.org/r87771 without much discussion on webkit-dev. The problem of this rule is that the type of bit fields determine the size of the padding in most compilers such as GCC and clang. In the following example, sizeof(U) is 4 but sizeof(C) is 1. struct U { unsigned x : 1; }; struct C { unsigned char x : 1; }; The rule I propose instead is as follows: Consecutive bit fields must use the same type. *Right*: struct S { uint8_t count : 7; uint8_t valid : 1; } struct C { uint32_t foo : 30; uint32_t bar : 2; } *Wrong*: struct S { uint8_t count : 7; bool valid : 1; } struct C { uint32_t foo : 30; uint8_t bar : 2; } - R. Niwa On Thu, Mar 29, 2012 at 1:21 AM Ryosuke Niwa <rniwa@webkit.org> wrote:

Hi,

Unlike gcc and clang, MSVC pads each consecutive member variables of the same type in bitfields. e.g. if you have: struct 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.

This is *not a compiler bug*. It's a spec. compliant behavior, and may in fact have a better run-time performance in some situations. However, for core objects like RenderObject and InlineBox, allocating extra 4-8 bytes per each object is prohibitory expensive.

In such cases, please *use the same POD type for all bitfield member variables*. (Storage class classifiers and variable qualifiers seem to have no effect on how variables are packed; e.g. mutable, const, etc...). For example, MSVC will allocate sizeof(unsigned) * 1 for AB if we rewrite the above code as: struct AB { unsigned m_1 : 31; unsigned m_2 : 1; }

When you're making this change, *be sure to audit all code that assigns a non-boolean value to m_2* because implicit type coercion into boolean is no longer in effect. For example,

AB ab; ab.m_2 = 2; puts(ab.m_2 ? "true" : "false");

will print "true" before the change and will print "false" after the change. An easy way to ensure you've audited all such code is to add getters and setters for all bitfield member variables or wrap them in a special structure or class as done in http://trac.webkit.org/changeset/103353 and http://trac.webkit.org/changeset/104254.

Best regards, Ryosuke Niwa Software Engineer Google Inc.