Bug 38920 – V8 overload support ported to JSC

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yaar Schnitman]

V8 overload support ported to JSC

[Yaar Schnitman]

Created attachment 55732 [details]
Patch

[Yaar Schnitman]

Comment on attachment 55732 [details]
Patch

Please cq+ if ok.

[Yaar Schnitman]

Created attachment 55882 [details]
Patch

[Adam Barth]

Comment on attachment 55882 [details]
Patch

WebCore/bindings/scripts/CodeGenerator.pm:378
 +          $function->{overloadIndex} = @{$nameToFunctionsMap{$name}};
This perl is slightly mysterious to me.

This looks reasonable.  It's a bit of a hack to skip the functions when the overload index is greater than one, but I think that's ok for now.

[Sam Weinig]

Comment on attachment 55882 [details]
Patch

> Index: WebCore/bindings/scripts/test/JS/JSTestObj.cpp
> ===================================================================
> --- WebCore/bindings/scripts/test/JS/JSTestObj.cpp	(revision 59097)
> +++ WebCore/bindings/scripts/test/JS/JSTestObj.cpp	(working copy)
> @@ -133,7 +133,7 @@ bool JSTestObjConstructor::getOwnPropert
>  #define THUNK_GENERATOR(generator)
>  #endif
>  
> -static const HashTableValue JSTestObjPrototypeTableValues[26] =
> +static const HashTableValue JSTestObjPrototypeTableValues[27] =
>  {
>      { "voidMethod", DontDelete | Function, (intptr_t)static_cast<NativeFunction>(jsTestObjPrototypeFunctionVoidMethod), (intptr_t)0 THUNK_GENERATOR(0) },
>      { "voidMethodWithArgs", DontDelete | Function, (intptr_t)static_cast<NativeFunction>(jsTestObjPrototypeFunctionVoidMethodWithArgs), (intptr_t)3 THUNK_GENERATOR(0) },
> @@ -160,6 +160,7 @@ static const HashTableValue JSTestObjPro
>      { "methodWithOptionalArg", DontDelete | Function, (intptr_t)static_cast<NativeFunction>(jsTestObjPrototypeFunctionMethodWithOptionalArg), (intptr_t)1 THUNK_GENERATOR(0) },
>      { "methodWithNonOptionalArgAndOptionalArg", DontDelete | Function, (intptr_t)static_cast<NativeFunction>(jsTestObjPrototypeFunctionMethodWithNonOptionalArgAndOptionalArg), (intptr_t)2 THUNK_GENERATOR(0) },
>      { "methodWithNonOptionalArgAndTwoOptionalArgs", DontDelete | Function, (intptr_t)static_cast<NativeFunction>(jsTestObjPrototypeFunctionMethodWithNonOptionalArgAndTwoOptionalArgs), (intptr_t)3 THUNK_GENERATOR(0) },
> +    { "overloadedMethod", DontDelete | Function, (intptr_t)static_cast<NativeFunction>(jsTestObjPrototypeFunctionOverloadedMethod), (intptr_t)2 THUNK_GENERATOR(0) },
>      { 0, 0, 0, 0 THUNK_GENERATOR(0) }
>  };
>  
> @@ -801,6 +802,80 @@ JSValue JSC_HOST_CALL jsTestObjPrototype
>      return jsUndefined();
>  }
>  
> +JSValue JSC_HOST_CALL jsTestObjPrototypeFunctionOverloadedMethod1(ExecState* exec, JSObject*, JSValue thisValue, const ArgList& args)
> +{
> +    UNUSED_PARAM(args);
> +    if (!thisValue.inherits(&JSTestObj::s_info))
> +        return throwError(exec, TypeError);
> +    JSTestObj* castedThis = static_cast<JSTestObj*>(asObject(thisValue));
> +    TestObj* imp = static_cast<TestObj*>(castedThis->impl());
> +    TestObj* objArg = toTestObj(args.at(0));
> +    const String& strArg = ustringToString(args.at(1).toString(exec));
> +
> +    imp->overloadedMethod(objArg, strArg);
> +    return jsUndefined();
> +}
> +
> +JSValue JSC_HOST_CALL jsTestObjPrototypeFunctionOverloadedMethod2(ExecState* exec, JSObject*, JSValue thisValue, const ArgList& args)
> +{
> +    UNUSED_PARAM(args);
> +    if (!thisValue.inherits(&JSTestObj::s_info))
> +        return throwError(exec, TypeError);
> +    JSTestObj* castedThis = static_cast<JSTestObj*>(asObject(thisValue));
> +    TestObj* imp = static_cast<TestObj*>(castedThis->impl());
> +    TestObj* objArg = toTestObj(args.at(0));
> +
> +    int argsCount = args.size();
> +    if (argsCount < 2) {
> +        imp->overloadedMethod(objArg);
> +        return jsUndefined();
> +    }
> +
> +    int intArg = args.at(1).toInt32(exec);
> +
> +    imp->overloadedMethod(objArg, intArg);
> +    return jsUndefined();
> +}
> +
> +JSValue JSC_HOST_CALL jsTestObjPrototypeFunctionOverloadedMethod3(ExecState* exec, JSObject*, JSValue thisValue, const ArgList& args)
> +{
> +    UNUSED_PARAM(args);
> +    if (!thisValue.inherits(&JSTestObj::s_info))
> +        return throwError(exec, TypeError);
> +    JSTestObj* castedThis = static_cast<JSTestObj*>(asObject(thisValue));
> +    TestObj* imp = static_cast<TestObj*>(castedThis->impl());
> +    const String& strArg = ustringToString(args.at(0).toString(exec));
> +
> +    imp->overloadedMethod(strArg);
> +    return jsUndefined();
> +}
> +
> +JSValue JSC_HOST_CALL jsTestObjPrototypeFunctionOverloadedMethod4(ExecState* exec, JSObject*, JSValue thisValue, const ArgList& args)
> +{
> +    UNUSED_PARAM(args);
> +    if (!thisValue.inherits(&JSTestObj::s_info))
> +        return throwError(exec, TypeError);
> +    JSTestObj* castedThis = static_cast<JSTestObj*>(asObject(thisValue));
> +    TestObj* imp = static_cast<TestObj*>(castedThis->impl());
> +    int intArg = args.at(0).toInt32(exec);
> +
> +    imp->overloadedMethod(intArg);
> +    return jsUndefined();
> +}
> +
> +JSValue JSC_HOST_CALL testObjPrototypeFunctionOverloadedMethod(ExecState* exec, JSObject*, JSValue thisValue, const ArgList& args)
> +{
> +    if ((args.size() == 2 && (args.at(0).isNull() || asObject(args.at(0))->inherits(JSTestObj::s_info) && (args.at(1).isNull() || args.at(1).isUndefined() || args.at(1).isString() || args.at(1).isObject())))
> +        return testObjPrototypeFunctionOverloadedMethod1Callback(args);
> +    if ((args.size() == 1 && (args.at(0).isNull() || asObject(args.at(0))->inherits(JSTestObj::s_info)) || (args.size() == 2 && (args.at(0).isNull() || asObject(args.at(0))->inherits(JSTestObj::s_info)))
> +        return testObjPrototypeFunctionOverloadedMethod2Callback(args);
> +    if ((args.size() == 1 && (args.at(0).isNull() || args.at(0).isUndefined() || args.at(0).isString() || args.at(0).isObject())))
> +        return testObjPrototypeFunctionOverloadedMethod3Callback(args);
> +    if (args.size() == 1)
> +        return testObjPrototypeFunctionOverloadedMethod4Callback(args);
> +    return throwError(exec, TypeError);
> +}
> +
>  JSC::JSValue toJS(JSC::ExecState* exec, JSDOMGlobalObject* globalObject, TestObj* object)
>  {


It is possible I am reading this wrong, but where are the implementations of testObjPrototypeFunctionOverloadedMethod1Callback and friends.  Are they supposed to be called jsTestObjPrototypeFunctionOverloadedMethod1 and friends. Are they supposed to take more parameters?  Can we optimize the if statement in this function so it does not do the same work multiple times (calling things like inherits)?

[Yaar Schnitman]

(In reply to comment #5)
> It is possible I am reading this wrong, but where are the implementations of testObjPrototypeFunctionOverloadedMethod1Callback and friends.  
Nice catch. This slipped in from V8. I'll fix that. I would have probably caught that once I turned on some of the overloads in the IDLs.

> Can we optimize the if statement in this function so it does not do the same work multiple times (calling things like inherits)?
This can probably be done, but its very hard and with significant added complexity to the generator. In real use cases (WebGL, Canvas and XHR overloads), the primary determinant is the length of the args vector, and therefore only a small portion of the if statements are actually passed through. Also, in these overrides, the overhead of override selection is dwarfed by what these overrides usually do (e.g. copy large arrays). Therefore such an optimization won't have a big impact.

[Sam Weinig]

Comment on attachment 55882 [details]
Patch

r-ing now that I know it is wrong.  I will look at making it a bit smarter in the future I guess.

[Yaar Schnitman]

Created attachment 55988 [details]
Patch

[Yaar Schnitman]

Created attachment 55989 [details]
Patch

[Yaar Schnitman]

Comment on attachment 55989 [details]
Patch

Fixed method names. Please cq+ if ok.

[Adam Barth]

Comment on attachment 55989 [details]
Patch

Thanks!

[Yaar Schnitman]

Thank you too!

[WebKit Commit Bot]

Comment on attachment 55989 [details]
Patch

Clearing flags on attachment: 55989

Committed r59533: <http://trac.webkit.org/changeset/59533>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/59533 might have broken GTK Linux 32-bit Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/59530
http://trac.webkit.org/changeset/59531
http://trac.webkit.org/changeset/59532
http://trac.webkit.org/changeset/59533