1209118 - Unify Ion GETPROP and GETELEM IC stubs

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

Baseline/Ion can generate GETPROP and SETPROP stubs for the most common cases, but GETELEM and SETELEM coverage is way worse. We do cache some *ELEM cases and there's some ad-hoc code sharing (and duplication..) to emit GETPROP and GETELEM stubs for native object properties, but only for a few cases.

For instance: Ion's GetPropertyCache can emit a Proxy stub. Ion's GetElementCache doesn't do anything to handle such proxies.

Bad *ELEM caching shows up in the wild (eg on Treeherder, bug 1166184) and also affects symbol properties: we'll always use *ELEM ops for those.

Two ways to fix this:

(1) Move most of the GetPropertyIC logic into a new helper class ("GetPropStubGenerator" or something) that's used by both GetPropertyIC and GetElementIC. For GetElementIC, each stub will emit an extra |key == cachedKey| check and then do the usual GetProperty stuff.

(2) Kill GetPropertyIC and always use GetElementIC (we could rename it of course). To avoid performance regressions, we'd have to make sure we don't emit unnecessary |key == cachedKey| guards if the index is a constant string, but that seems doable.

Personally I really like (2). For Ion (1) would work, but requires some verbosity passing stuff to the new class. For Baseline (2) seems a lot easier than (1). Doing (2) also means obj[constant-string-or-symbol] will get exactly the same treatment as a GETPROP, which seems pretty nice for symbols in particular.

Comment 1

[Kannan Vijayan [:djvj]]

I agree with the general sentiment.  The only downside I see is that unifying by making everything go through a GetElem path means that all GetProp stubs (which will now be the same as GetElem stubs) will do an additional name-check, instead of the name being implicit.

I suppose the overhead of IC in the first place (branch => code cache invalidation => target object guards => branch back to mainline code) would drown that out, so it's not a really big issue.  We might even be able to avoid the name guard by having a |bool constantName| parameter to the generator code, which indicates that the name is always going to be the same every time the stub is entered (in baseline, this can be stored as a bit in the |extra| bits on the stub type word), and can be used to omit the name-guards.

The approach is good.

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Kannan Vijayan [:djvj] from comment #1)
> We might even be able to
> avoid the name guard by having a |bool constantName| parameter to the
> generator code, which indicates that the name is always going to be the same
> every time the stub is entered (in baseline, this can be stored as a bit in
> the |extra| bits on the stub type word), and can be used to omit the
> name-guards.

Yep, that's what I was thinking as well (the last sentence of (2)). We could modify Ion's GetElementIC to store the index_ as a ConstantOrRegister, so the IC can easily check if it's a constant and that means there's also no regalloc overhead...

I'll try to prototype this tomorrow and see how it turns out.

Comment 3

[Brian Hackett [Laid off!]]

(2) sounds good to me.

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> I'll try to prototype this tomorrow and see how it turns out.

I've a stack of patches to use Ion's GetPropertyIC for GETELEM and it's really nice. For instance, after that it was almost trivial to replace PropertyName* with jsid in all the GetPropertyIC stubs to make them work with symbols. Will upload patches after cleaning them up.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Combined patch

Combined patch.

 17 files changed, 550 insertions(+), 763 deletions(-)

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Add id operand to MGetPropertyCache

Adds an operand to MGetPropertyCache for the property id, right now it's just MConstant(StringValue(name)).

Pretty straight-forward for the most part, but I had to add useBoxOrTypedOrConstant to avoid ending up with 4 LGetPropertyCache* LIR instructions for each combination of {Typed, Value} input and output.

This worked very well and I think we should consider using useBoxOrTypedOrConstant for other LIR instructions that have separate typed and Value versions atm.

(MGetElementCache "cheats" here: if the output is a Value, the id is also boxed and vice versa, so it only needs two LIR instructions, but with useBoxOrTypedOrConstant we can do better than that.)

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Fix GetPropertyIC stubs

This patch changes all GetPropertyIC stubs to work with a Value id. This required:

* Using jsid instead of PropertyName*, so the stubs work with symbols.

* Emitting a guard if the id is not a constant. The code for the string case I mostly stole from GetElementIC. GetElementIC will be removed later so I didn't bother factoring it out.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Move arguments element stub

This moves the arguments element stub from GetElementIC to GetPropertyIC.

I also converted it to the tryAttach* style we use for other GetPropertyIC stubs, it's more readable.

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Move dense element stub

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Part 5 - Move dense element hole stub

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Part 6 - Move typed-or-unboxed element stub

I also had to move allowDoubleResult from MGetElementCache to MGetPropertyCache.

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Part 7 - Allow disabling GetPropertyIC

GetElementIC can disable itself (remove all stubs and give up on attaching a new one) after it fails to attach a stub or it has too many stubs.

This patch adds that mechanism to GetPropertyIC as well.

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Part 8 - Use MGetPropertyCache for GETELEM

Comment 14

[Jan de Mooij [:jandem]]

Attachment: Part 9 - rm GetElement IC

11 files changed, 0 insertions(+), 445 deletions(-)

Comment 15

[Jan de Mooij [:jandem]]

Feel free to just review the combined patch, it might be faster in this case.

And FWIW, the micro-benchmark below demonstrates symbols properties are now optimized just like string properties:

before: 603 ms
after:   13 ms

var sym = Symbol("foo");
var o = {[sym]: 3};

function f() {
    var res = 0;
    for (var i=0; i<10000000; i++) {
	res += o[sym];
    }
    return res;
}
var t = new Date;
f();
print(new Date - t);

Comment 16

[Jan de Mooij [:jandem]]

FWIW after these patches land, I want to do some more Ion GetPropertyIC cleanup, but I'll do that in a separate bug.

Comment 17

[Jan de Mooij [:jandem]]

Eric, any chance you can get to those patches soonish? If not I can try to find another reviewer. The patches are pretty bitrot-prone and it's also blocking my other IC work (I'm waiting until this is in).

Comment 18

[Eric Faust [:efaust]]

Comment on attachment 8668297 [details] [diff] [review]
Part 1 - Add id operand to MGetPropertyCache

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

I believe so. r=me with questions below answered.

::: js/src/jit/CodeGenerator.cpp
@@ +8473,5 @@
>      addCache(ins, allocateCache(cache));
>  }
>  
> +ConstantOrRegister
> +CodeGenerator::toConstantOrRegister(LInstruction* lir, size_t n, MIRType type)

I was going to complain about this being called 'n', but since it means different things for different types, it makes sense. No change required.

::: js/src/jit/IonCaches.cpp
@@ +1934,5 @@
> +    uint32_t dummy;
> +    if (!idval.isString() || !idval.toString()->isAtom() || idval.toString()->asAtom().isIndex(&dummy))
> +        return true;
> +
> +    if (!id().constant())

I think there are several places in this patch that attempt to defend against things other than direct getprops flowing into the IC. I *think* this line is one of them? How are id() and idval different, here?

@@ +2011,5 @@
>      }
>  
> +    jsbytecode* pc = cache.idempotent() ? nullptr : cache.pc();
> +
> +    if (!pc || *pc == JSOP_GETPROP || *pc == JSOP_CALLPROP || *pc == JSOP_LENGTH) {

Is this good with the type system? Is the jsbytecode -> JSOp conversion OK as implicit without warning?

Further, I don't see why this is necessary yet. It will certainly be useful later in this stack, but it should be OK without it, as we are only seeing PROP operations at the moment, right?

Comment 19

[Eric Faust [:efaust]]

Comment on attachment 8668298 [details] [diff] [review]
Part 2 - Fix GetPropertyIC stubs

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

This is quite nice. I'm excited to see this all coming together.

::: js/src/jit/IonCaches.cpp
@@ +1349,5 @@
> +
> +    if (this->id().constant())
> +        return;
> +
> +    TypedOrValueRegister reg = this->id().reg();

can we call this idReg instead? There's a lot flying around in there.

@@ -1934,5 @@
> -    uint32_t dummy;
> -    if (!idval.isString() || !idval.toString()->isAtom() || idval.toString()->asAtom().isIndex(&dummy))
> -        return true;
> -
> -    if (!id().constant())

OK, this makes more sense now in light of the Id checks, but I still think it was superfluous, as we couldn't actually see any non-constant ops yet.

Comment 20

[Eric Faust [:efaust]]

Comment on attachment 8668299 [details] [diff] [review]
Part 3 - Move arguments element stub

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

LGTM

Comment 21

[Eric Faust [:efaust]]

Comment on attachment 8668302 [details] [diff] [review]
Part 4 - Move dense element stub

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

Straightforward.

Comment 22

[Eric Faust [:efaust]]

Comment on attachment 8668303 [details] [diff] [review]
Part 5 - Move dense element hole stub

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

Yep

Comment 23

[Eric Faust [:efaust]]

Comment on attachment 8668306 [details] [diff] [review]
Part 7 - Allow disabling GetPropertyIC

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

Sure.

Comment 24

[Eric Faust [:efaust]]

Comment on attachment 8668307 [details] [diff] [review]
Part 8 - Use MGetPropertyCache for GETELEM

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

PULL THE SWITCH!

Comment 25

[Eric Faust [:efaust]]

Comment on attachment 8668309 [details] [diff] [review]
Part 9 - rm GetElement IC

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

Bet this one felt good. Good riddance. :)

Comment 26

[Jan de Mooij [:jandem]]

(In reply to Eric Faust [:efaust] from comment #18)
> > +    if (!id().constant())
> 
> I think there are several places in this patch that attempt to defend
> against things other than direct getprops flowing into the IC. I *think*
> this line is one of them? How are id() and idval different, here?

id() is the register-or-constant holding the id, and idval is the actual HandleValue. These checks aren't really needed at this point though, as your other comment pointed out.

> Is this good with the type system? Is the jsbytecode -> JSOp conversion OK
> as implicit without warning?

Yup, we do this in several places and it's a bit less verbose than JSOp(*pc) == JSOP_FOO.

> Further, I don't see why this is necessary yet. It will certainly be useful
> later in this stack, but it should be OK without it, as we are only seeing
> PROP operations at the moment, right?

Right, I moved some code out of this patch and could/should have moved this too :)

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7970f1b47660
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d44da0e94ec
https://hg.mozilla.org/integration/mozilla-inbound/rev/05db9e0bf5fc
https://hg.mozilla.org/integration/mozilla-inbound/rev/4690d7ba64a1
https://hg.mozilla.org/integration/mozilla-inbound/rev/174df1312e14
https://hg.mozilla.org/integration/mozilla-inbound/rev/05e4d4f27f9e
https://hg.mozilla.org/integration/mozilla-inbound/rev/c97ff0e6aaa7
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b5f5e6b45c0
https://hg.mozilla.org/integration/mozilla-inbound/rev/c17c1dae6ea6

Comment 28

[Jan de Mooij [:jandem]]

This seems to have regressed Octane-TypeScript by ~11%. It looks like it's caused by the disable-cache mechanism I ported from GetElementIC to GetPropertyIC, I'll look into this.

Other than that, things are fine I think. Some small wins on the assorted benchmarks like lodash1.

Let's see what Dromaeo does..

Comment 29

[Jan de Mooij [:jandem]]

Attachment: Fix TypeScript regression

Don't disable the cache if we have a constant id (GETPROP) and attached the maximum number of stubs.

Pretty lame but it restores the old behavior in this case and fixes the Octane-TypeScript regression. The right long-term fix is to handle megamorphism better.

Comment 30

[Jan de Mooij [:jandem]]

Attachment: One-line fix

I noticed we didn't increment failedUpdates_ anywhere. The patch also simplifies the condition.

Posting this as separate patch because these changes easily regress perf, so I want to land this later.

Comment 31

[Eric Faust [:efaust]]

Comment on attachment 8672661 [details] [diff] [review]
Fix TypeScript regression

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

This is a little nose-wrinkly, but it makes sense. File the followup before landing.

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7970f1b47660
https://hg.mozilla.org/mozilla-central/rev/4d44da0e94ec
https://hg.mozilla.org/mozilla-central/rev/05db9e0bf5fc
https://hg.mozilla.org/mozilla-central/rev/4690d7ba64a1
https://hg.mozilla.org/mozilla-central/rev/174df1312e14
https://hg.mozilla.org/mozilla-central/rev/05e4d4f27f9e
https://hg.mozilla.org/mozilla-central/rev/c97ff0e6aaa7
https://hg.mozilla.org/mozilla-central/rev/9b5f5e6b45c0
https://hg.mozilla.org/mozilla-central/rev/c17c1dae6ea6

Comment 33

[Sean Stangl [:sstangl]]

This was a 17.2% regression on octane-typescript:

http://arewefastyet.com/#machine=28&view=breakdown&suite=octane

Comment 34

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5d213e47dd6

Comment 35

[Jan de Mooij [:jandem]]

(In reply to Sean Stangl [:sstangl] from comment #33)
> This was a 17.2% regression on octane-typescript:
> 
> http://arewefastyet.com/#machine=28&view=breakdown&suite=octane

See previous comments, the patch I just pushed should fix this. Fingers crossed...

(In reply to Eric Faust [:efaust] from comment #31)
> This is a little nose-wrinkly, but it makes sense. File the followup before
> landing.

I updated the comment to mention bug 1107515, I really want to fix that bug after the other IC changes...

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b5d213e47dd6

Comment 37

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e1d668ff156d

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e1d668ff156d