IonMonkey: Remove slow IonContext lookup in AutoFlushCache constructor

RESOLVED FIXED in mozilla26

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks 1 bug)

unspecified
mozilla26
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Assignee

Description

6 years ago
Posted patch PatchSplinter Review
The testcase in bug 856128 has > 17 million calls to SetPropertyIC::update. The AutoFlushCache constructor (and functions it calls) show up very high in the profile, we spend a ton of time there looking up the current IonContext and IonRuntime.

This is no longer necessary, all callers can easily get the IonRuntime from somewhere else and pass it to AutoFlushCache.

The patch does this, simplifies the code a bit and makes AutoFlushInhibitor look a bit more like AutoFlushCache. This makes the test in bug 856128 at least a few hundred ms faster (and should help all JS code that spends a lot of time in IC slow paths).
Attachment #800106 - Flags: review?(hv1989)
Assignee

Comment 1

6 years ago
FWIW, this should also help bug 907369 a bit:

(In reply to Boris Zbarsky [:bz] from comment #5)
> For the object case, we end up doing a bunch of ion::GetPropertyId::update,
> which spends 13% of its time in AutoFlushCache ctors.
Assignee

Updated

6 years ago
Summary: IonMonkey; Remove slow IonContext lookup in AutoFlushCache constructor → IonMonkey: Remove slow IonContext lookup in AutoFlushCache constructor
Comment on attachment 800106 [details] [diff] [review]
Patch

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

::: js/src/jit/Ion.cpp
@@ +216,5 @@
>      AutoLockForExclusiveAccess lock(cx);
>      AutoCompartment ac(cx, cx->atomsCompartment());
>  
>      IonContext ictx(cx, NULL);
> +    AutoFlushCache afc("IonRuntime::initialize", cx->runtime()->ionRuntime());

Why not using "this". I think this should be the same, but more visible?
Attachment #800106 - Flags: review?(hv1989) → review+
Assignee

Comment 4

6 years ago
(In reply to Hannes Verschore [:h4writer] from comment #2)
> Why not using "this". I think this should be the same, but more visible?

Oops done, thanks!
https://hg.mozilla.org/mozilla-central/rev/992102e6d2d2
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.