Scalar Replacement: Allow writing to MNewArrayCopyOnWrite arrays

RESOLVED FIXED in Firefox 59

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: evilpie, Assigned: jandem)

Tracking

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

2 years ago
I didn't implement this in bug 1288392, because it seemed to make some things simpler. This might just work now however, or maybe initFromTemplateObject needs to be adjusted.
Priority: -- → P3
Reporter

Updated

2 years ago
Blocks: 1425687
Assignee

Comment 1

2 years ago
Posted patch Patch (obsolete) — Splinter Review
This implements scalar replacement for stores to COW arrays. We now also support MConvertElementsToDoubles.

With these changes the raytracer in bug 1425687 is > 2x faster.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8938415 - Flags: review?(nicolas.b.pierron)
Attachment #8938415 - Flags: review?(evilpies)
Reporter

Comment 2

2 years ago
Comment on attachment 8938415 [details] [diff] [review]
Patch

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

Looks pretty good. Are writes guaranteed to be preceded by CopyElementsOnWrite?
Attachment #8938415 - Flags: review?(evilpies) → feedback+
Comment on attachment 8938415 [details] [diff] [review]
Patch

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

Everything looks good except for the next comment:

::: js/src/jit/ScalarReplacement.cpp
@@ +1438,5 @@
> +            oom_ = true;
> +            return;
> +        }
> +
> +        state_->setCopyElementsForWrite();

This flag should be updated in ArrayMemoryView::mergeIntoSuccessorState, otherwise the following test case might fail:

function arrayCopyWrite1(i) {
    // (block 0)
    var a = [1, 1];
    if (i % 2 == 0)
        // (block 1)
        a[0] = a[0] + 1;
    // (block 2)
    assertEq(a[0], 1);
    assertEq(a[1], 2);
    assertRecoveredOnBailout(a, true);
    resumeHere();
    return a.length;
}


The problem being that block 0, will create block 2, MArrayState without the copy flag. When the mutated data from block 1 is merged into block 2, the function mergeIntoSuccessorState will add the Phi nodes but forget to set the copy flag on block 2 MArrayState.

Note, the same applies, if the array mutation is nested in a for-loop:

function arrayCopyWrite2(i) {
    var a = [1, 1];
    for (var x = 0; x < 2; x++) {
        if (x % 2 == 1)
            resumeHere(); // (MArrayState needs the Copy flag)
        else
            a[0] = a[0] + 1;
    }
    assertEq(a[0], 1);
    assertEq(a[1], 2);
    assertRecoveredOnBailout(a, true);
    return a.length;
}

The easiest way, would be to make the Copy flag be a boolean operand of the MArrayState vector which would then be merged as a Phi operand by mergeIntoSuccessorState, which should solve both issues seen above.
Attachment #8938415 - Flags: review?(nicolas.b.pierron) → feedback+
Assignee

Comment 4

2 years ago
Posted patch Patch (obsolete) — Splinter Review
Good catch! This adds the copyElementsForWrite flag as operand to MArrayState and adds some tests.
Attachment #8938415 - Attachment is obsolete: true
Attachment #8940191 - Flags: review?(nicolas.b.pierron)
Assignee

Comment 5

2 years ago
Another option is to check in RArrayState::recover whether we have an element that's != the COW element and then call maybeCopyElementsForWrite. That way we don't have to track copyElementsForWrite at all. I can do that too..
Flags: needinfo?(nicolas.b.pierron)
(In reply to Jan de Mooij [:jandem] from comment #5)
> Another option is to check in RArrayState::recover whether we have an
> element that's != the COW element and then call maybeCopyElementsForWrite.
> That way we don't have to track copyElementsForWrite at all. I can do that
> too..

I guess this would work, and even do some de-duplication.
I think this is even better than keeping track of the copy flag.
Flags: needinfo?(nicolas.b.pierron)
Assignee

Comment 7

2 years ago
Posted patch PatchSplinter Review
This works and is a much simpler patch.
Attachment #8940191 - Attachment is obsolete: true
Attachment #8940191 - Flags: review?(nicolas.b.pierron)
Attachment #8940213 - Flags: review?(nicolas.b.pierron)
Assignee

Updated

2 years ago
Duplicate of this bug: 1136737
Attachment #8940213 - Flags: review?(nicolas.b.pierron) → review+

Comment 9

2 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ceb61da04fd8
Make scalar replacement of arrays work in more cases (writes to COW elements, double elements). r=nbp

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ceb61da04fd8
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.