Closed Bug 1425687 Opened 7 years ago Closed 7 years ago

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


(Core :: JavaScript Engine, defect, P3)




Tracking Status
firefox59 --- fixed


(Reporter: jandem, Assigned: jandem)





(1 file)

We have a big perf cliff on the JS version of the raytracer here:

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.
Attached 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]

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!

Attachment #8940669 - Flags: review?(evilpies) → review+
Pushed by
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.
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.