1353359 - Port BindName IC to CacheIR

Status: RESOLVED FIXED

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

Description

[Jan de Mooij [:jandem]]

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.

Comment 1

[Jan de Mooij [:jandem]]

(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.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Add BindNameIRGenerator

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.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Add global stub

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).

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Add environment stub

Again similar to the GetName code, except that we have to return the environment object and we have to stop at unqualified var objects.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Port Ion IC, remove IonCache

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(-)

Comment 6

[Tom S]

Comment on attachment 8857936 [details] [diff] [review]
Part 1 - Add BindNameIRGenerator

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

Great.

Comment 7

[Tom S]

Comment on attachment 8857938 [details] [diff] [review]
Part 3 - Add environment stub

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

Looks sensible.

Comment 8

[Tom S]

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?

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a5e16d18cdb
part 1 - Add BindNameIRGenerator and use it in Baseline. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0777fa8a631
part 2 - Add BindName stub for global (BINDGNAME) lookups. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/a74790bf48bd
part 3 - Add BindName stub for non-global (BINDNAME) lookups. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/c57012db4d11
part 4 - Use BindName IC in Ion and remove the old IonCache infrastructure. r=evilpie

Comment 10

[Jan de Mooij [:jandem]]

(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.

Comment 11

[Guilherme Lima]

These patches improved sunspider-partial-sums by 22% and misc-bugs-636096-model2d by 26%, according to AWFY.

Comment 12

[Tom S]

This might have regressed six-speed-map-string-es6.

Comment 13

[Jan de Mooij [:jandem]]

(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.

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/5a5e16d18cdb
https://hg.mozilla.org/mozilla-central/rev/e0777fa8a631
https://hg.mozilla.org/mozilla-central/rev/a74790bf48bd
https://hg.mozilla.org/mozilla-central/rev/c57012db4d11