NodeRareData doesn't need to have a vtable pointer

• Component: DOM
• Status: RESOLVED FIXED

[Ryosuke Niwa]

We have a vtable pointer in NodeRareData just so that its destructor and reportMemoryUsage can use them. That's silly because the only place wee delete NodeRareData is Node::clearRareData and we already explicitly calls delete.

[Elliott Sprehn]

I have a patch for this from a few months ago but it made the code a little confusing so I had decided against it. I can just upload it now though...

You can't just naively rely on us calling delete since deleting ElementRareData from a base class ptr with a non virtual destructor produces undefined behavior.

[Ryosuke Niwa]

Created attachment 182891 [details]
Removes the vtable pointer

[Elliott Sprehn]

Comment on attachment 182891 [details]
Removes the vtable pointer

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

Can we devirtualize createRareData() as well so people can't return subclasses anymore? There's no reason to support generic subclasses of rare data with virtual createRareData() since they wouldn't have their destructors handled properly.

> Source/WebCore/dom/ElementRareData.cpp:39
> +struct SameSizeAsElementRareData {

Why not just inherit NodeRareData ?

[Ryosuke Niwa]

(In reply to comment #3)
> (From update of attachment 182891 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182891&action=review
> 
> Can we devirtualize createRareData() as well so people can't return subclasses anymore? There's no reason to support generic subclasses of rare data with virtual createRareData() since they wouldn't have their destructors handled properly.

Let's do that in a separate patch. There are few cleanups we can do there.

[Benjamin Poulain]

> > Source/WebCore/dom/ElementRareData.cpp:39
> > +struct SameSizeAsElementRareData {
> 
> Why not just inherit NodeRareData ?

That would defeat the purpose.

[Ryosuke Niwa]

(In reply to comment #5)
> > > Source/WebCore/dom/ElementRareData.cpp:39
> > > +struct SameSizeAsElementRareData {
> > 
> > Why not just inherit NodeRareData ?
> 
> That would defeat the purpose.

It's same thing since I'm including NodeRareData as a member variable. I'm simply ensuring that the size of NodeRareData is not zero, in which case sizeof(SameSizeAsElementRareData) > sizeof(ElementRareData), among other things.

[Elliott Sprehn]

(In reply to comment #6)
> (In reply to comment #5)
> > > > Source/WebCore/dom/ElementRareData.cpp:39
> > > > +struct SameSizeAsElementRareData {
> > > 
> > > Why not just inherit NodeRareData ?
> > 
> > That would defeat the purpose.
> 
> It's same thing since I'm including NodeRareData as a member variable. I'm simply ensuring that the size of NodeRareData is not zero, in which case sizeof(SameSizeAsElementRareData) > sizeof(ElementRareData), among other things.

I don't understand this comment. Why would this make the struct bigger than the subclass? In general in WebCore we just inherit, ex. SameSizeAsRenderText, ShadowRoot, CSSRule, CSSRuleBase etc. I don't see anyone doing it like this.

[Benjamin Poulain]

> I don't understand this comment. Why would this make the struct bigger than the subclass? In general in WebCore we just inherit, ex. SameSizeAsRenderText, ShadowRoot, CSSRule, CSSRuleBase etc. I don't see anyone doing it like this.

I thought you were commenting on SameSizeAsNodeRareData. I should probably read the patch :)

[Ryosuke Niwa]

(In reply to comment #7)
> I don't understand this comment. Why would this make the struct bigger than the subclass? In general in WebCore we just inherit, ex. SameSizeAsRenderText, ShadowRoot, CSSRule, CSSRuleBase etc. I don't see anyone doing it like this.

struct A { };
struct B { A a; int b; }
struct C : A { int c; }

Then sizeof(B) > sizeof(C) in most implementations.

[Benjamin Poulain]

Comment on attachment 182891 [details]
Removes the vtable pointer

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

Looks good to me. I like both Elliott's ideas.

> Source/WebCore/dom/ElementRareData.cpp:44
> +    void* pointers[7];
> +    LayoutSize sizeForResizing;

Please move sizeForResizing before pointers.

[Elliott Sprehn]

(In reply to comment #9)
> (In reply to comment #7)
> > I don't understand this comment. Why would this make the struct bigger than the subclass? In general in WebCore we just inherit, ex. SameSizeAsRenderText, ShadowRoot, CSSRule, CSSRuleBase etc. I don't see anyone doing it like this.
> 
> struct A { };
> struct B { A a; int b; }
> struct C : A { int c; }
> 
> Then sizeof(B) > sizeof(C) in most implementations.

Oh that's really fascinating! In this case sizeof(A) is actually 1 not 0. Thanks. :)

[Ryosuke Niwa]

(In reply to comment #10)
> (From update of attachment 182891 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182891&action=review
> 
> Looks good to me. I like both Elliott's ideas.
> 
> > Source/WebCore/dom/ElementRareData.cpp:44
> > +    void* pointers[7];
> > +    LayoutSize sizeForResizing;
> 
> Please move sizeForResizing before pointers.

I'm gonna move both non-pointer times to before pointers as we discussed in person.

[Ryosuke Niwa]

Committed r139833: <http://trac.webkit.org/changeset/139833>

[Csaba Osztrogonác]

(In reply to comment #13)
> Committed r139833: <http://trac.webkit.org/changeset/139833>

It made all tests crash on GTK and on Qt:
- http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release/builds/33025
- http://build.webkit.org/builders/Qt%20Linux%20Release/builds/56380

It seems they are GC related crashes. Could you check it please?

[Ryosuke Niwa]

(In reply to comment #14)
> (In reply to comment #13)
> > Committed r139833: <http://trac.webkit.org/changeset/139833>
> 
> It made all tests crash on GTK and on Qt:
> - http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release/builds/33025
> - http://build.webkit.org/builders/Qt%20Linux%20Release/builds/56380
> 
> It seems they are GC related crashes. Could you check it please?

Oops, sorry about that.This is strange maybe it doesn't like non-virtual destructor somehow?

[Benjamin Poulain]

Do you have a backtrace with symbols somewhere?

[Csaba Osztrogonác]

and Chromium too: http://build.webkit.org/builders/Chromium%20Mac%20Release%20%28Tests%29/builds/31809

[Csaba Osztrogonác]

(In reply to comment #16)
> Do you have a backtrace with symbols somewhere?
I'm on it. But unfortunately it doesn't occur in debug mode.

[Benjamin Poulain]

Just a hunch: it looks like it is using libstd delete && libc free instead of the FastAlloc delete and free.

[Ryosuke Niwa]

Fix attempted in http://trac.webkit.org/changeset/139838. The problem was that m_rareData is of type NodeRareDataBase even though it's an instance of NodeRareData so we have to explicitly cast it into NodeRareData before deleting it.

[Darin Adler]

Comment on attachment 182891 [details]
Removes the vtable pointer

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

> Source/WebCore/dom/NodeRareData.h:309
> +    // This member function is intentionially not virtual to avoid adding a vtable pointer.

Typo: intentionially

[Ryosuke Niwa]

(In reply to comment #21)
> (From update of attachment 182891 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182891&action=review
> 
> > Source/WebCore/dom/NodeRareData.h:309
> > +    // This member function is intentionially not virtual to avoid adding a vtable pointer.
> 
> Typo: intentionially

Oops, fixed in http://trac.webkit.org/changeset/140700.