Bug 216069 – Store all styling flags in m_rendererWithStyleFlags

• Component: DOM
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Right now, there are quite a few node flags that are only used by elements for styling purposes.

Move all the remaining flags there since CSS JIT no longer rely on these flags directly,
and CompactPointerTuple has been expanded to store up to 16 bits of data as opposed to just 8 bits.

[Ryosuke Niwa]

Created attachment 407751 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 407751 [details]
Patch

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

> Source/WebCore/dom/Node.cpp:843
> +        uint16_t flags = m_rendererWithStyleFlags.type();
> +        flags &= ~static_cast<uint16_t>(NodeStyleFlag::StyleValidityMask);
> +        flags |= static_cast<uint16_t>(validity) << s_styleValidityShift;
> +        m_rendererWithStyleFlags.setType(flags);

Can we make this cleaner by defining m_rendererWithStyleFlags differently?

> Source/WebCore/dom/Node.h:603
> +        DynamicChildMatchingFlagMask = (1 << 6) - 1,
> +
> +        ChildNeedsStyleRecalc = 1 << 6,
> +        DirectChildNeedsStyleRecalc = 1 << 7,
> +        StyleRecalcMask = ChildNeedsStyleRecalc | DirectChildNeedsStyleRecalc,
> +
> +        // Bits 8 & 9 are used to store StyleValidity
> +        StyleValidityMask = 3 << s_styleValidityShift,
> +        StyleResolutionShouldRecompositeLayer = 1 << 10,
> +
> +        ChildrenAffectedByFirstChildRules = 1 << 11,
> +        ChildrenAffectedByLastChildRules = 1 << 12,
> +        AffectsNextSiblingElementStyle = 1 << 13,
> +        StyleIsAffectedByPreviousSibling = 1 << 14,
> +        DescendantsAffectedByPreviousSibling = 1 << 15,

This looks pretty messy. Can we separate out the pure settings bits into an OptionSet<>?

[Ryosuke Niwa]

(In reply to Simon Fraser (smfr) from comment #2)
> Comment on attachment 407751 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407751&action=review
> 
> > Source/WebCore/dom/Node.cpp:843
> > +        uint16_t flags = m_rendererWithStyleFlags.type();
> > +        flags &= ~static_cast<uint16_t>(NodeStyleFlag::StyleValidityMask);
> > +        flags |= static_cast<uint16_t>(validity) << s_styleValidityShift;
> > +        m_rendererWithStyleFlags.setType(flags);
> 
> Can we make this cleaner by defining m_rendererWithStyleFlags differently?

What do you mean by “cleaner”?

> > Source/WebCore/dom/Node.h:603
> > +        DynamicChildMatchingFlagMask = (1 << 6) - 1,
> > +
> > +        ChildNeedsStyleRecalc = 1 << 6,
> > +        DirectChildNeedsStyleRecalc = 1 << 7,
> > +        StyleRecalcMask = ChildNeedsStyleRecalc | DirectChildNeedsStyleRecalc,
> > +
> > +        // Bits 8 & 9 are used to store StyleValidity
> > +        StyleValidityMask = 3 << s_styleValidityShift,
> > +        StyleResolutionShouldRecompositeLayer = 1 << 10,
> > +
> > +        ChildrenAffectedByFirstChildRules = 1 << 11,
> > +        ChildrenAffectedByLastChildRules = 1 << 12,
> > +        AffectsNextSiblingElementStyle = 1 << 13,
> > +        StyleIsAffectedByPreviousSibling = 1 << 14,
> > +        DescendantsAffectedByPreviousSibling = 1 << 15,
> 
> This looks pretty messy. Can we separate out the pure settings bits into an
> OptionSet<>?

No, CompactTuplePointer doesn’t support OptionSet. There have been multiple attempts to add the support and all of them got rolled out.

[Ryosuke Niwa]

Created attachment 407841 [details]
Updated per review comments

[Darin Adler]

Comment on attachment 407841 [details]
Updated per review comments

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

> Source/WebCore/dom/Node.h:605
> +        uint16_t toRaw() { return bitwise_cast<uint16_t>(*this); }

const

> Source/WebCore/dom/Node.h:622
> +        uint8_t m_styleValidity : 2;
> +        uint8_t m_dynamicStyleRelation : 6;
> +        uint8_t m_flags : 8;

Why is uint8_t better for this than unsigned? I ask in part because the style script checks and complains.

[Ryosuke Niwa]

(In reply to Darin Adler from comment #5)
> Comment on attachment 407841 [details]
> Updated per review comments
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407841&action=review
> 
> > Source/WebCore/dom/Node.h:605
> > +        uint16_t toRaw() { return bitwise_cast<uint16_t>(*this); }
> 
> const
> 
> > Source/WebCore/dom/Node.h:622
> > +        uint8_t m_styleValidity : 2;
> > +        uint8_t m_dynamicStyleRelation : 6;
> > +        uint8_t m_flags : 8;
> 
> Why is uint8_t better for this than unsigned? I ask in part because the
> style script checks and complains.

Oh, I initially used uint16_t since we don't have 32 bits here and then switched to use uint8_t when I realized I can get away with it. I guess it could be been more clear to use uint16_t since that's what we're using for CompactPointerType? I think it would be confusing for this struct to be using unsigned but only having 16-bit storage underneath.

That style checker warning is there to complain about different types being mixed in bitfields resulting in padding on Windows (cl.exe). Here, the warning doesn't apply because all bitfields use uint8_t. In any case, if there was any padding added, this. code would be totally incorrect.

[Darin Adler]

Can we fix the style checker? It’s checking a rule that’s incorrect.

But also, I don’t agree that "unsigned x : 1" is confusing. To be it says "a 1-bit unsigned value". Maybe to you it says "a 1-bit 32-bit unsigned value"?

[Ryosuke Niwa]

(In reply to Darin Adler from comment #7)
> Can we fix the style checker? It’s checking a rule that’s incorrect.
> 
> But also, I don’t agree that "unsigned x : 1" is confusing. To be it says "a
> 1-bit unsigned value". Maybe to you it says "a 1-bit 32-bit unsigned value"?

Well, sizeof(X) is 4 and sizeof(Y) is 1 so the type used to define the bitfield matters to determine the width of the overall struct. Here, Because I'm trying to bitwise cast this struct with uint16_t, I think it would be confusing to be using unsigned, which has the same width as uint32_t.

struct X {
  unsigned x : 1;
}

struct Y {
  unsigned char x : 1;
}

[Ryosuke Niwa]

(In reply to Darin Adler from comment #7)
> Can we fix the style checker? It’s checking a rule that’s incorrect.

Hm... odd. This rule isn't really listed anywhere on https://webkit.org/code-style-guidelines/. I guess we can update the style checker but we should probably add a rule there as well.

[Darin Adler]

(In reply to Ryosuke Niwa from comment #8)
> Well, sizeof(X) is 4 and sizeof(Y) is 1 so the type used to define the
> bitfield matters to determine the width of the overall struct.

That is something I don’t understand. Why would the underlying type behind a 1-bit bit field affect the width of the structure it’s in? Nothing in the C++ standard suggests that should be done. I seem to remember it as a thing I thought of as a compiler bug in old versions of Visual Studio.

[Ryosuke Niwa]

(In reply to Darin Adler from comment #10)
> (In reply to Ryosuke Niwa from comment #8)
> > Well, sizeof(X) is 4 and sizeof(Y) is 1 so the type used to define the
> > bitfield matters to determine the width of the overall struct.
> 
> That is something I don’t understand. Why would the underlying type behind a
> 1-bit bit field affect the width of the structure it’s in? Nothing in the
> C++ standard suggests that should be done. I seem to remember it as a thing
> I thought of as a compiler bug in old versions of Visual Studio.

I'm not familiar with C++ spec but the aforementioned sizeof behavior is exhibited by both GCC and clang. The issue specific to Visual Studio is padding between bitfields of different underlying types. In the following example, Visual Studio may end up using 8+ bytes as opposed to 4 bytes in clang/GCC because cl.exec will "y" to be 4-byte aligned:

struct {
    unsigned short y : 4;
    unsigned int x : 4;
}

[Ryosuke Niwa]

Anyway, I think I'm gonna land this as with (after const fix). I can change it to unsigned if you're so inclined but I think it's better to stick with uint16_t or uint8_t to keep struct same size as what CompactPointerTuple::type/setType returns/accepts.

[Darin Adler]

(In reply to Ryosuke Niwa from comment #12)
> I can change it to unsigned if you're so inclined

I just want to understand the C++ language and compiler situation/rules and what our guideline is. I won’t insist on anything specific here!

[Ryosuke Niwa]

Committed r266578: <https://trac.webkit.org/changeset/266578>

[Radar WebKit Bug Importer]

<rdar://problem/68325332>

[Ryosuke Niwa]

(In reply to Darin Adler from comment #13)
> (In reply to Ryosuke Niwa from comment #12)
> > I can change it to unsigned if you're so inclined
> 
> I just want to understand the C++ language and compiler situation/rules and
> what our guideline is. I won’t insist on anything specific here!

Okay, I've brought back my old PSA thread from 2012 on webkit-dev to discuss this:
https://lists.webkit.org/pipermail/webkit-dev/2020-September/031376.html