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().
Created attachment 109388 [details] Patch
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.
> > + 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.
> 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.
Created attachment 110059 [details] Patch
Comment on attachment 110059 [details] Patch Attachment 110059 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10009009
Created attachment 110065 [details] Fix qt
Comment on attachment 110065 [details] Fix qt r=me
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
Committed r96996: <http://trac.webkit.org/changeset/96996>
It broke all tests on the Qt bot. :-/ http://build.webkit.org/results/Qt%20Linux%20Release/r96996%20%2838308%29/results.html
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?
Additionally it broke the Qt Snow Leopard bot: http://build.webkit.org/builders/Qt%20SnowLeopard%20Release/builds/2560
(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).
(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.
I will check this tomorrow if that is acceptable.
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.
(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!
Created attachment 110381 [details] Fixing qt
Comment on attachment 110381 [details] Fixing qt r=me
Comment on attachment 110381 [details] Fixing qt Clearing flags on attachment: 110381 Committed r97097: <http://trac.webkit.org/changeset/97097>
All reviewed patches have been landed. Closing bug.