Created attachment 8928690 [details] [diff] [review] Scalar replacement for NewArrayCopyOnWrite 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)
Attachment #8928690 - Flags: feedback?(nicolas.b.pierron) → feedback+
Created attachment 8929772 [details] [diff] [review] v1 - Scalar replacement for NewArrayCopyOnWrite 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.
Created attachment 8929827 [details] [diff] [review] v2 - Scalar replacement for NewArrayCopyOnWrite 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
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 email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/52c26719155e Use Scalar Replacement on MNewArrayCopyOnWrite arrays. r=nbp
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.