Bug 69186 – Remove getCallDataVirtual methods

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

[Mark Hahnenberg]

This is the last step in de-virtualizing getCallData.  We need to remove all getCallDataVirtual function definitions and replace the call sites with methodTable()->getCallData().

[Mark Hahnenberg]

Created attachment 109388 [details]
Patch

[Darin Adler]

Comment on attachment 109388 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:11
> +        Removed all getCallDataVirtual methods and replaced their call sites 
> +        with an explicit lookup in the MethodTable.  Also had to add ClassInfo 
> +        structs to some of the classes that overrode getCallData but didn't 
> +        provide their own ClassInfo structure.

Can we add the new ClassInfo structures first in a separate patch? Seems that it would make both patches easier to review.

> Source/JavaScriptCore/runtime/Error.cpp:214
> +    static Structure* createStructure(JSGlobalData& globalData, JSGlobalObject* globalObject, JSValue proto) 
> +    { 
> +        return Structure::create(globalData, globalObject, proto, TypeInfo(ObjectType, StructureFlags), &s_info); 
> +    }

The word prototype should not be abbreviated to proto. Same comment for the many other new createStructure functions. Also, can at least some of these functions be private instead of public? If they are private and only used in create functions (see comment below), then there’s a good chance we can move the function bodies into .cpp files without losing inline.

> Source/JavaScriptCore/runtime/Error.cpp:216
> +    static JS_EXPORTDATA const ClassInfo s_info;

Does this need to be public? Same comment for all the other new s_info.

> Source/JavaScriptCore/runtime/Error.cpp:229
> -    return StrictModeTypeErrorFunction::create(exec, exec->lexicalGlobalObject(), exec->lexicalGlobalObject()->internalFunctionStructure(), message);
> +    JSGlobalObject* globalObject = exec->lexicalGlobalObject();
> +    return StrictModeTypeErrorFunction::create(exec, globalObject, StrictModeTypeErrorFunction::createStructure(exec->globalData(), globalObject, globalObject->functionPrototype()), message);

This seems to change from reusing a single structure to creating a new structure each time. Is that acceptable for performance? I guess these aren’t created that often?

I believe the reason we pass a structure in to the create functions has to do with when they were constructors, not functions. It seems to me that nowadays we can move this responsibility into the create function since we have done away with the "allocation during constructor" problem. Doing that would allow the createStructure function to be private.

> Source/JavaScriptCore/runtime/InternalFunction.cpp:76
> +CallType InternalFunction::getCallData(JSCell*, CallData&)
> +{
> +    ASSERT_NOT_REACHED();
> +    return CallTypeNone;
> +}

Is this better than having a null pointer in the method table?

> Source/JavaScriptCore/runtime/JSCell.h:138
> -
> +    

Seems like a gratuitous whitespace change.

> Source/JavaScriptCore/runtime/Structure.h:308
> +    inline CallType getCallData(JSValue value, CallData& callData)

Could this function go in a different header file? I don’t understand why Structure.h is the right file for this. It seems unpleasant that Structure.h now has to include ClassInfo.h.

[Geoffrey Garen]

> > +    return StrictModeTypeErrorFunction::create(exec, globalObject, > StrictModeTypeErrorFunction::createStructure(exec->globalData(), globalObject, globalObject-> >functionPrototype()), message);

You can do this without creating a new structure each time if you give JSGlobalData a data member for StrictModeTypeErrorFunction's structure. Then it can be shared, like the structure for InternalFunction.

[Geoffrey Garen]

> You can do this without creating a new structure each time if you give JSGlobalData a data member for StrictModeTypeErrorFunction's structure. Then it can be shared, like the structure for InternalFunction.

Actually, you probably want to put the data member on JSGlobalObject.

[Mark Hahnenberg]

Created attachment 110059 [details]
Patch

[Early Warning System Bot]

Comment on attachment 110059 [details]
Patch

Attachment 110059 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10009009

[Mark Hahnenberg]

Created attachment 110065 [details]
Fix qt

[Geoffrey Garen]

Comment on attachment 110065 [details]
Fix qt

r=me

[WebKit Review Bot]

Comment on attachment 110065 [details]
Fix qt

Rejecting attachment 110065 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
lativeJIT64.cpp
	M	Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
	M	Source/JavaScriptCore/create_hash_table
r96993 = 4b8cca37ba5752f09cddaf4d8d1e9807200bd175 (refs/remotes/origin/master)
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.
Updating chromium port dependencies using gclient...

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/10000371

[Mark Hahnenberg]

Committed r96996: <http://trac.webkit.org/changeset/96996>

[Csaba Osztrogonác]

It broke all tests on the Qt bot. :-/

http://build.webkit.org/results/Qt%20Linux%20Release/r96996%20%2838308%29/results.html

[Csaba Osztrogonác]

Reopen, because it was rolled out by http://trac.webkit.org/changeset/97006

Zoltan or Gabor, could you check this bug please and give some hints what happened?

[Csaba Osztrogonác]

Additionally it broke the Qt Snow Leopard bot: http://build.webkit.org/builders/Qt%20SnowLeopard%20Release/builds/2560

[Mark Hahnenberg]

(In reply to comment #13)
> Additionally it broke the Qt Snow Leopard bot: http://build.webkit.org/builders/Qt%20SnowLeopard%20Release/builds/2560

The SL breakage appears to be due to a bug in the Qt build tools (see bug 67799).

[Csaba Osztrogonác]

(In reply to comment #14)
> (In reply to comment #13)
> > Additionally it broke the Qt Snow Leopard bot: http://build.webkit.org/builders/Qt%20SnowLeopard%20Release/builds/2560
> 
> The SL breakage appears to be due to a bug in the Qt build tools (see bug 67799).

Sorry, I didn't check the build log. You're right, 67799 is the culprit.
Dihan, Alexis, please trigger a clean build on the Qt-SL bots after this patch landed.

[Zoltan Herczeg]

I will check this tomorrow if that is acceptable.

[Geoffrey Garen]

It looks like the Qt failure may be due to Qt's DumpRenderTree linking directly to JSC's internal C++ API. If true, the right long-term fix is to migrate Qt's DumpRenderTree to using JSC's public C API.

[Mark Hahnenberg]

(In reply to comment #17)
> It looks like the Qt failure may be due to Qt's DumpRenderTree linking directly to JSC's internal C++ API. If true, the right long-term fix is to migrate Qt's DumpRenderTree to using JSC's public C API.

I think the failure was caused by the fact that QtRuntimeMetaMethod and QtRuntimeConnectionMethod were using QtRuntimeMethod's structure, which gave them the wrong method table.  I'm all for reducing the amount of coupling between JSC internals and other projects though!

[Mark Hahnenberg]

Created attachment 110381 [details]
Fixing qt

[Geoffrey Garen]

Comment on attachment 110381 [details]
Fixing qt

r=me

[WebKit Review Bot]

Comment on attachment 110381 [details]
Fixing qt

Clearing flags on attachment: 110381

Committed r97097: <http://trac.webkit.org/changeset/97097>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.