Bug 143942 – Merge TreeShared into Node.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Andreas Kling]

Node is the only remaining subclass of TreeShared. Let's just fold TreeShared into Node.

[Andreas Kling]

Created attachment 251143 [details]
Patch

[Darin Adler]

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?

[Andreas Kling]

(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.

[Andreas Kling]

Created attachment 251153 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 251153 [details]
Patch for landing

Clearing flags on attachment: 251153

Committed r183009: <http://trac.webkit.org/changeset/183009>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

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

[Darin Adler]

Really!? I wonder what happened.

[Andreas Kling]

(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().

[Andreas Kling]

The bug was that DOM nodes were no longer getting WTF_MAKE_FAST_ALLOCATED :|

Fixed in <https://trac.webkit.org/r183183>

[Darin Adler]

Wow, how did we miss that!? Did we also forget to make Node noncopyable, or was that handled some other way?

[Chris Dumez]

(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 :)