Use Scalar Replacement on MNewArrayCopyOnWrite arrays.

RESOLVED FIXED in Firefox 59

Status

()

P5
normal
RESOLVED FIXED
2 years ago
5 months ago

People

(Reporter: nbp, Assigned: evilpie)

Tracking

(Blocks: 1 bug)

Trunk
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

2 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

Updated

2 years ago
Priority: -- → P5
(Assignee)

Comment 1

a year ago
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)
(Assignee)

Updated

a year ago
Blocks: 1177735
(Reporter)

Updated

a year ago
Attachment #8928690 - Flags: feedback?(nicolas.b.pierron) → feedback+
(Assignee)

Comment 2

a year ago
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.
Attachment #8928690 - Attachment is obsolete: true
Attachment #8929772 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 3

a year ago
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
Attachment #8929772 - Attachment is obsolete: true
Attachment #8929772 - Flags: review?(nicolas.b.pierron)
Attachment #8929827 - Flags: review?(nicolas.b.pierron)
(Reporter)

Comment 4

a year 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
Attachment #8929827 - Flags: review?(nicolas.b.pierron) → review+
(Assignee)

Comment 5

a year 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.

Comment 6

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

Comment 7

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/52c26719155e
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
(Assignee)

Updated

a year ago
Blocks: 1420172
status-firefox50: affected → ---
You need to log in before you can comment on or make changes to this bug.