Bug 68839 – De-virtualize JSCell::visitChildrenVirtual and remove all other visitChildrenVirtual methods

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Hahnenberg]

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.

[Mark Hahnenberg]

Created attachment 108755 [details]
Patch

[Darin Adler]

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.

[Mark Hahnenberg]

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

[Mark Hahnenberg]

Created attachment 109042 [details]
Patch

[Darin Adler]

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.

[Darin Adler]

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

[Geoffrey Garen]

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

[Mark Hahnenberg]

Created attachment 109091 [details]
Patch

[Mark Hahnenberg]

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.

[Geoffrey Garen]

Comment on attachment 109091 [details]
Patch

r=me, the robots are not our masters.

[WebKit Review Bot]

Comment on attachment 109091 [details]
Patch

Clearing flags on attachment: 109091

Committed r96346: <http://trac.webkit.org/changeset/96346>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.