Move the remaining bit flags from ElementRareData to Node::m_nodeFlags to avoid the creation of ElementRareData in more cases.
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}
Created attachment 408061 [details] Patch
Awesome!
Created attachment 408063 [details] Fixed builds
Comment on attachment 408063 [details] Fixed builds ElementRareData size assertion is failing on Windows
Created attachment 408069 [details] Fixed builds 2
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.
(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.
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.
(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.
Created attachment 408104 [details] Updated per Darin's review comments
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.
(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.
Created attachment 408206 [details] Patch for landing
Comment on attachment 408206 [details] Patch for landing Wait for EWS
Comment on attachment 408206 [details] Patch for landing Clearing flags on attachment: 408206 Committed r266714: <https://trac.webkit.org/changeset/266714>
All reviewed patches have been landed. Closing bug.
<rdar://problem/68485722>