Setting named properties on array objects de-optimize all future array accesses. I suspect the JS engine panics whenever a named property is set on an array and invalidates code. See attached example. If "Test 2" is commented out performance is fine, otherwise array accesses slow down by a factor of ~30X. V8 has similar behavior, but it appears to be worse. In SM, this appears to happen when you override a property defined on Array.prototype, in V8 this happens for any named properties set on array objects.

Calling a cloned version of "accessIndirect" in Test 3 avoids the problem, so I'm guessing the issue here is that "accessIndirect" gets invalidated once the array shape changes and never gets recompiled.

There're 2 main things here: (1) Inlining non-singleton-typed functions (i.e., functions that aren't defined in the global scope). (2) ICs installed because of context insensitivity. (1) needs some TI background. Object, functions included, types are either singleton or non-singleton, singleton here meaning we can get to the single function having the type. Global functions are singleton typed, functions created inside other functions are not. Because of bug 883973, inlining of non-singleton type functions is turned off. This means the |indexGet| that lives on the arrays themselves (not on the prototype), since they're being created inside functions, can't be inlined. (2) There's only one copy of |indirectAccess| but 3 different array objects with different allocation site-keyed types are passed to it, so getting the |indexGet| property requires an IC. This is a slowdown that can't really be avoided at the moment without manual cloning. It also plays into (1). I *think* that if |indexGet| *could* be inlined, we would be able to compile a poly function dispatch inline despite there being 2 different targets.

Hmm. Comment 2 is pretty specific to this testcase's use of polymorphic indexGet, but comment 0 and the testcase claim this is a general named prop problem. Which is correct?

(In reply to Boris Zbarsky (:bz) from comment #3) > Hmm. Comment 2 is pretty specific to this testcase's use of polymorphic > indexGet, but comment 0 and the testcase claim this is a general named prop > problem. Which is correct? Comment 2 is my attempt at explaining what I saw in the engine causing the slowdown. I think mbx's comment is how it looks from the outside, without knowing more about what Ion is doing exactly.

So bug 893038 turned inlining of non-singleton typed functions back on. This test case is still slow though, because to be able to inline polymorphic targets, some very specific criteria must be met: all targets must be singleton typed and live on the prototypes of the set of receiver objects. That is, in this example, all versions of |indexGet| must be singleton typed and must live on the [[Prototype]] of all 3 arrays. This is clearly not true, so Ion doesn't do a polymorphic inline. Short term workaround is to ensure that each callsite to |indexGet| is monomorphic, preferably both in the receiver and the property. Medium term fix would be to support polymorphic inlining for non-singleton typed objects. Long term fix is to deal with context sensitivity somehow.

CC'ing djvj since he wrote the poly inline stuff afaik.

Just re-tested this: SM: Access Indirect: 36 Access Indirect Override: 845 Access Indirect: 882 V8: Access Indirect: 53 Access Indirect Override: 117 Access Indirect: 102 JSC: Access Indirect: 42 Access Indirect Override: 522 Access Indirect: 3897 I.e., we're fastest by a substantial margin until we deoptimize. After that, we're terribly slow, though not as terribly as JSC is. Note that V8 also takes a hit, but only to about 2x, not close to 25x like us.

Jandem, is there anything we can do about this?

(In reply to Till Schneidereit [:till] from comment #8) > Jandem, is there anything we can do about this? Sorry for the late response. Shu is right and the polymorphic inlining needs to handle the lambda case. Will see how hard this is.

This patch makes polymorphic inlining work with lambdas, it turned out to be pretty straight-forward. For the attached micro-benchmark: Before / After / V8 Access Indirect: 21 / 21 / 31 Access Indirect Override: 603 / 79 / 85 Access Indirect: 639 / 107 / 111 So it's a lot faster than before and a bit faster than V8 in all cases.

\o/

Just realized the previous patch had a change that isn't necessary.

Comment on attachment 8550228 [details] [diff] [review] Patch Review of attachment 8550228 [details] [diff] [review]: ----------------------------------------------------------------- Awesome perf improvement there :) ::: js/src/jit/MIR.h @@ +9373,5 @@ > // Map from JSFunction* -> MBasicBlock. > struct Entry { > JSFunction *func; > + // func's TypeObject or nullptr if func is a singleton. If non-null, > + // FunctionDispatch should guard on this, instead of func. The comment here is unclear. The first 'if' refers to the func field, the second 'if' to the funcType field. Something like the following might be clearer: // if |func| has a singleton type, |funcType| is null. Otherwise, |funcType| holds the TypeObject for |func|, and dispatch guards on the type instead of directly on the function.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/99ee26ed26e5 I'm suspicious that devtools tests weren't happy, since there was a sudden increase in their constant level of failure and a couple of new unfiled failures, but they're always a bit iffy and that was nothing but suspicion. b2g desktop linux64 gaia unit test (that's Gu), calendar/test/unit/interval_tree_test.js timed out 100% on ten tries on your push, timed out 0% on the push before. The fact that it was green on the push above you, and then went back to timing out on every push above that... I can't explain.

(In reply to Phil Ringnalda (:philor) from comment #15) > b2g desktop linux64 gaia unit test (that's Gu), > calendar/test/unit/interval_tree_test.js timed out 100% on ten tries on your > push, timed out 0% on the push before. The fact that it was green on the > push above you, and then went back to timing out on every push above that... > I can't explain. This one was actually a crash; maybe the crash reporter confused the test harness. Just pushed a fix for that to Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1223e97761ad (Full Try run to make sure I don't miss something this time.)