Bug 190668

<rdar://problem/45342781>

Created attachment 404186 [details] Patch

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

Created attachment 404922 [details] Patch Add more test cases and reflect-apply-while-having-a-bad-time.js.

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

Created attachment 408355 [details] Patch Rename Reflect.apply and mark as @globalPrivate.

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

Created attachment 408382 [details] Patch Avoid creating extra JSFunction for Reflect.apply.

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.