| Summary: | Introduce OpIsCallable bytecode and intrinsic | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alexey Shvayka <ashvayka> | ||||||
| Component: | JavaScriptCore | Assignee: | Alexey Shvayka <ashvayka> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Trivial | CC: | ews-watchlist, ggaren, joepeck, keith_miller, mark.lam, msaboff, ross.kirsling, saam, tzagallo, webkit-bug-importer, ysuzuki | ||||||
| Priority: | P2 | Keywords: | InRadar, WebExposed | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| See Also: |
https://bugs.webkit.org/show_bug.cgi?id=214525 https://bugs.webkit.org/show_bug.cgi?id=214443 |
||||||||
| Attachments: |
|
||||||||
|
Description
Alexey Shvayka
2020-08-17 09:18:23 PDT
Created attachment 406719 [details]
Patch
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) 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 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?
(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 Created attachment 406868 [details]
Patch
Fix typos in ChangeLog and LayoutTests, remove extra JSFunctionType check.
Committed r265907: <https://trac.webkit.org/changeset/265907> All reviewed patches have been landed. Closing bug and clearing flags on attachment 406868 [details]. *** Bug 202485 has been marked as a duplicate of this bug. *** |