[webkit-reviews] review granted: [Bug 46207] Speed up function.apply(..., arguments) : [Attachment 68271] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Oliver Hunt <oliver at apple.com>'s
request for review:
Bug 46207: Speed up function.apply(..., arguments)
https://bugs.webkit.org/show_bug.cgi?id=46207

Attachment 68271: Patch
https://bugs.webkit.org/attachment.cgi?id=68271&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=68271&action=review

r=me, but I think you should make these changes.

> JavaScriptCore/jit/JITOpcodes.cpp:1533
> +	   emitLoad(argsOffset, regT1, regT0);
> +	   slowJumps.append(branchTestPtr(NonZero, regT0));
> +	   slowJumps.append(branchTestPtr(NonZero, regT1));

To test for JSValue(), test the tag against JSValue::EmptyValueTag.

> JavaScriptCore/jit/JITOpcodes.cpp:1541
> +	   slowJumps.append(branch32(Above, regT0,
Imm32(Arguments::MaxArguments)));

Do you need this check on the hot path? In theory, the only limit to argument
use on the hot path is the size of the register file, which you check below.

> JavaScriptCore/jit/JITOpcodes.cpp:1560
> +	   if (sizeof(Register) == 2 * ScalePtr) {
> +	       loadPtr(BaseIndex(regT1, regT0, ScalePtr,
static_cast<unsigned>(-sizeof(Register) + sizeof(void*))), regT3);
> +	       storePtr(regT3, BaseIndex(regT2, regT0, ScalePtr,
sizeof(void*)));
> +	   }

Might be clearer just to use #if USE(JSVALUE32_64) here instead.

> JavaScriptCore/jit/JITOpcodes.cpp:1561
> +	   jump(loopStart);

This will "jump to jump" each time through the loop. Might be faster to do
"branchTest32(Zero, regT0)" here, branching back to the sub32 above.