831349 - IonMonkey: IonCache has*Stub flags are not reset when caches are flushed.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

We should reset the has*Stub booleans. Without this reset, for a function which is kept alive across GCs, such as mozRequestAnimationFrame callbacks, we will get rid of all stubs but we would not reset the has*Stub flags.  This would cause us to fallback to a function call instead of using a generic stub path.

I don't think this show up in benchmarks yet, but we should probably add one to the assorted benchmark.  We should also back-port this patch as this might be a noticeable performance issue.

Comment 1

[Alex Keybl [:akeybl]]

(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #0)
> I don't think this show up in benchmarks yet, but we should probably add one
> to the assorted benchmark.  We should also back-port this patch as this
> might be a noticeable performance issue.

Let's track once this is a known perf issue. Sounds like this behavior already shipped in FF18, and we don't yet have any data around it.

Comment 2

[Nicolas B. Pierron [:nbp]]

(In reply to Alex Keybl [:akeybl] from comment #1)
> (In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #0)
> > I don't think this show up in benchmarks yet, but we should probably add one
> > to the assorted benchmark.  We should also back-port this patch as this
> > might be a noticeable performance issue.
> 
> Let's track once this is a known perf issue. Sounds like this behavior
> already shipped in FF18, and we don't yet have any data around it.

The reason why we don't even have metric there is that the code generated by IonMonkey is considered as too critical to even think of adding an extra counter in the functions.  In practice I think it would be fine to maintain the call-counter for *large* functions and use it to estimate if we are slowing down over-time.

If you want to test for such a thing, you can create a simple test case and measure the time taken by the frame, such as:

function foo(obj) {
  return obj.a + obj.b;
}

var obj1 = {a: "1", b: "2"};
var obj2 = {b: "b", a: "a"};

function callback() {
  var start = performance.now();
  for (var i = 0; i < 1000; ++i) foo(obj1);
  for (var i = 0; i < 1000; ++i) foo(obj2);
  var end = performance.now();
  dump(end - start);
  mozRequestAnimationFrame(callback);
}

mozRequestAnimationFrame(callback);


Such test case will work best before the 8 GC triggered by the allocations of Strings which are producing garbage.  The "foo" function would be compiled, but it will remain because of it is indirectly called by the "requestAnimationFrame" function.  Cache entries will be discarded, but the stub counter will not be reset, causing a wring overflow of stubs.

I think this might be a major performance issue of IonMonkey, as we are falling back into VM functions calls.

Comment 3

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

(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #2)
> (In reply to Alex Keybl [:akeybl] from comment #1)
> > Let's track once this is a known perf issue. Sounds like this behavior
> > already shipped in FF18, and we don't yet have any data around it.
> 
> The reason why we don't even have metric there

"Data/known perf issue" as in, data on whether this is affecting people in the wild. Right now this is purely speculative. It sounds like a good idea to fix it, but no need to track yet.

Comment 4

[Nicolas B. Pierron [:nbp]]

Attachment: Shell micro benchmark.

Micro benchmark which illustrate the performance issue when run with --ion-inlining=off.

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: Reset Ion cache flags when flushed.

Running the micro benchmark with --ion-inlining=off gives the following results:
Before: ~3600ms
After: ~1900ms

Comment 6

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

Comment on attachment 727955 [details] [diff] [review]
Reset Ion cache flags when flushed.

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

::: js/src/ion/IonCaches.cpp
@@ +1033,5 @@
>  
>  void
> +GetPropertyIC::reset()
> +{
> +    this->IonCache::reset();

is |this->| needed?

Comment 7

[Nicolas B. Pierron [:nbp]]

(In reply to David Anderson [:dvander] from comment #6)
> ::: js/src/ion/IonCaches.cpp
> @@ +1033,5 @@
> >  
> >  void
> > +GetPropertyIC::reset()
> > +{
> > +    this->IonCache::reset();
> 
> is |this->| needed?

No it is not, this is an old habit that I have due to the inheritance of parametrized classes. :(

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

Comment 8

[Joe Drew (not getting mail)]

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