There are a number of moderate slowdowns being introduced by Ion because it can't throw away ICs when they start generating lots of stubs (i.e. megamorphic sites). Instead, Ion ICs just stop growing at that point, and every subsequent use of the IC travels through all the guards for every stub, and then to the fallback stub-generating handler which does a lot of manual work. On a case-by-case basis, we need to modify Ion ICs so that when they get full, they insert a generic handler at the front of the IC. Every IC will need its own specific generic stub, so this is best implemented on an as-needed basis to achieve targeted performance wins. To start, the SetElement IC is a good target. Fixing it will help Dromaeo-DOM by several percent.

Another advantage of the baseline compiler is that we could tell ahead of time that building an out-of-line IC path would not be worth it, since ion::GetProperty is faster than ion::GetPropertyCache if only just for the register spills.

So this issue is single-handedly making dromaeo's dom-attr test slower in current trunk than in Firefox 17, even though there were significant improvements in the actual DOM parts of it. Furthermore, I've seen this coming up in jQuery profiles a good bit... Any chance of us actually getting somewhere on this in the sorta-near term (like first half of 2012, at this point)?

And by "getting to" I mean "shipping", not "checking in", which is why at this point the best I'm hoping for is 2012Q2.

So I was looking for some quick pick-me-up work and I had an idea about an easy way of implementing IC-disabling in Ion - reset the IC, mark a "disabled" flag on so more stubs don't get added, and just change the fallback C++ function to have a simple path in the case that it notices that the IC is disabled. Patch to disable SetElem ICs is attached. On my test machine, this improves dom-attr's "element.expando" performance from ~321 runs/second to ~390 runs/second - about a 20% improvement. This might actually be worthwhile compared to inserting a custom stub that does the disabling. The overhead of this approach compared to that is not that high. GetTopIonJSScript is cheap as long as you're not trying to get the safepoint index (which is being skipped here) - it'll just touch the top of the stack and get everything it needs, so even the CPU cache is not going to get affected. The rest of the operations (e.g. cache.getScriptedLocation) are quick and constant time. Also, the implementation is really simple and can be extended to all the other ICs with little effort. Boris: on what tests do we need IC disabling in order to gain performance? This patch attacks DOM-attr/element.expando. Are there others?

Comment on attachment 695520 [details] [diff] [review] Disable GetElem ICs Review of attachment 695520 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonCaches.h @@ +169,5 @@ > void updateBaseAddress(IonCode *code, MacroAssembler &masm); > + > + // disable the IC. > + void disable(); > + inline bool isDisabled() const { return disabled_; } nit: expand this to { return disabled_; }

Nits addressed. https://hg.mozilla.org/integration/mozilla-inbound/rev/45853b5892f6

> Boris: on what tests do we need IC disabling in order to gain performance? Sorry, I wasn't cced... The tests where I know this is/was biting us are: 1) dom-attr's "element.expando" for GetElement (and maybe "element.expando = value" for SetElement). 2) Various stuff in cssquery-* for GetElement (in fact, the 3% speedup on inbound in those is what brought me here).

The previous patch wasn't type monitoring the result of the getelems. Fixing that.

Comment on attachment 701297 [details] [diff] [review] Fix type monitoring of results of disabled GetElem ICs Review of attachment 701297 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonCaches.cpp @@ +1703,5 @@ > RootedScript script(cx); > jsbytecode *pc; > cache.getScriptedLocation(&script, &pc); > RootedValue lval(cx, ObjectValue(*obj)); > The review tool is whining about extra whitespace here, which already existed in the file. Would you mind deleting it as part of this patch?