1135001 - Get rid of GetTopJitJSScript calls in Ion ICs

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

We often spend a lot of time in GetPropertyIC::update, SetPropertyIC::update etc. Although this is not ideal and will never be super fast, there are some things we can do to make this path faster.

All Ion caches currently call GetTopJitJSScript, to get to the IonCache. This patch eliminates these calls by passing the outer JSScript as argument to the update functions. It also adds a separate AutoDetectInvalidation constructor that we can easily inline.

It improves a simple getprop micro-benchmark from 460 ms to 375 ms.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

Realized GetTopJitJSScript no longer needs the returnAddr outparam.

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8567009 [details] [diff] [review]
Patch

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

::: js/src/jit/CodeGenerator.cpp
@@ +7799,5 @@
>      saveLive(lir);
>  
>      pushArg(ic->scopeChainReg());
>      pushArg(Imm32(ool->getCacheIndex()));
> +    pushArg(ImmGCPtr(gen->info().script()));

Why do we have to "Push" an extra constant to the stack?  Only one should be enough.

Currently, it seems that we first need the IonScript for finding the IonCache entry, what we could do is replace all the getCacheIndex by a pointer to the data-section of the IonScript.  This way we would not need an extra argument, and we remove the lookup of the IonCache entry.

Then, as we need the outerScript for attaching stubs, we could just bake it in the IonCache structure.

Comment 3

[Brian Hackett [Laid off!]]

Comment on attachment 8567011 [details] [diff] [review]
Patch

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

Passing in an IonCache* or something seems like it would be a little more efficient, but the way this patch does things also seems fine.  Do we have any ability to update ImmPtr or ImmWord values in the generated code after code generation finishes?

Comment 4

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/68277ab46641

(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> Currently, it seems that we first need the IonScript for finding the
> IonCache entry, what we could do is replace all the getCacheIndex by a
> pointer to the data-section of the IonScript.  This way we would not need an
> extra argument, and we remove the lookup of the IonCache entry.

(In reply to Brian Hackett (:bhackett) from comment #3)
> Passing in an IonCache* or something seems like it would be a little more
> efficient, but the way this patch does things also seems fine.  Do we have
> any ability to update ImmPtr or ImmWord values in the generated code after
> code generation finishes?

This is probably doable, but it requires more bookkeeping and patching during compilation. This patch is just a very simple but effective performance improvement; passing an IonCache* is more complicated and it probably isn't much/measurably faster.

Comment 5

[Brian Hackett [Laid off!]]

(In reply to Jan de Mooij [:jandem] from comment #4)
> This is probably doable, but it requires more bookkeeping and patching
> during compilation. This patch is just a very simple but effective
> performance improvement; passing an IonCache* is more complicated and it
> probably isn't much/measurably faster.

Agreed, sorry if I wasn't clear enough earlier.

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/68277ab46641