Bug 190668 - Introduce ReflectApplyFunctionCallDotNode to optimize Reflect.apply
Summary: Introduce ReflectApplyFunctionCallDotNode to optimize Reflect.apply
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Enhancement
Assignee: Alexey Shvayka
URL:
Keywords: InRadar
Depends on: 190671
Blocks:
  Show dependency treegraph
 
Reported: 2018-10-17 10:33 PDT by Mark Lam
Modified: 2020-11-30 16:55 PST (History)
10 users (show)

See Also:


Attachments
Patch (25.55 KB, patch)
2020-07-13 15:22 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (35.20 KB, patch)
2020-07-22 07:38 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (33.55 KB, patch)
2020-09-09 12:04 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (34.64 KB, patch)
2020-09-09 16:25 PDT, Alexey Shvayka
saam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2018-10-17 10:33:51 PDT
Currently, it only checks for Function.apply and actually slows down the call to Reflect.apply.  We should either provide an optimized Reflect.apply or remove the check against Function.apply when we detect Reflect.apply.
Comment 1 Radar WebKit Bug Importer 2018-10-17 10:34:35 PDT
<rdar://problem/45342781>
Comment 2 Alexey Shvayka 2020-07-13 15:22:15 PDT
Created attachment 404186 [details]
Patch
Comment 3 Alexey Shvayka 2020-07-13 15:23:13 PDT
(In reply to Alexey Shvayka from comment #2)
> Created attachment 404186 [details]
> Patch

Warmed-up runs, --outer 25:

                                      r264266                    patch                                       

reflect-apply-array-literal      273.3377+-18.5141    ^     18.7225+-0.5229        ^ definitely 14.5994x faster
reflect-apply-spread              78.0319+-1.9222     ^     57.0211+-1.0070        ^ definitely 1.3685x faster
reflect-apply                     76.8752+-10.2207    ^     57.1569+-0.5991        ^ definitely 1.3450x faster

<geometric>                      117.0257+-4.6683     ^     39.3459+-0.4078        ^ definitely 2.9743x faster
Comment 4 Saam Barati 2020-07-21 12:03:43 PDT
Comment on attachment 404186 [details]
Patch

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

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2041
> +            // Reflect.apply(...args)
> +            SpreadExpressionNode* spread = static_cast<SpreadExpressionNode*>(listNode->m_expr);
> +            RefPtr<RegisterID> spreadRegister = generator.emitNode(spread->expression());
> +            generator.emitExpressionInfo(spread->divot(), spread->divotStart(), spread->divotEnd());
> +            RefPtr<RegisterID> functionRegister = generator.emitGetByVal(generator.newTemporary(), spreadRegister.get(), generator.emitLoad(nullptr, jsNumber(0)));
> +            RefPtr<RegisterID> thisRegister = generator.emitGetByVal(generator.newTemporary(), spreadRegister.get(), generator.emitLoad(nullptr, jsNumber(1)));
> +            RefPtr<RegisterID> argumentsRegister = generator.emitGetByVal(generator.newTemporary(), spreadRegister.get(), generator.emitLoad(nullptr, jsNumber(2)));
> +
> +            emitIsObject(argumentsRegister.get());
> +            generator.emitCallVarargsInTailPosition(returnValue.get(), functionRegister.get(), thisRegister.get(), argumentsRegister.get(), generator.newTemporary(), 0, divot(), divotStart(), divotEnd(), DebuggableCall::Yes);
> +            generator.emitJump(end.get());

how is this spec compliant? Don't we need to run the iterator protocol?
Comment 5 Mark Lam 2020-07-21 12:24:08 PDT
Comment on attachment 404186 [details]
Patch

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

> JSTests/stress/reflect-apply.js:140
> +    var args = [1, 2];

The spec (https://tc39.es/ecma262/#sec-reflect.apply) says that the 3rd argument can be anything that is array-like.  Can you also add cases for object that are array-like e.g. the arguments object in a function, a plain old object with indexed properties, a proxy of an array with a get handler.  Other interesting cases to test with: an array with a few holes in the front or middle, an array with a trailing comma.

Also, in a separate file, add a case for an object with an indexed property in its prototype.  You'll want a different test file because the "object with an indexed property in its prototype" case will put the VM into a "haveABadTime" state.  You can also keep all the other test cases to run in this state.  You can call this other test file reflect-apply-while-having-a-bad-time.js
Comment 6 Alexey Shvayka 2020-07-22 07:38:06 PDT
Created attachment 404922 [details]
Patch

Add more test cases and reflect-apply-while-having-a-bad-time.js.
Comment 7 Alexey Shvayka 2020-07-22 07:46:41 PDT
(In reply to Saam Barati from comment #4)
> how is this spec compliant? Don't we need to run the iterator protocol?

Please note we are emitting `spread` & `new_array_with_spread` bytecode ops here, which convert iterable to an array (see `slow_path_spread` and IteratorHelpers.js).
The same approach is used in CallFunctionCallDotNode.
Comment 8 Alexey Shvayka 2020-07-22 07:49:41 PDT
Comment on attachment 404922 [details]
Patch

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

> JSTests/stress/reflect-apply.js:170
> +shouldBe(Reflect.apply(...(function* () {

I've added a test case with spread & generator.
Comment 9 Darin Adler 2020-08-14 09:45:32 PDT
Comment on attachment 404922 [details]
Patch

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

> Source/JavaScriptCore/builtins/BuiltinNames.h:49
> +    macro(reflectApplyFunction) \

Originally these were sorted alphabetically, with our traditional WebKit "sorted as the Unix sort tool does". But it seems that people instead started adding new ones at the bottom?
Comment 10 Yusuke Suzuki 2020-08-15 22:27:14 PDT
Comment on attachment 404922 [details]
Patch

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

Still reviewing, but one comment.

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1076
> +    m_linkTimeConstants[static_cast<unsigned>(LinkTimeConstant::reflectApplyFunction)].initLater([] (const Initializer<JSCell>& init) {
> +            init.set(JSFunction::create(init.vm, reflectObjectApplyCodeGenerator(init.vm), init.owner));
> +        });

Instead,

1. Let's rename ReflectObject's apply function to `reflectApply` function.
2. Let's mark it as @globalPrivate

Then, this is done automatically.
Comment 11 Alexey Shvayka 2020-09-09 12:04:46 PDT
Created attachment 408355 [details]
Patch

Rename Reflect.apply and mark as @globalPrivate.
Comment 12 Yusuke Suzuki 2020-09-09 12:55:43 PDT
Comment on attachment 408355 [details]
Patch

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

> Source/JavaScriptCore/runtime/ReflectObject.cpp:82
> +    JSC_BUILTIN_FUNCTION_WITHOUT_TRANSITION(vm.propertyNames->builtinNames().applyPublicName(), reflectObjectReflectApplyCodeGenerator, static_cast<unsigned>(PropertyAttribute::DontEnum));

Let's get link-time-constant from global object instead via `globalObject->linkTimeConstant(...)`.
Comment 13 Alexey Shvayka 2020-09-09 16:25:37 PDT
Created attachment 408382 [details]
Patch

Avoid creating extra JSFunction for Reflect.apply.
Comment 14 Saam Barati 2020-11-30 16:55:46 PST
Comment on attachment 408382 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:17
> +        To accomodate monkey-patching, Reflect.apply is exposed via @globalPrivate

accomodate => accommodate

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2001
> +RegisterID* ReflectApplyFunctionCallDotNode::emitBytecode(BytecodeGenerator& generator, RegisterID* dst)

I wonder if we can somehow abstract this function to deal with all the variants that are doing similar things.