Re: [webkit-dev] PSA: Bit fields won't be packed on Windows if you mix types
> On Sep 3, 2020, at 11:14 PM, Fujii Hironori wrote: > > BTW, I don't like to idea adding a new rule, but keeping old style code. It > introduces divergence between the guideline and actual code. Do we really > need a new rule that one doesn’t necessary have to follow? I understand that you don’t like this. But all of our style rules are like this. We don’t apply them across our entire project at the moment we institute them. They guide the future of WebKit. So this is not a valid argument against adding a new rule. You can help us apply a given rule to existing code if you feel strongly in a particular case, and in some cases it might be practical to quickly apply it everywhere relevant. Maybe in this case it is. But that’s not the basis for deciding whether to make it a rule. — Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] PSA: Bit fields won't be packed on Windows if you mix types
On Thu, Sep 3, 2020 at 11:15 PM Fujii Hironori wrote: > > On Fri, Sep 4, 2020 at 2:56 PM Ryosuke Niwa wrote: > >> On Thu, Sep 3, 2020 at 10:11 PM Fujii Hironori >> wrote: >> >>> >>> On Fri, Sep 4, 2020 at 1:31 PM Ryosuke Niwa wrote: >>> Consecutive bit fields must use the same type. >>> >>> RenderLayer is mixing bool and unsigned in the consecutive bit fields. >>> They should use only uint8_t, right? >>> >>> https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/rendering/RenderLayer.h#L1263 >>> >> >> Under the proposed coding style guide, yes, although I highly doubt the >> size of RenderLayer really matters. >> >> > Do you mean your new rule is applicable only for performance > critical parts? > No, I'm saying that applying the proposed guideline wouldn't necessarily help memory usage cause RenderLayer is a really large object anyway. It doesn't mean we shouldn't apply. BTW, I don't like to idea adding a new rule, but keeping old > style code. It introduces divergence between the guideline and > actual code. Do we really need a new rule that one doesn't > necessary have to follow? > We should follow the proposed guideline in new code but we typically don't update existing code to match the new guideline when we introduce a new coding style guideline. - R. Niwa ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] PSA: Bit fields won't be packed on Windows if you mix types
On Fri, Sep 4, 2020 at 2:56 PM Ryosuke Niwa wrote: > On Thu, Sep 3, 2020 at 10:11 PM Fujii Hironori > wrote: > >> >> On Fri, Sep 4, 2020 at 1:31 PM Ryosuke Niwa wrote: >> >>> Consecutive bit fields must use the same type. >>> >> >> RenderLayer is mixing bool and unsigned in the consecutive bit fields. >> They should use only uint8_t, right? >> >> https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/rendering/RenderLayer.h#L1263 >> > > Under the proposed coding style guide, yes, although I highly doubt the > size of RenderLayer really matters. > > Do you mean your new rule is applicable only for performance critical parts? BTW, I don't like to idea adding a new rule, but keeping old style code. It introduces divergence between the guideline and actual code. Do we really need a new rule that one doesn't necessary have to follow? ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] PSA: Bit fields won't be packed on Windows if you mix types
On Thu, Sep 3, 2020 at 10:11 PM Fujii Hironori wrote: > > On Fri, Sep 4, 2020 at 1:31 PM Ryosuke Niwa wrote: > >> Consecutive bit fields must use the same type. >> > > RenderLayer is mixing bool and unsigned in the consecutive bit fields. > They should use only uint8_t, right? > > https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/rendering/RenderLayer.h#L1263 > Under the proposed coding style guide, yes, although I highly doubt the size of RenderLayer really matters. - R. Niwa ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] PSA: Bit fields won't be packed on Windows if you mix types
On Fri, Sep 4, 2020 at 1:31 PM Ryosuke Niwa wrote: > Consecutive bit fields must use the same type. > RenderLayer is mixing bool and unsigned in the consecutive bit fields. They should use only uint8_t, right? https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/rendering/RenderLayer.h#L1263 ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] PSA: Bit fields won't be packed on Windows if you mix types
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 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
https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] PSA: Bit fields won't be packed on Windows if you mix types
Yup. I'm enabling that for InlineBox on Windows in this bug:
https://bugs.webkit.org/show_bug.cgi?id=82578
- Ryosuke
On Thu, Mar 29, 2012 at 1:38 AM, Eric Seidel wrote:
> 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 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
> >
>
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] PSA: Bit fields won't be packed on Windows if you mix types
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 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
>
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
