Node is the only remaining subclass of TreeShared. Let's just fold TreeShared into Node.
Created attachment 251143 [details] Patch
Comment on attachment 251143 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=251143&action=review > Source/WebCore/dom/Node.h:545 > + void ref() > + { > + ASSERT(isMainThread()); > + ASSERT(!m_deletionHasBegun); > + ASSERT(!m_inRemovedLastRefFunction); > + ASSERT(!m_adoptionIsRequired); > + ++m_refCount; > + } These function bodies are kind of big. I’d rather see them at the end of the file, outside the class definition. Some day we have a chance of being able to read the class definition. > Source/WebCore/dom/Node.h:560 > + void deref() > + { > + ASSERT(isMainThread()); > + ASSERT(m_refCount >= 0); > + ASSERT(!m_deletionHasBegun); > + ASSERT(!m_inRemovedLastRefFunction); > + ASSERT(!m_adoptionIsRequired); > + if (--m_refCount <= 0 && !parentNode()) { > +#ifndef NDEBUG > + m_inRemovedLastRefFunction = true; > +#endif > + removedLastRef(); > + } > + } This one too. > Source/WebCore/dom/Node.h:567 > + bool hasOneRef() const > + { > + ASSERT(!m_deletionHasBegun); > + ASSERT(!m_inRemovedLastRefFunction); > + return m_refCount == 1; > + } Maybe even this one. > Source/WebCore/dom/Node.h:572 > + int refCount() const > + { > + return m_refCount; > + } Not sure about this one. Not sure why we even have it! > Source/WebCore/dom/Node.h:578 > +#ifndef NDEBUG > + bool m_deletionHasBegun { false }; > + bool m_inRemovedLastRefFunction { false }; > + bool m_adoptionIsRequired { true }; > +#endif Lame that these are public, but I guess that’s how we do it. > Source/WebCore/dom/Node.h:721 > + int m_refCount; How did you decide whether to put this before or after m_nodeFlags?
(In reply to comment #2) > Comment on attachment 251143 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=251143&action=review > > > Source/WebCore/dom/Node.h:545 > > + void ref() > > + { > > + ASSERT(isMainThread()); > > + ASSERT(!m_deletionHasBegun); > > + ASSERT(!m_inRemovedLastRefFunction); > > + ASSERT(!m_adoptionIsRequired); > > + ++m_refCount; > > + } > > These function bodies are kind of big. I’d rather see them at the end of the > file, outside the class definition. Some day we have a chance of being able > to read the class definition. Sure thing, I'll pop them all out. > > Source/WebCore/dom/Node.h:721 > > + int m_refCount; > > How did you decide whether to put this before or after m_nodeFlags? I didn't think enough about this. I'll move it to before so the object layout matches what we had before the change.
Created attachment 251153 [details] Patch for landing
Comment on attachment 251153 [details] Patch for landing Clearing flags on attachment: 251153 Committed r183009: <http://trac.webkit.org/changeset/183009>
All reviewed patches have been landed. Closing bug.
May have caused a 3.6% regression on DoYouEvenBench: https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22mac-yosemite%22%2C%22DoYouEvenBench%2FFull%3ATime%3ATotal%22%5D%5D https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22mac-mavericks%22%2C%22DoYouEvenBench%2FFull%3ATime%3ATotal%22%5D%5D
Really!? I wonder what happened.
(In reply to comment #8) > Really!? I wonder what happened. I'll try to reproduce locally, but if there's indeed a problem, my guess would be failure to inline Node::ref() or Node::deref().
The bug was that DOM nodes were no longer getting WTF_MAKE_FAST_ALLOCATED :| Fixed in <https://trac.webkit.org/r183183>
Wow, how did we miss that!? Did we also forget to make Node noncopyable, or was that handled some other way?
(In reply to comment #10) > The bug was that DOM nodes were no longer getting WTF_MAKE_FAST_ALLOCATED :| > > Fixed in <https://trac.webkit.org/r183183> oh boy I'm glad the bots caught this :)