1425687 - Don't use copy-on-write elements for small arrays

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Jan de Mooij [:jandem]]

We have a big perf cliff on the JS version of the raytracer here: https://sniklaus.com/blog/raytracer

We keep calling into the VM to copy COW arrays, but these arrays are really small (like [0, 0, 0]) so this is just silly.

If I change the emitter to use COW arrays only if we have more than 8 elements, our time improves from 400 to 180 or so.

We should also optimize the COW path, probably do nursery allocation inline in JIT code.

Comment 1

[Jan de Mooij [:jandem]]

Given the huge perf difference, it's possible the problem here is that scalar replacement of COW arrays needs to support StoreElement, and maybe we need a recover instruction for MNewCopyOnWriteArray. I'll look into this later.

Comment 2

[Tom S [:evilpie]]

This is quite the performance footgun. We definitely don't support StoreElement on COW arrays. We only just started supporting reading from COW arrays in bug 1288392. I had already filed bug 1420172 for supporting scalar replacement for COWs, but as you said we should probably also not be using cows for this. These arrays have three simple int elements.

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #0)
> If I change the emitter to use COW arrays only if we have more than 8
> elements, our time improves from 400 to 180 or so.

Bug 1420172 makes scalar replacement work for these COW arrays and I see a similar improvement from that. I still want to fix this bug though - I added some logging and we sometimes use COW arrays for arrays with 1 element. The VM call to copy such elements is a lot more expensive than just doing a single store.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Patch

With this patch we require at least 5 elements for COW arrays.

Comment 5

[Tom S [:evilpie]]

Comment on attachment 8940669 [details] [diff] [review]
Patch

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

::: js/src/jit-test/tests/ion/recover-cow-arrays.js
@@ +29,5 @@
>  function escape(arr) { global_arr = arr; }
>  
> +function checkCOW() {
> +    assertEq(hasCopyOnWriteElements([1, 2, 3, 4]), false);
> +    // If this fails, we should probably update the tests below!

Nice

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed7d70165892
Don't use copy-on-write elements for small arrays. r=evilpie

Comment 7

[Guilherme Lima]

The patch caused a big regression on https://arewefastyet.com/#machine=29&view=single&suite=six-speed&subtest=spread-literal-es5
But it also improved a lot https://arewefastyet.com/#machine=29&view=single&suite=misc&subtest=typedobj-write-struct-field-standard

Comment 8

[Jan de Mooij [:jandem]]

Hm the spread-literal-es5 test does:

  var ret = [1];
  ret.push(1, 2, 3);
  return ret;

I wonder if we were able to optimize this away somehow. Because this is definitely one of these cases where the patch improves things.

Comment 9

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/ed7d70165892