Bug 68839

Summary: De-virtualize JSCell::visitChildrenVirtual and remove all other visitChildrenVirtual methods
Product: WebKit Reporter: Mark Hahnenberg <mhahnenberg>
Component: JavaScriptCoreAssignee: Mark Hahnenberg <mhahnenberg>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, ggaren, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 67690    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Mark Hahnenberg 2011-09-26 15:13:43 PDT
The final step in moving visitChildren into the MethodTable in ClassInfo is to make JSCell::visitChildrenVirtual non-virtual and change its implementation to lookup the correct function pointer in the MethodTable.  We also need to remove all of the other visitChildrenVirtual methods throughout the class hierarchy.
Comment 1 Mark Hahnenberg 2011-09-26 16:38:30 PDT
Created attachment 108755 [details]
Patch
Comment 2 Darin Adler 2011-09-27 10:21:07 PDT
Comment on attachment 108755 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSCell.cpp:37
> +void JSCell::visitChildrenVirtual(SlotVisitor& visitor)
> +{
> +    classInfo()->methodTable.visitChildrenFunctionPtr(this, visitor);
> +}

I think this should be inlined at the call site, not left as a cell member function.
Comment 3 Mark Hahnenberg 2011-09-27 11:28:00 PDT
> I think this should be inlined at the call site, not left as a cell member function.

Maybe I could declare it as inline in JSObject.h?  That would alleviate the cost of the extra method call but still allow us to hide the internal method table representation in case we decide to do something later like caching method lookups in the Structure.
Comment 4 Mark Hahnenberg 2011-09-28 11:56:14 PDT
Created attachment 109042 [details]
Patch
Comment 5 Darin Adler 2011-09-28 14:28:54 PDT
Comment on attachment 109042 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSCell.h:89
> +        void visitChildrenVirtual(SlotVisitor&);

This function name is now not great. I suggest just inlining the logic at the call site, as I suggested before. Or renaming this. I don’t think we should use “virtual” to mean “dispatch through the structure method table” since it has a specific C++ meaning already.
Comment 6 Darin Adler 2011-09-28 14:29:47 PDT
(In reply to comment #3)
> > I think this should be inlined at the call site, not left as a cell member function.
> 
> Maybe I could declare it as inline in JSObject.h?  That would alleviate the cost of the extra method call but still allow us to hide the internal method table representation in case we decide to do something later like caching method lookups in the Structure.

I think the real issue is that it’s not a great abstraction. The word “virtual” doesn’t seem great to me, and I don’t think having the single level of indirection has a real practical benefit if we later decide to change things.
Comment 7 Geoffrey Garen 2011-09-28 14:53:19 PDT
> ...in case we decide to do something later like caching method lookups in the Structure.

This is a good point, but I think your patch has proven that we won't need that optimization later. If visitChildren can afford an extra pointer indirection, so can all the other virtual functions, which are less hot.

I tend to agree with Darin's suggestion, since I don't like functions that just call other functions. To make sure call sites aren't super-verbose, I'd give JSCell convenience functions for retrieving structure(), classInfo(), or methodTable().

Also, I think most of the targets of these function pointers can be protected. It's almost always a bug to call one directly. (The only case I know of where it makes sense to call one of these functions directly is the optimization in MarkStack, where we call a specific function target after checking the object's type.)

> Source/JavaScriptCore/ChangeLog:58
> +        Added an out of line virtual destructor to JSWrapperObject and ScopeChainNode to 
> +        appease the vtable gods.  It refused to compile if there were no virtual methods in 
> +        both of these classes due to the presence of a weak vtable pointer.

If this happens again, I think it would be better to add a "virtual void noWeakVtableDummy()" to the JSCell hierarchy so we can avoid having to change real, meaningful functions to satisfy the weak vtable gods.

You can address these comments in a follow-up patch.
Comment 8 Mark Hahnenberg 2011-09-28 16:52:26 PDT
Created attachment 109091 [details]
Patch
Comment 9 Mark Hahnenberg 2011-09-29 10:45:46 PDT
I'm not really sure about the style errors.  The errors weren't posted by the review bot, they're not accessible from the patch status page, and I get no errors when I run check-webkit-style locally.
Comment 10 Geoffrey Garen 2011-09-29 12:02:33 PDT
Comment on attachment 109091 [details]
Patch

r=me, the robots are not our masters.
Comment 11 WebKit Review Bot 2011-09-29 12:36:18 PDT
Comment on attachment 109091 [details]
Patch

Clearing flags on attachment: 109091

Committed r96346: <http://trac.webkit.org/changeset/96346>
Comment 12 WebKit Review Bot 2011-09-29 12:36:24 PDT
All reviewed patches have been landed.  Closing bug.