WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
69186
Remove getCallDataVirtual methods
https://bugs.webkit.org/show_bug.cgi?id=69186
Summary
Remove getCallDataVirtual methods
Mark Hahnenberg
Reported
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().
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
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Mark Hahnenberg
Comment 1
2011-09-30 18:20:45 PDT
Created
attachment 109388
[details]
Patch
Darin Adler
Comment 2
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.
Geoffrey Garen
Comment 3
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.
Geoffrey Garen
Comment 4
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.
Mark Hahnenberg
Comment 5
2011-10-06 16:43:26 PDT
Created
attachment 110059
[details]
Patch
Early Warning System Bot
Comment 6
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
Mark Hahnenberg
Comment 7
2011-10-06 17:24:51 PDT
Created
attachment 110065
[details]
Fix qt
Geoffrey Garen
Comment 8
2011-10-07 16:46:56 PDT
Comment on
attachment 110065
[details]
Fix qt r=me
WebKit Review Bot
Comment 9
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
Mark Hahnenberg
Comment 10
2011-10-07 18:43:29 PDT
Committed
r96996
: <
http://trac.webkit.org/changeset/96996
>
Csaba Osztrogonác
Comment 11
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
Csaba Osztrogonác
Comment 12
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?
Csaba Osztrogonác
Comment 13
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
Mark Hahnenberg
Comment 14
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
).
Csaba Osztrogonác
Comment 15
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.
Zoltan Herczeg
Comment 16
2011-10-10 03:09:14 PDT
I will check this tomorrow if that is acceptable.
Geoffrey Garen
Comment 17
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.
Mark Hahnenberg
Comment 18
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!
Mark Hahnenberg
Comment 19
2011-10-10 12:37:21 PDT
Created
attachment 110381
[details]
Fixing qt
Geoffrey Garen
Comment 20
2011-10-10 12:51:58 PDT
Comment on
attachment 110381
[details]
Fixing qt r=me
WebKit Review Bot
Comment 21
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
>
WebKit Review Bot
Comment 22
2011-10-10 15:31:16 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug