Closed Bug 1135001 Opened 5 years ago Closed 5 years ago

Get rid of GetTopJitJSScript calls in Ion ICs

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #8567009 - Flags: review?(bhackett1024)
Attached patch PatchSplinter Review
Realized GetTopJitJSScript no longer needs the returnAddr outparam.
Attachment #8567009 - Attachment is obsolete: true
Attachment #8567009 - Flags: review?(bhackett1024)
Attachment #8567011 - Flags: review?(bhackett1024)
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 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?
Attachment #8567011 - Flags: review?(bhackett1024) → review+
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.
(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.
https://hg.mozilla.org/mozilla-central/rev/68277ab46641
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.