Bug 78612 – Implement fast path for op_new_array in the baseline JIT

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Hahnenberg]

Currently, op_new_array simply does a stub call out to C++ code. With the new copying collector, we should be able to implement an inline asm fast path for op_new_array.

[Mark Hahnenberg]

Created attachment 127868 [details]
Patch

[Filip Pizlo]

Comment on attachment 127868 [details]
Patch

I can dig it.  But check performance.

[Mark Hahnenberg]

Created attachment 127886 [details]
Bencher results

[WebKit Review Bot]

Comment on attachment 127868 [details]
Patch

Clearing flags on attachment: 127868

Committed r108291: <http://trac.webkit.org/changeset/108291>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Filip Pizlo]

I think you broke 32-bit.

[Filip Pizlo]

Yeah you totally broke 32-bit. ;-)  I'm going to roll this out for now since it's blocking the interpreter.

[Filip Pizlo]

These are the failures.  Looks like you need to be careful about the JIT code you emit on 32-bit.  In particular, you can't copy JSValues around using Ptr; you have to use two 32s.



** Danger, Will Robinson! Danger! The following failures have been introduced:
	ecma/Date/15.9.3.1-1.js
	ecma/Date/15.9.3.1-2.js
	ecma/Date/15.9.3.1-3.js
	ecma/Date/15.9.3.1-4.js
	ecma/Date/15.9.3.1-5.js
	ecma/Date/15.9.3.2-4.js
	ecma/Date/15.9.3.2-5.js
	ecma/Date/15.9.3.8-2.js
	ecma/Date/15.9.3.8-3.js
	ecma/Date/15.9.3.8-4.js
	ecma/Date/15.9.3.8-5.js
	ecma/Date/15.9.4.2.js
	ecma_2/String/match-002.js
	ecma_3/Array/15.4.4.3-1.js
	ecma_3/Expressions/11.9.6-1.js
	ecma_3/RegExp/regress-85721.js
	js1_2/Array/splice2.js
	js1_2/regexp/alphanumeric.js
	js1_2/regexp/control_characters.js
	js1_2/regexp/digit.js
	js1_2/regexp/vertical_bar.js
	js1_2/regexp/whitespace.js
	js1_2/regress/regress-7703.js
	js1_5/Expressions/regress-96526-argsub.js
	js1_5/Expressions/regress-96526-delelem.js
	js1_5/Expressions/regress-96526-noargsub.js
	js1_5/GetSet/getset-004.js
	js1_5/GetSet/getset-005.js
	js1_5/Regress/regress-146596.js

29 regressions found.

[Filip Pizlo]

Rolled out in http://trac.webkit.org/changeset/108307

[Mark Hahnenberg]

Created attachment 128051 [details]
Patch

[Filip Pizlo]

Comment on attachment 128051 [details]
Patch

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

r=me if you fix the two issues.

> Source/JavaScriptCore/jit/JITInlineMethods.h:514
> +        loadPtr(Address(callFrameRegister, (valuesRegister + i) * sizeof(Register)), storagePtr);
> +        storePtr(storagePtr, Address(storageResult, ArrayStorage::vectorOffset() + sizeof(WriteBarrier<Unknown>) * i));
> +        loadPtr(Address(callFrameRegister, (valuesRegister + i) * sizeof(Register) + sizeof(uint32_t)), storagePtr);
> +        storePtr(storagePtr, Address(storageResult, ArrayStorage::vectorOffset() + sizeof(WriteBarrier<Unknown>) * i + sizeof(uint32_t)));

Minor nit:

We typically use load32 and store32 instead of loadPtr and storePtr for JSVALUE32_64.  I think historically that was required because we might compile the JSVALUE32_64 code for 64-bit.  We don't do that anymore, but I kind of like seeing load32/store32 because it reminds me that I'm looking at JSVALUE32_64 code rather than code for JSVALUE64.

> Source/JavaScriptCore/jit/JITInlineMethods.h:529
> +#if CPU(BIG_ENDIAN)
> +        store32(TrustedImm32(static_cast<int>(JSValue::EmptyValueTag)), Address(storageResult, ArrayStorage::vectorOffset() + sizeof(WriteBarrier<Unknown>) * i));
> +        storePtr(TrustedPtr(0), Address(storageResult, ArrayStorage::vectorOffset() + sizeof(WriteBarrier<Unknown>) * i + sizeof(uint32_t)));
> +#else
> +        storePtr(TrustedImmPtr(0), Address(storageResult, ArrayStorage::vectorOffset() + sizeof(WriteBarrier<Unknown>) * i));
> +        store32(TrustedImm32(static_cast<int>(JSValue::EmptyValueTag)), Address(storageResult, ArrayStorage::vectorOffset() + sizeof(WriteBarrier<Unknown>) * i + sizeof(uint32_t)));
> +#endif

You could get rid of the endian-casing if you use OBJECT_OFFSETOF into EncodedValueDescriptor.

[Mark Hahnenberg]

Committed r108681: <http://trac.webkit.org/changeset/108681>