Nicolas B. Pierron [:nbp] Reporter
	Description • 8 years ago

The test case provided in [1] uses NewArrayCopyOnWrite, when used with JavaScript Arrays, while we should expect Scalar Replacement to remove the Array allocation completely, thus not even relying on the GC.

Based on the test case, this should never happen as the Array is never used after the loop.  Thus "Eliminate phis" should remove the phi from the loop header, and "Scalar Replacement" should remove the array properties accesses.

We have 2 issues in this test case:
  1. MNewArrayCopyOnWrite is not identified as an array that we can safely recover on bailout.
  2. MConvertElementsToDoubles is not yet supported (Bug 1136737).

[1] http://blog.leaningtech.com/2016/07/faster-typedarray-creation-on.html

	Tom S [:evilpie] Assignee
	Comment 1 • 7 years ago

I noticed that in the six-speed template_string_tag.es5 tests we had a LoadElement for simple string value in an array that never changes. Turns out we never implemented scalar replacement for COW arrays. This patch is still has some room for improvement, but actually works for that test case.

	Tom S [:evilpie] Assignee
	Comment 2 • 7 years ago

I added support for encoding the initialHeap with the recover instruction. I also ported the tests from ion/recover-array.js where possible. Looked good locally, but just pushed to try as well: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9121387d0635122f4c8ca61d9992cf00b00a4a91.

	Tom S [:evilpie] Assignee
	Comment 3 • 7 years ago

I had to replace MOZ_ASSERT with MOZ_RELEASE_ASSERT in RArrayState::recover, otherwise "val" would be unused.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1bf8fef2a740a680741a5f7a311b9227c69ee35b

	Nicolas B. Pierron [:nbp] Reporter
	Comment 4 • 7 years ago

Comment on attachment 8929827 [details] [diff] [review]
v2 - Scalar replacement for NewArrayCopyOnWrite

Review of attachment 8929827 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit-test/tests/ion/recover-cow-arrays.js
@@ +10,5 @@
> +if (getJitCompilerOptions()["ion.forceinlineCaches"])
> +    setJitCompilerOption("ion.forceinlineCaches", 0);
> +
> +// This function is used to force a bailout when it is inlined, and to recover
> +// the frame which is inlining this function.

nit: (maybe-self-nit?) the frame of the function in which this function is inlined.

@@ +125,5 @@
> +    assertRecoveredOnBailout(a, true);
> +    return a.length;
> +}
> +
> +// We don't handle COW array writes

Q: Why not?

@@ +155,5 @@
> +    assertRecoveredOnBailout(a, false);
> +    return a.length;
> +}
> +
> +// Same test as the previous one, but the Array.prototype is changed to reutn

nit: return

	Tom S [:evilpie] Assignee
	Comment 5 • 7 years ago

Thanks for reviewing.
Disallowing writing to COW arrays made some stuff simpler, but I actually think it wouldn't be that hard to fix now. I will try to do that next.

	Pulsebot
	Comment 6 • 7 years ago

Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/52c26719155e
Use Scalar Replacement on MNewArrayCopyOnWrite arrays. r=nbp

	Cosmin Sabou [:CosminS]
	Comment 7 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/52c26719155e