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

RESOLVED FIXED in Firefox 59

Status

()

defect
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

()

Attachments

(1 attachment)

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.
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.
Priority: -- → P3
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.
Depends on: 1420172
(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.
Posted patch PatchSplinter Review
With this patch we require at least 5 elements for COW arrays.
Attachment #8940669 - Flags: review?(evilpies)
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
Attachment #8940669 - Flags: review?(evilpies) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed7d70165892
Don't use copy-on-write elements for small arrays. r=evilpie
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.
https://hg.mozilla.org/mozilla-central/rev/ed7d70165892
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.