Port BindName IC to CacheIR

RESOLVED FIXED in Firefox 55

Status

()

P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p3])

Attachments

(4 attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
(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]
(Assignee)

Comment 2

2 years ago
Created attachment 8857936 [details] [diff] [review]
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.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8857936 - Flags: review?(evilpies)
(Assignee)

Comment 3

2 years ago
Created attachment 8857937 [details] [diff] [review]
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).
Attachment #8857937 - Flags: review?(evilpies)
(Assignee)

Comment 4

2 years ago
Created attachment 8857938 [details] [diff] [review]
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.
Attachment #8857938 - Flags: review?(evilpies)
(Assignee)

Comment 5

2 years ago
Created attachment 8857942 [details] [diff] [review]
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(-)
Attachment #8857942 - Flags: review?(evilpies)
Comment on attachment 8857936 [details] [diff] [review]
Part 1 - Add BindNameIRGenerator

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

Great.
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+

Comment 9

2 years ago
Pushed by jandemooij@gmail.com:
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
(Assignee)

Comment 10

2 years ago
(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

2 years ago
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.
(Assignee)

Comment 13

2 years ago
(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.
Depends on: 1356822
You need to log in before you can comment on or make changes to this bug.