Bug 216208 – Move all remaining flags from ElementRareData to Node to reduce the frequency

• Component: DOM
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Move the remaining bit flags from ElementRareData to Node::m_nodeFlags to avoid the creation of ElementRareData in more cases.

[Ryosuke Niwa]

We can also dramatically reduce the frequency of the creation of ElementRareData when tabIndex is set if we just special case 0 and -1. I've done some study on various poplar websites, and they also almost use 0 and -1 as follows:

d = {}; document.querySelectorAll('[tabindex]').forEach((e) => { if (e.tabIndex in d) d[e.tabIndex]++; else d[e.tabIndex]=0; });

Gmail
    {0: 176, -1: 1300} = $5

Slack
    {0: 4, 1: 0, -1: 130} = 

Facebook
    {0: 151, -1: 12}

Twitter
    {0: 31}

Quip
    {0: 53}

CNN
    {0: 7, -1: 10}

NYTimes
    {0: 1, 99: 0, 100: 0, -1: 0} 

YouTube
    {0: 59, -1: 186}

[Ryosuke Niwa]

Created attachment 408061 [details]
Patch

[Yusuke Suzuki]

Awesome!

[Ryosuke Niwa]

Created attachment 408063 [details]
Fixed builds

[Darin Adler]

Comment on attachment 408063 [details]
Fixed builds

ElementRareData size assertion is failing on Windows

[Ryosuke Niwa]

Created attachment 408069 [details]
Fixed builds 2

[Darin Adler]

Comment on attachment 408069 [details]
Fixed builds 2

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

Great!

> Source/WebCore/dom/Element.cpp:247
> +        clearFlag(TabIndexIsZeroOrInRareDataFlag);

I think it would be easier to reason about this if it was treated as a 4-value enumeration rather than two flags. The technique is great but the logic is confusing this way.

> Source/WebCore/dom/Element.cpp:269
> +    bool isOne = getFlag(TabIndexIsZeroOrInRareDataFlag);

Did you mean to name this “isZero”?

> Source/WebCore/dom/ElementRareData.h:60
> +    int tabIndex() const { return m_tabIndex; }

The name here isn’t great because this is not the tab index in the 0, -1, or “no tab index” cases. And also 0 is a special value meaning “not stored here”, not a tab index of 0. I think a different name could make some of that clearer. Like maybe “unusual tab index”. And a comment somewhere explaining the special meaning of 0? Maybe an assertion that the getter isn’t called when m_tabIndex is 0?

> Source/WebCore/dom/ElementRareData.h:61
> +    void setTabIndex(int);

Ditto on the name.

> Source/WebCore/dom/ElementRareData.h:161
> +    int m_tabIndex { 0 };

Ditto.

> Source/WebCore/dom/Node.cpp:137
> +    case NodeRareData::UseType::PseudoElements:

Sorting the cases makes sense when an enumeration gets long like this.

> Source/WebCore/dom/Node.h:548
> +#if ENABLE(FULLSCREEN_API)

Would it be better to not put this inside an #if? I can never tell; certainly more conditionals make the code messier.

[Ryosuke Niwa]

(In reply to Darin Adler from comment #7)
> Comment on attachment 408069 [details]
> Fixed builds 2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=408069&action=review
> 
> Great!
> 
> > Source/WebCore/dom/Element.cpp:247
> > +        clearFlag(TabIndexIsZeroOrInRareDataFlag);
> 
> I think it would be easier to reason about this if it was treated as a
> 4-value enumeration rather than two flags. The technique is great but the
> logic is confusing this way.

Okay, I was debating between those two options myself. Let's add an enum class like this:

    static constexpr unsigned s_tabIndexStateBitOffset = 30;

...

        TabIndexStateFirstBit = 1 << s_tabIndexStateBitOffset,

        DefaultNodeFlags = IsParsingChildrenFinishedFlag
    };

    enum class TabIndexState : uint8_t {
        NotSet = 0,
        Zero = 1,
        NegativeOne = 2,
        SetInRareData = 3,
    };

...

    TabIndexState tabIndexState() { return static_cast<TabIndexState>(m_nodeFlags >> s_tabIndexStateBitOffset); }
    void setTabIndexState(TabIndexState state) { return m_nodeFlags = static_cast<uint32_t>(state) << s_tabIndexStateBitOffset; }

> > Source/WebCore/dom/Element.cpp:269
> > +    bool isOne = getFlag(TabIndexIsZeroOrInRareDataFlag);
> 
> Did you mean to name this “isZero”?

Oops, yeah, but this code is rewritten now.

> > Source/WebCore/dom/ElementRareData.h:60
> > +    int tabIndex() const { return m_tabIndex; }
> 
> The name here isn’t great because this is not the tab index in the 0, -1, or
> “no tab index” cases. And also 0 is a special value meaning “not stored
> here”, not a tab index of 0. I think a different name could make some of
> that clearer. Like maybe “unusual tab index”. And a comment somewhere
> explaining the special meaning of 0? Maybe an assertion that the getter
> isn’t called when m_tabIndex is 0?

Sure. This was another thing I was thinking of doing anyway.
I was thinking of "uncommon" but "unusual" works too.
And let's add an assertion as you say in the getter
since I already have an assertion & comment in the setter.

> > Source/WebCore/dom/Node.h:548
> > +#if ENABLE(FULLSCREEN_API)
> 
> Would it be better to not put this inside an #if? I can never tell;
> certainly more conditionals make the code messier.

I'd leave this for now but I agree with this. There is no point in build-guarding every code like this.

[Darin Adler]

Comment on attachment 408069 [details]
Fixed builds 2

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

>>> Source/WebCore/dom/Element.cpp:247
>>> +        clearFlag(TabIndexIsZeroOrInRareDataFlag);
>> 
>> I think it would be easier to reason about this if it was treated as a 4-value enumeration rather than two flags. The technique is great but the logic is confusing this way.
> 
> Okay, I was debating between those two options myself. Let's add an enum class like this:
> 
>     static constexpr unsigned s_tabIndexStateBitOffset = 30;
> 
> ...
> 
>         TabIndexStateFirstBit = 1 << s_tabIndexStateBitOffset,
> 
>         DefaultNodeFlags = IsParsingChildrenFinishedFlag
>     };
> 
>     enum class TabIndexState : uint8_t {
>         NotSet = 0,
>         Zero = 1,
>         NegativeOne = 2,
>         SetInRareData = 3,
>     };
> 
> ...
> 
>     TabIndexState tabIndexState() { return static_cast<TabIndexState>(m_nodeFlags >> s_tabIndexStateBitOffset); }
>     void setTabIndexState(TabIndexState state) { return m_nodeFlags = static_cast<uint32_t>(state) << s_tabIndexStateBitOffset; }

Looks like there is some masking missing from the code you wrote there.

[Ryosuke Niwa]

(In reply to Darin Adler from comment #9)
> Comment on attachment 408069 [details]
> Fixed builds 2
>
> >     TabIndexState tabIndexState() { return static_cast<TabIndexState>(m_nodeFlags >> s_tabIndexStateBitOffset); }
> >     void setTabIndexState(TabIndexState state) { return m_nodeFlags = static_cast<uint32_t>(state) << s_tabIndexStateBitOffset; }
> 
> Looks like there is some masking missing from the code you wrote there.

Indeed. This will obviously not work LOL. I'm uploading a new patch for another round of review since I've changed a lot of things.

[Ryosuke Niwa]

Created attachment 408104 [details]
Updated per Darin's review comments

[Darin Adler]

Comment on attachment 408104 [details]
Updated per Darin's review comments

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

> Source/WebCore/dom/Element.cpp:244
> +void Element::setTabIndexExplicitly(Optional<int> tabIndex)

This function can leave behind a stale leftover unusual tab index stored in the rare data that should never be looked at. I suppose that’s OK.

> Source/WebCore/dom/Element.cpp:274
> +        RELEASE_ASSERT(hasRareData());
> +        return elementRareData()->unusualTabIndex();

Why is this RELEASE_ASSERT needed? Doesn’t seem valuable to me.

> Source/WebCore/dom/ElementRareData.h:233
> +    ASSERT(m_unusualTabIndex && m_unusualTabIndex != -1); // Common values of 0 and -1 are stored as TabIndexState in Node.

The main purpose of this assertion is to catch cases where we are getting the index before it was set. Otherwise, it’s a waste to assert the same thing in both the setter and getter.

So it could assert only the 0 case; it’s not so much checking for an invalid index as checking that we don’t ask for an index when one is not there.

> Source/WebCore/dom/Node.h:557
> +    static constexpr uint32_t s_tabIndexStateBitMask = 3 << s_tabIndexStateBitOffset;

I think this needs to be 3U to get rid of the warning (treated as an error) that is causing the build failure on Windows. Hope that works!

> Source/WebCore/dom/Node.h:563
> +    enum class TabIndexState : uint8_t {
> +        NotSet = 0,
> +        Zero = 1,
> +        NegativeOne = 2,
> +        InRareData = 3,
> +    };

I don’t think you need the four explicit numeric values here.

[Ryosuke Niwa]

(In reply to Darin Adler from comment #12)
> Comment on attachment 408104 [details]
> Updated per Darin's review comments
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=408104&action=review
> 
> > Source/WebCore/dom/Element.cpp:244
> > +void Element::setTabIndexExplicitly(Optional<int> tabIndex)
> 
> This function can leave behind a stale leftover unusual tab index stored in
> the rare data that should never be looked at. I suppose that’s OK.

Yeah, TabIndexState will tell us whether to look into NodeRareData or not.

> > Source/WebCore/dom/Element.cpp:274
> > +        RELEASE_ASSERT(hasRareData());
> > +        return elementRareData()->unusualTabIndex();
> 
> Why is this RELEASE_ASSERT needed? Doesn’t seem valuable to me.

Oh, right. This used to be a security bug but I've made NodeRareData & RenderObject no longer share the same pointer so this would be a safe nullptr crash now. Converted to ASSERT now.

> > Source/WebCore/dom/ElementRareData.h:233
> > +    ASSERT(m_unusualTabIndex && m_unusualTabIndex != -1); // Common values of 0 and -1 are stored as TabIndexState in Node.
> 
> The main purpose of this assertion is to catch cases where we are getting
> the index before it was set. Otherwise, it’s a waste to assert the same
> thing in both the setter and getter.
> 
> So it could assert only the 0 case; it’s not so much checking for an invalid
> index as checking that we don’t ask for an index when one is not there.

Let's a good point. Let's just assert for 0.

> > Source/WebCore/dom/Node.h:557
> > +    static constexpr uint32_t s_tabIndexStateBitMask = 3 << s_tabIndexStateBitOffset;
> 
> I think this needs to be 3U to get rid of the warning (treated as an error)
> that is causing the build failure on Windows. Hope that works!

Yup. I have that local change but I guess I forgot to re-upload the patch.

> > Source/WebCore/dom/Node.h:563
> > +    enum class TabIndexState : uint8_t {
> > +        NotSet = 0,
> > +        Zero = 1,
> > +        NegativeOne = 2,
> > +        InRareData = 3,
> > +    };
> 
> I don’t think you need the four explicit numeric values here.

It's true but I think it would make it clear how many bits are needed.

[Ryosuke Niwa]

Created attachment 408206 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 408206 [details]
Patch for landing

Wait for EWS

[Ryosuke Niwa]

Comment on attachment 408206 [details]
Patch for landing

Clearing flags on attachment: 408206

Committed r266714: <https://trac.webkit.org/changeset/266714>

[Ryosuke Niwa]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/68485722>