Closed Bug 1288392 Opened 8 years ago Closed 7 years ago

Use Scalar Replacement on MNewArrayCopyOnWrite arrays.

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: nbp, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

Attachments

(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). [1] http://blog.leaningtech.com/2016/07/faster-typedarray-creation-on.html
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: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9121387d0635122f4c8ca61d9992cf00b00a4a91.
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. https://treeherder.mozilla.org/#/jobs?repo=try&revision=1bf8fef2a740a680741a5f7a311b9227c69ee35b
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 evilpies@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/52c26719155e Use Scalar Replacement on MNewArrayCopyOnWrite arrays. r=nbp
Status: NEW → RESOLVED
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.

Attachment

General

Created:
Updated:
Size: