851552 - IonMonkey: disable caches after failing to add new stub for some times

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

When running jpeg2000 we are hitting the following a lot:

8% js::ion::GetElementIC::update(JSContext*, unsigned int, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>)
That shouldn't take like 8% of our time

For these caches in jpeg2000 we never add any stub, but we try every time. So cache.stubCount_ = 0. I think after some time we should decide to disable the cache. Just like we currently do when there are to much stubs.

Doing this improved the benchmark from 7.8s to 6.7s.
That is a 16% improvement.

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: Disable caches when attaching a stub failed too many times

Disable the GetElementIc cache when it has no stubs, but we repeatedly try to add a new stub and fail because the code doesn't handle that particular input.

Comment 2

[Kannan Vijayan [:djvj]]

(In reply to Hannes Verschore [:h4writer] from comment #1)
> Created attachment 726186 [details] [diff] [review]
> Disable caches when attaching a stub failed too many times
> 
> Disable the GetElementIc cache when it has no stubs, but we repeatedly try
> to add a new stub and fail because the code doesn't handle that particular
> input.

What's causing us to not add stubs in these cases?  Are they not optimizable?

Comment 3

[Hannes Verschore [:h4writer]]

(In reply to Kannan Vijayan [:djvj] from comment #2)
> What's causing us to not add stubs in these cases?  Are they not optimizable?

For JPEG2000 that is bug 851792. I also have a patch for that, only need some polish.

Therefore this bug is solely to not bite to much in the execution time when we haven't added a stub yet or decided we don't want one.

Comment 4

[Kannan Vijayan [:djvj]]

I don't know if disabling stubs like this should actually be done.

If we're hitting the fallback path on IC chains too much, then it's an indication that we have a case to optimize that we're not optimizing.  That optimization might be adding a generalized optimized stub, but that should be a conscious "positive" choice, not the "negative" choice of "we're hitting the fallback too much, let's stop trying to attach new stubs".

It seems like bug 851792 is the real fix for the perf issue you identified.

I don't understand what you mean when you say "we haven't added the stub yet".  The stub would be added the first time that case is seen, no?

Comment 5

[Hannes Verschore [:h4writer]]

"we haven't added the stub yet" == "we haven't created code that would create a stub for that case"

I'm not sure in this case that is a difference. Because I only disable the cache when there are no stubs and we saw 16 times not getting a stub attached. I.e. in this case it would be the same to add a "general stub" or to disable it ...

Comment 6

[Kannan Vijayan [:djvj]]

Comment on attachment 726186 [details] [diff] [review]
Disable caches when attaching a stub failed too many times

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

::: js/src/ion/IonCaches.cpp
@@ +1499,5 @@
>  
>      return true;
>  }
>  
> +const size_t GetElementIC::MAX_FAILED_UPDATES = 16;

Nit: This can just be initialized in the header itself.

Comment 7

[Hannes Verschore [:h4writer]]

I didn't moved the MAX_FAILED_UPDATES, as we also have MAX_STUBS and both are in the .cpp file. I think we should do same for both. I.e. if you still want to move it, I'll move both into the header, else I keep them both in the .cpp file

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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