Bug 215572 – Introduce OpIsCallable bytecode and intrinsic

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Alexey Shvayka]

Introduce OpIsCallable bytecode and intrinsic

[Alexey Shvayka]

Created attachment 406719 [details]
Patch

[Ross Kirsling]

Comment on attachment 406719 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406719&action=review

Looks good from my perspective.

> Source/JavaScriptCore/ChangeLog:12
> +        1. Aligns slow_path_is_function with DFG/FTL implementations by introducing
> +           jsTypeofIsFunction() helper. This fixes `typeof document.all === "function"`
> +           to return `true` instead of `false`.

I think you have the operator swapped here -- should be !==, right?

> Source/JavaScriptCore/runtime/Operations.h:49
> +        if (object->type() == JSFunctionType)
> +            return true;
> +        if (object->structure(vm)->masqueradesAsUndefined(globalObject))
> +            return false;
> +        if (object->isCallable(vm))
> +            return true;

isCallable starts by checking JSFunctionType, right? So I think we could drop the first conditional unless there's a perf implication.

> LayoutTests/js/dom/script-tests/document-all-typeof-is-function-fold.js:4
> +function testTypeofIsFuction() {

nit: Fuction -> Function (x2)

[Saam Barati]

Comment on attachment 406719 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406719&action=review

LGTM too

> Source/JavaScriptCore/runtime/Operations.h:47
> +            return false;

Your change log says the opposite

[Alexey Shvayka]

I appreciate all the reviews! A question/suggestion:

Instead of adding a new bytecode op, which I suppose should be avoided whenever possible, we could define @isCallable as a function:

```
@globalPrivate
function isCallable(value)
{
    "use strict";

    return typeof value === "function" || @isCallableMasquerader(value);
}
```

with @isCallableMasquerader being a link-time-constant implemented in C++.
OpIsCallable is a nice primitive, yet it isn't required except by built-ins.
Should I benchmark this approach?

[Saam Barati]

(In reply to Alexey Shvayka from comment #4)
> I appreciate all the reviews! A question/suggestion:
> 
> Instead of adding a new bytecode op, which I suppose should be avoided
> whenever possible, we could define @isCallable as a function:
> 
> ```
> @globalPrivate
> function isCallable(value)
> {
>     "use strict";
> 
>     return typeof value === "function" || @isCallableMasquerader(value);
> }
> ```
> 
> with @isCallableMasquerader being a link-time-constant implemented in C++.
> OpIsCallable is a nice primitive, yet it isn't required except by built-ins.
> Should I benchmark this approach?

I like your current approach more

[Alexey Shvayka]

Created attachment 406868 [details]
Patch

Fix typos in ChangeLog and LayoutTests, remove extra JSFunctionType check.

[EWS]

Committed r265907: <https://trac.webkit.org/changeset/265907>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406868 [details].

[Radar WebKit Bug Importer]

<rdar://problem/67434636>

[Alexey Shvayka]

*** Bug 202485 has been marked as a duplicate of this bug. ***