Bug 190668 – Introduce ReflectApplyFunctionCallDotNode to optimize Reflect.apply

• Status: ASSIGNED
• Component: JavaScriptCore

[Mark Lam]

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.

[Radar WebKit Bug Importer]

<rdar://problem/45342781>

[Alexey Shvayka]

Created attachment 404186 [details]
Patch

[Alexey Shvayka]

(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

[Saam Barati]

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?

[Mark Lam]

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

[Alexey Shvayka]

Created attachment 404922 [details]
Patch

Add more test cases and reflect-apply-while-having-a-bad-time.js.

[Alexey Shvayka]

(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.

[Alexey Shvayka]

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.

[Darin Adler]

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?

[Yusuke Suzuki]

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.

[Alexey Shvayka]

Created attachment 408355 [details]
Patch

Rename Reflect.apply and mark as @globalPrivate.

[Yusuke Suzuki]

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(...)`.

[Alexey Shvayka]

Created attachment 408382 [details]
Patch

Avoid creating extra JSFunction for Reflect.apply.

[Saam Barati]

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.