Bug 151467 – implement op_get_rest_length so that we can allocate the rest array with the right size from the start

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

This will allow other optimizations.

[Saam Barati]

Created attachment 265945 [details]
patch

[Geoffrey Garen]

Comment on attachment 265945 [details]
patch

r=me

[Mark Lam]

Comment on attachment 265945 [details]
patch

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

> Source/JavaScriptCore/ChangeLog:10
> +        it might be a constant value in the precense of inlining in the DFG.

typo: precense ==> presence.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5353
>      SpeculateCellOperand array(this, node->child1());
> -    GPRReg arrayGPR = array.gpr();
> -
> -    GPRTemporary argumentsLength(this);
> -    GPRReg argumentsLengthGPR = argumentsLength.gpr();
> -
>      GPRTemporary argumentsStart(this);
> +    SpeculateStrictInt32Operand arrayLength(this, node->child2());

Do we really need a speculation (i.e. runtime check) for the int32-ness of the array length?  Since this value can only be populated with the result of op_get_rest_length, doesn't that already ensure that it is always an int32?  Would a DFG compile time assertion suffice here instead?

Hmmm, for that matter, can the array operand above be anything but a JSArray since the bytecode generator only passes an array as an operand to op_copy_rest.  Will we ever have a chance to invoke op_copy_rest on variable input?  If not, would a DFG compile time assertion suffice here as well?

> Source/JavaScriptCore/jit/JITOpcodes.cpp:1415
> +    unsigned numParamsToSkip = currentInstruction[2].u.unsignedValue;
> +    load32(payloadFor(JSStack::ArgumentCount), regT0);
> +    sub32(TrustedImm32(1), regT0);
> +    Jump zeroLength = branch32(LessThanOrEqual, regT0, TrustedImm32(numParamsToSkip));
> +    sub32(TrustedImm32(numParamsToSkip), regT0);

Technically, you should use Imm32(numParamsToSkip) here instead of TrustedImm32(...).  That is because the numParamsToSkip is defined by the user and can be any value.  Hence, we want to blind that constant if necessary.  In practice for non-malicious code, numParamsToSkip will tend to be small and not need blinding.  But if you use Imm32(), it will take care of figuring that out for you.

[Saam Barati]

Comment on attachment 265945 [details]
patch

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

>> Source/JavaScriptCore/jit/JITOpcodes.cpp:1415
>> +    sub32(TrustedImm32(numParamsToSkip), regT0);
> 
> Technically, you should use Imm32(numParamsToSkip) here instead of TrustedImm32(...).  That is because the numParamsToSkip is defined by the user and can be any value.  Hence, we want to blind that constant if necessary.  In practice for non-malicious code, numParamsToSkip will tend to be small and not need blinding.  But if you use Imm32(), it will take care of figuring that out for you.

Thanks for letting me know. I didn't know this. 
I'll make the change and also in the other places
I do TrustedImm32 w.r.t rest parameters.

[Saam Barati]

landed in:
http://trac.webkit.org/changeset/192814