[webkit-reviews] review granted: [Bug 69186] Remove getCallDataVirtual methods : [Attachment 109388] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Oct 2 12:03:01 PDT 2011


Darin Adler <darin at apple.com> has granted Mark Hahnenberg
<mhahnenberg at apple.com>'s request for review:
Bug 69186: Remove getCallDataVirtual methods
https://bugs.webkit.org/show_bug.cgi?id=69186

Attachment 109388: Patch
https://bugs.webkit.org/attachment.cgi?id=109388&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.


More information about the webkit-reviews mailing list