925201 - IonMonkey: SetElementIC dense array stub incorrectly handles holes.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Eric Faust [:efaust]]

The dense array stub doesn't know anything about holes, so if you were supposed to call a setter, you'll never get there.

Comment 1

[Eric Faust [:efaust]]

So, Shu and I discussed two different approaches to solving this problem, both of which will clearly work to solve the problem.

I suggested a simple lookup and branch on the value we would return. If it's a hole, then simply fall back out of the stub. This has the obvious downside that it adds a load and a branch on the hot path, even if they would all succeed.

Shu suggested using the TI guarding scheme that the rest of the Get/Set Elem schemes use. The downside here is that if it does happen, even if all accesses would have succeeded because the defined properties don't line up with the holes in the arrays, we still invalidate the whole script. The upside, and I agree with him that it's a sizable one, is that /very/ few people want to define the property "2" on the prototype chain of an ArrayObject.

The question really comes down to whether we think the ICs are a standalone as the last bastion of hope againt VM fallback (which they really kinda are, and we have treated them as such elsewhere), or if IonMonkey is an optimizing compiler first, and we should just use our optimization techniques to protect the ICs as well.

Needinfo Jan, since this is more or less a policy decision. I think I favor the extra branch slightly, due to the "last bastion" argument, though admittedly it will hack somewhat into performance, and the net benchmark effects have yet to be seen. Admittedly, both approaches have their pathological cases. Since property definitions (especially setters) are rare events, invalidation may well be a feasible solution performance-wise.

Comment 2

[Jan de Mooij [:jandem]]

Good question. I think the extra branch shouldn't matter too much when it's not taken. What's worse is that it can cause us to call into the VM when we don't really have to. SetElementIC::attachDenseElement has code to bump the initialized length, this is useful when filling an array like this:

  var a = [];
  for (var i=0; i<1000; i++)
      a[i] = 0;

But this will always overwrite holes so if we *do* add that extra branch we may as well remove this bump-initialized-length case; it's a bit unfortunate.

How about the following: we add a bit to the SetElementIC, something like protoHasIndexedProperties. This is |false| only if (1) the baseline IC accessed dense elements and (2) we used TI to guard there are no extra properties. If somebody touches something on the prototype, we invalidate and the IC uses the extra branch next time.

I think invalidation is okay in this case, we probably have to invalidate most Ion code anyway for the inlined GETELEM/SETELEM paths...

Comment 3

[Eric Faust [:efaust]]

Attachment: Fix

Fix for both sequential and parallel set element ICs.

Comment 4

[Shu-yu Guo [:shu]]

Comment on attachment 816094 [details] [diff] [review]
Fix

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

Sorry for the delay, looks good to me with minor comment nits addressed.

::: js/src/jit/IonBuilder.cpp
@@ +7235,5 @@
>          return true;
>      }
>  
> +    // We can avoid worrying about holes in the IC if we know a priori we are safe
> +    // from them.

Nit: I'd add an explanation for why we are safe from them, namely that TI is guarding against the proto chain having no indexed properties.

::: js/src/jit/IonCaches.cpp
@@ +3457,5 @@
>          BaseIndex target(elements, index, TimesEight);
>  
> +        // If TI cannot help us deal with HOLES on the proto chain, we have to
> +        // be very careful to check for ourselves. Start by only allowing things
> +        // within the initialized length

Weird wording, makes it sound like there are holes on the prototype chain itself. Maybe "If TI cannot guarantee the lack of indexed properties on the prototype chain, we must check for holes ourselves."

Comment 5

[Eric Faust [:efaust]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/79287e1634a4

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/79287e1634a4