807498 - IonMonkey: Implement disabling of Ion ICs when they get too big

Status: NEW

(Core :: JavaScript Engine, defect)

Description

[Kannan Vijayan [:djvj]]

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.

Comment 1

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

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.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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)?

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

Comment 4

[Kannan Vijayan [:djvj]]

Attachment: Disable GetElem ICs

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 5

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

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_;
}

Comment 6

[Kannan Vijayan [:djvj]]

Nits addressed.

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

Comment 7

[Boris Zbarsky [:bzbarsky]]

> 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).

Comment 8

[Kannan Vijayan [:djvj]]

Attachment: Fix type monitoring of results of disabled GetElem ICs

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

Comment 9

[Sean Stangl [:sstangl]]

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?

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/45853b5892f6

Comment 11

[Kannan Vijayan [:djvj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d20ea031c033

Comment 12

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/d20ea031c033