Bug 106965 - NodeRareData doesn't need to have a vtable pointer
Summary: NodeRareData doesn't need to have a vtable pointer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 106981
  Show dependency treegraph
 
Reported: 2013-01-15 17:58 PST by Ryosuke Niwa
Modified: 2013-01-24 11:27 PST (History)
11 users (show)

See Also:


Attachments
Removes the vtable pointer (5.88 KB, patch)
2013-01-15 18:20 PST, Ryosuke Niwa
benjamin: review+
benjamin: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2013-01-15 17:58:18 PST
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.
Comment 1 Elliott Sprehn 2013-01-15 18:09:59 PST
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.
Comment 2 Ryosuke Niwa 2013-01-15 18:20:03 PST
Created attachment 182891 [details]
Removes the vtable pointer
Comment 3 Elliott Sprehn 2013-01-15 19:24:51 PST
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 ?
Comment 4 Ryosuke Niwa 2013-01-15 19:35:17 PST
(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.
Comment 5 Benjamin Poulain 2013-01-15 20:23:23 PST
> > Source/WebCore/dom/ElementRareData.cpp:39
> > +struct SameSizeAsElementRareData {
> 
> Why not just inherit NodeRareData ?

That would defeat the purpose.
Comment 6 Ryosuke Niwa 2013-01-15 20:37:33 PST
(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.
Comment 7 Elliott Sprehn 2013-01-15 20:45:52 PST
(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.
Comment 8 Benjamin Poulain 2013-01-15 20:47:08 PST
> 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 :)
Comment 9 Ryosuke Niwa 2013-01-15 20:48:43 PST
(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.
Comment 10 Benjamin Poulain 2013-01-15 21:04:12 PST
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.
Comment 11 Elliott Sprehn 2013-01-15 21:04:30 PST
(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. :)
Comment 12 Ryosuke Niwa 2013-01-15 21:09:38 PST
(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.
Comment 13 Ryosuke Niwa 2013-01-15 21:24:20 PST
Committed r139833: <http://trac.webkit.org/changeset/139833>
Comment 14 Csaba Osztrogonác 2013-01-15 22:37:17 PST
(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?
Comment 15 Ryosuke Niwa 2013-01-15 22:43:02 PST
(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?
Comment 16 Benjamin Poulain 2013-01-15 22:50:29 PST
Do you have a backtrace with symbols somewhere?
Comment 17 Csaba Osztrogonác 2013-01-15 22:51:36 PST
and Chromium too: http://build.webkit.org/builders/Chromium%20Mac%20Release%20%28Tests%29/builds/31809
Comment 18 Csaba Osztrogonác 2013-01-15 22:52:10 PST
(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.
Comment 19 Benjamin Poulain 2013-01-15 22:53:51 PST
Just a hunch: it looks like it is using libstd delete && libc free instead of the FastAlloc delete and free.
Comment 20 Ryosuke Niwa 2013-01-15 22:55:01 PST
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.
Comment 21 Darin Adler 2013-01-16 09:20:36 PST
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
Comment 22 Ryosuke Niwa 2013-01-24 11:27:30 PST
(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.