792631 - IonMonkey: Slow property access perf regression.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Michael Bebenita [:mbx]]

The following code is ~ 10x slower in IonMonkey, 65s (js) vs 6s with (js --no-ion) on a MacPro 2x2.4 GHz Quad-Core Intel Xeon:

function bar() {
  var a = {x: 1}, b = {y: 2};
  var o = a;
  var s = 0;
  for (var i = 0; i < 500000000; i++) {
    s += o.x !== undefined ? o.x : (o.y !== undefined ? o.y : 3);
    if (i === 1000) {
      o = b;
    }
  }
  return s;
}

print(bar());

Comment 1

[Shu-yu Guo [:shu]]

Some more context: optimizing ActionScript property accesses in Shumway (the Flash-in-JS research project) depends on this being fast.

Comment 2

[Sean Stangl [:sstangl]]

Testcase spends all its time in the GetPropertyCache, never actually caching anything, searching through ShapeTables, etc. When a shape lookup fails, we don't cache |undefined| return.

Comment 3

[Boris Zbarsky [:bzbarsky]]

JM has a missing property IC to handle this case, I believe...

Comment 4

[Nicolas B. Pierron [:nbp]]

Attachment: Add Ion IC for missing properties.

Comment 5

[Nicolas B. Pierron [:nbp]]

Comment on attachment 673524 [details] [diff] [review]
Add Ion IC for missing properties.

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

::: js/src/ion/IonCaches.cpp
@@ +1168,5 @@
>      if (!JSObject::lookupProperty(cx, obj, name, &holder, &shape))
>          return false;
>  
> +    if (!IsCacheableGetPropReadSlot(obj, holder, shape) &&
> +        !IsCacheableNoProperty(obj, holder, shape, NULL, output())) {

I fixed a failure here, I added the pc and checked for JSOP_CALLELEM in IsCacheableNoProperty to prevent IC with __noSuchMethod__.

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 673524 [details] [diff] [review]
Add Ion IC for missing properties.

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

::: js/src/ion/IonCaches.cpp
@@ +283,5 @@
> +                               Address(holderReg, JSObject::offsetOfShape()),
> +                               ImmGCPtr(holder->lastProperty()),
> +                               &prototypeFailures);
> +            } else {
> +                // The property does not exists. Guard on everything in the

s/does not exists/does not exist/

Comment 8

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a2c17905a17

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4a2c17905a17

Comment 11

[Nicolas B. Pierron [:nbp]]

Attachment: Backport patches.

This patch includes the original patch plus Bug 807035 and Bug 807047 with a little modification (in methodjit/PolyIC.cpp) to change the MarkNotIdempotent call because the unsafeGet function does not exists in Aurora.

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
N/A, a similar feature was added to JM in fx17, and Ion was added in fx18 without the equivalent. The goal is to back-port this change to fx18 to avoid regressions

User impact if declined:
Prevent IonMonkey regression feeling.

Testing completed (on m-c, etc.):
Yes, Bug 807035 and Bug 807047 are now fixed an no new bug seems to blame this patch.

Risk to taking this patch (and alternatives if risky): 
Low risk.

String or UUID changes made by this patch:
N/A

Comment 12

[bhavana bajaj [:bajaj]]

Comment on attachment 679760 [details] [diff] [review]
Backport patches.

Approving on Aurora . Bug is tracking 18,been on central for a week,low risk & is needed to avoid possible regressions due to IonMonkey landing in 18.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e4231c5c2147