Closed Bug 1353359 Opened 7 years ago Closed 7 years ago

Port BindName IC to CacheIR


(Core :: JavaScript Engine: JIT, enhancement, P2)




Performance Impact low
Tracking Status
firefox55 --- fixed


(Reporter: jandem, Assigned: jandem)


(Blocks 1 open bug)



(4 files)

After bug 1326437 Ion's BindName IC will be the only IonCaches IC left.

BindName is not very interesting (BindNameIC::update doesn't even get called on octane/sunspider/kraken/assorted because it requires things like eval or weird scope chains), but porting it will let us remove a bunch of IonCaches code.
(In reply to Jan de Mooij [:jandem] from comment #0)
> things like eval or weird scope chains

Code like this often runs in Baseline, so having Baseline IC support for BindName would be a nice side-effect.
Whiteboard: [qf]
Whiteboard: [qf] → [qf:p3]
These patches don't use the spewer, I want to add that at the same time as GetName (bug 1353353) because these ICs are so similar.
Assignee: nobody → jdemooij
Attachment #8857936 - Flags: review?(evilpies)
This is based on the GetName code and the current Ion code, although the Ion code has been broken (stub is never attached) since the global lexical was added.

Despite what I thought in comment 0, this case is actually pretty hot for Baseline and this patch improves sunspider partial-sums in particular (because it assigns to a number of globals).
Attachment #8857937 - Flags: review?(evilpies)
Again similar to the GetName code, except that we have to return the environment object and we have to stop at unqualified var objects.
Attachment #8857938 - Flags: review?(evilpies)
Ports the Ion BindName IC and removes IonCache.

Note that we no longer need to purge caches on GC when we're preserving JIT code. This might improve performance a bit in some cases.

 18 files changed, 165 insertions(+), 1169 deletions(-)
Attachment #8857942 - Flags: review?(evilpies)
Comment on attachment 8857936 [details] [diff] [review]
Part 1 - Add BindNameIRGenerator

Review of attachment 8857936 [details] [diff] [review]:

Attachment #8857936 - Flags: review?(evilpies) → review+
Attachment #8857937 - Flags: review?(evilpies) → review+
Comment on attachment 8857938 [details] [diff] [review]
Part 3 - Add environment stub

Review of attachment 8857938 [details] [diff] [review]:

Looks sensible.
Attachment #8857938 - Flags: review?(evilpies) → review+
Comment on attachment 8857942 [details] [diff] [review]
Part 4 - Port Ion IC, remove IonCache

Review of attachment 8857942 [details] [diff] [review]:

YEAH \o/ Great! It's finally done.

::: js/src/jit/IonCaches.h
@@ +31,5 @@
>                                                 PropertyResult prop);
>  bool IsCacheableGetPropCallScripted(JSObject* obj, JSObject* holder, Shape* shape,
>                                      bool* isTemporarilyUnoptimizable = nullptr);
>  bool IsCacheableGetPropCallNative(JSObject* obj, JSObject* holder, Shape* shape);

Should move this to CacheIR. But we have to do that for some SharedIC functions as well, followup?
Attachment #8857942 - Flags: review?(evilpies) → review+
Pushed by
part 1 - Add BindNameIRGenerator and use it in Baseline. r=evilpie
part 2 - Add BindName stub for global (BINDGNAME) lookups. r=evilpie
part 3 - Add BindName stub for non-global (BINDNAME) lookups. r=evilpie
part 4 - Use BindName IC in Ion and remove the old IonCache infrastructure. r=evilpie
(In reply to Tom Schuster [:evilpie] from comment #8)
> Should move this to CacheIR. But we have to do that for some SharedIC
> functions as well, followup?

I filed bug 1340153 a while ago. We should rm IonCaches.cpp/h, just need to move the few remaining functions elsewhere. I'll do that next week.
These patches improved sunspider-partial-sums by 22% and misc-bugs-636096-model2d by 26%, according to AWFY.
This might have regressed six-speed-map-string-es6.
(In reply to Tom Schuster [:evilpie] from comment #12)
> This might have regressed six-speed-map-string-es6.

I don't think it uses BindName anywhere and it only affected 64-bit. Probably some kind of compiler/CPU quirk or alignment issue somewhere, these micro-benchmarks are pretty sensitive to that.
Performance Impact: --- → P3
Whiteboard: [qf:p3]
You need to log in before you can comment on or make changes to this bug.