Bug 69186 - Remove getCallDataVirtual methods
Summary: Remove getCallDataVirtual methods
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Hahnenberg
URL:
Keywords:
Depends on: 69311 69697
Blocks: 67690
  Show dependency treegraph
 
Reported: 2011-09-30 16:27 PDT by Mark Hahnenberg
Modified: 2011-10-10 15:31 PDT (History)
8 users (show)

See Also:


Attachments
Patch (76.27 KB, patch)
2011-09-30 18:20 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (80.65 KB, patch)
2011-10-06 16:43 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Fix qt (80.70 KB, patch)
2011-10-06 17:24 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Fixing qt (82.83 KB, patch)
2011-10-10 12:37 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Hahnenberg 2011-09-30 16:27:09 PDT
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().
Comment 1 Mark Hahnenberg 2011-09-30 18:20:45 PDT
Created attachment 109388 [details]
Patch
Comment 2 Darin Adler 2011-10-02 12:03:01 PDT
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.
Comment 3 Geoffrey Garen 2011-10-02 15:06:16 PDT
> > +    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.
Comment 4 Geoffrey Garen 2011-10-02 15:07:39 PDT
> 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.
Comment 5 Mark Hahnenberg 2011-10-06 16:43:26 PDT
Created attachment 110059 [details]
Patch
Comment 6 Early Warning System Bot 2011-10-06 17:11:09 PDT
Comment on attachment 110059 [details]
Patch

Attachment 110059 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10009009
Comment 7 Mark Hahnenberg 2011-10-06 17:24:51 PDT
Created attachment 110065 [details]
Fix qt
Comment 8 Geoffrey Garen 2011-10-07 16:46:56 PDT
Comment on attachment 110065 [details]
Fix qt

r=me
Comment 9 WebKit Review Bot 2011-10-07 17:06:34 PDT
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
Comment 10 Mark Hahnenberg 2011-10-07 18:43:29 PDT
Committed r96996: <http://trac.webkit.org/changeset/96996>
Comment 11 Csaba Osztrogonác 2011-10-08 01:47:53 PDT
It broke all tests on the Qt bot. :-/

http://build.webkit.org/results/Qt%20Linux%20Release/r96996%20%2838308%29/results.html
Comment 12 Csaba Osztrogonác 2011-10-08 02:42:02 PDT
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?
Comment 13 Csaba Osztrogonác 2011-10-08 02:43:20 PDT
Additionally it broke the Qt Snow Leopard bot: http://build.webkit.org/builders/Qt%20SnowLeopard%20Release/builds/2560
Comment 14 Mark Hahnenberg 2011-10-08 17:37:11 PDT
(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).
Comment 15 Csaba Osztrogonác 2011-10-09 03:34:09 PDT
(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.
Comment 16 Zoltan Herczeg 2011-10-10 03:09:14 PDT
I will check this tomorrow if that is acceptable.
Comment 17 Geoffrey Garen 2011-10-10 12:16:12 PDT
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.
Comment 18 Mark Hahnenberg 2011-10-10 12:31:04 PDT
(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!
Comment 19 Mark Hahnenberg 2011-10-10 12:37:21 PDT
Created attachment 110381 [details]
Fixing qt
Comment 20 Geoffrey Garen 2011-10-10 12:51:58 PDT
Comment on attachment 110381 [details]
Fixing qt

r=me
Comment 21 WebKit Review Bot 2011-10-10 15:31:10 PDT
Comment on attachment 110381 [details]
Fixing qt

Clearing flags on attachment: 110381

Committed r97097: <http://trac.webkit.org/changeset/97097>
Comment 22 WebKit Review Bot 2011-10-10 15:31:16 PDT
All reviewed patches have been landed.  Closing bug.