Closed Bug 1288392 Opened 8 years ago Closed 7 years ago

Use Scalar Replacement on MNewArrayCopyOnWrite arrays.


(Core :: JavaScript Engine: JIT, defect, P5)




Tracking Status
firefox59 --- fixed


(Reporter: nbp, Assigned: evilpie)


(Blocks 1 open bug)



(1 file, 2 obsolete files)

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

Priority: -- → P5
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.
Assignee: nobody → evilpies
Attachment #8928690 - Flags: feedback?(nicolas.b.pierron)
Blocks: six-speed
Attachment #8928690 - Flags: feedback?(nicolas.b.pierron) → feedback+
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:
Attachment #8928690 - Attachment is obsolete: true
Attachment #8929772 - Flags: review?(nicolas.b.pierron)
I had to replace MOZ_ASSERT with MOZ_RELEASE_ASSERT in RArrayState::recover, otherwise "val" would be unused.
Attachment #8929772 - Attachment is obsolete: true
Attachment #8929772 - Flags: review?(nicolas.b.pierron)
Attachment #8929827 - Flags: review?(nicolas.b.pierron)
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
Attachment #8929827 - Flags: review?(nicolas.b.pierron) → review+
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.
Pushed by
Use Scalar Replacement on MNewArrayCopyOnWrite arrays. r=nbp
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Blocks: 1420172
You need to log in before you can comment on or make changes to this bug.