1520759 - Inline getters accessed through jsop_getelem when the property key is a constant

Status: RESOLVED FIXED

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

Description

[André Bargull [:anba]]

The idea is to call IonBuilder::getPropTryCommonGetter in IonBuilder::getElemTryGetProp, so when the property key is a constant, we can try to inline the getter similar to how getters are already inlined when accessed through jsop_getprop.

This ensures the performance in this test case is the same for jsop_getprop and jsop_getelem accesses.

var name = "prop";

class Obj {
    constructor(v) {
        this._prop = v;
    }
    get prop() {
        return this._prop;
    }
}

function f() {
    var holder = new Obj(1);
    var r = 0
    var t = dateNow()
    for (var i = 0; i < 5000000; ++i) {
        // r += holder.prop;
        r += holder[name];
    }
    return [dateNow() - t, r];
}

for (var i = 0; i < 10; ++i) print(f());

Improving the exact test case from above is probably not important for real-world benchmarks resp. websites, because why would anyone use a jsop_getelem access here for a string-valued property? But when the property is a symbol, jsop_getelem accesses are required, so improving this case suddenly no longer looks irrelevant. And one case where symbol-valued property accesses to a getter are used, is the SpeciesConstructor function.

Comment 1

[André Bargull [:anba]]

Attachment: bug1520759.patch

Requesting feedback instead of review for now, because I don't know if the BaselineInspector changes are correct to make. Like, is it at all okay/sound to check for specific jsid baked into IC stubs? (Note: I've never worked with BaselineInspector, so I don't know at all what works there and what not!)

IonBuilder changes:

• Main change: Add a call to getPropTryCommonGetter in getElemTryGetProp.
• And then change a bunch of methods to use jsid instead of PropertyName*, so we can pass through constant Symbol property keys.

Ion.h changes:

• Adjust IsIonInlinablePC now that getters in JSOP_[GET,CALL]ELEM can be inlined.

BaselineInspector changes:

• If the current pc points to a JSOP_[GET,CALL]ELEM operation, also check the IC stubs for this specific jsid.
• The id-guard sequence is defined in IRGenerator::emitIdGuard.

Drive-by change:

• Remove unused default parameter value in freezePropertiesForCommonPrototype.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 9037192 [details] [diff] [review]
bug1520759.patch

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

Looks good! One thing to be aware of is bailouts from inlined getters/setters, see:

https://searchfox.org/mozilla-central/rev/78e8de968efd77ef3b30ea081bce07614cd6776b/js/src/jit/BaselineBailouts.cpp#1088
https://searchfox.org/mozilla-central/rev/78e8de968efd77ef3b30ea081bce07614cd6776b/js/src/jit/BaselineBailouts.cpp#446-447
https://searchfox.org/mozilla-central/rev/78e8de968efd77ef3b30ea081bce07614cd6776b/js/src/jit/BaselineIC.cpp#2954-2971

This is pretty tricky and an area we want to simplify (like the BaselineInspector!).

::: js/src/jit/IonBuilder.cpp
@@ +10147,5 @@
>    return pushTypeBarrier(ins, types, BarrierKind::TypeSet);
>  }
>  
>  NativeObject* IonBuilder::commonPrototypeWithGetterSetter(
> +    TemporaryTypeSet* types, jsid id, bool isGetter, JSFunction* getterOrSetter,

Nice, I really like when the core property lookup/optimization code works with any jsid instead of just PropertyName.

Comment 3

[André Bargull [:anba]]

(In reply to Jan de Mooij [:jandem] from comment #2)

Looks good! One thing to be aware of is bailouts from inlined
getters/setters, see:

Thanks for the heads-up, bailout handling was indeed completely missing!

Comment 4

[André Bargull [:anba]]

Attachment: bug1520759.patch

Updates when compared to comment #1:

• Added missing bailout handling based on the existing bailout code for JSOP_GETPROP. (Thank you!)
• Added a IsIonInlinableGetterOrSetterPC helper to avoid repeating IsGetPropPC(pc) || IsGetElemPC(pc) || IsSetPropPC(pc) in multiple places.

I'm not 100% sure the comment I've added to [1] is correct, because, unsurprisingly, I didn't quite grok everything in InitFromBailout. :-)

[1] https://searchfox.org/mozilla-central/rev/6c784c93cfbd5119ed07773a170b59fbce1377ea/js/src/jit/BaselineBailouts.cpp#1089-1096

Comment 5

[Nicolas B. Pierron [:nbp]]

(In reply to André Bargull [:anba] from comment #0)

Improving the exact test case from above is probably not important for real-world benchmarks resp. websites, because why would anyone use a jsop_getelem access here for a string-valued property?

This case actually happens quite frequently in template frameworks.

Comment 6

[Jan de Mooij [:jandem]]

André, sorry for the delay :/ I'll try to get to this tomorrow.

Comment 7

[André Bargull [:anba]]

No worries, I didn't plan to land this last week during the soft-freeze anyway. :-)

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 9038554 [details] [diff] [review]
bug1520759.patch

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

Looks great, thanks for doing this! Just some nits below.

::: js/src/jit/BaselineBailouts.cpp
@@ +1096,5 @@
>          // This means that the depth is actually one *more* than expected
>          // by the interpreter, as there is now a JSFunction, |this| and [arg],
> +        // rather than the expected |this| and [arg].
> +        // If the inlined accessor is a getelem operation, the numbers do match,
> +        // but that's just because getelem expects one more item on the stack.

Yeah I think this is right because we have:

getprop: obj
inlined getter: getter, this (= obj) (+1)

setprop: obj, val
inlined setter: setter, this (= obj), val (+1)

getelem: obj, key
inlined getter: getter, this (= obj) (+0)

::: js/src/jit/BaselineIC.cpp
@@ +496,4 @@
>      case Call_ScriptedFunCall:
>      case Call_ConstStringSplit:
>      case WarmUpCounter_Fallback:
>      // These two fallback stubs don't actually make non-tail calls,

Nit: s/two/three/

::: js/src/jit/BaselineInspector.cpp
@@ +1068,5 @@
>  
> +static bool GuardSpecificAtomOrSymbol(CacheIRReader& reader, ICStub* stub,
> +                                      const CacheIRStubInfo* stubInfo,
> +                                      ValOperandId keyId, jsid id) {
> +  if (JSID_IS_ATOM(id)) {

Add a brief comment, something like:

// Try to match an id guard emitted by IRGenerator::emitIdGuard.

@@ +1133,5 @@
>    //   GuardSpecificObject objId, <global>
> +  //
> +  // If we test for a specific jsid, [..Id Guard..] is implemented through:
> +  //   GuardIs(String|Symbol) keyId
> +  //   <GuardSpecific(Atom|Symbol) keyId>

Nit: maybe remove the <> here? It reads as an optional instruction but it's not :) Maybe something like:

GuardSpecific(Atom|Symbol) keyId, <atom|symbol>

@@ +1270,5 @@
>  
> +  // Only GetElem operations need to guard against a specific property id.
> +  if (!IsGetElemPC(pc)) {
> +    id = JSID_EMPTY;
> +  }

Can we add this:

} else {
    MOZ_ASSERT(IsGetPropPC(pc) || IsSetPropPC(pc));
}

Then if someone tries to inline setters at SETELEM they'll hit this.

@@ +1316,5 @@
>    // propShape has the getter/setter we're interested in.
> +  //
> +  // If we test for a specific jsid, [..Id Guard..] is implemented through:
> +  //   GuardIs(String|Symbol) keyId
> +  //   <GuardSpecific(Atom|Symbol) keyId>

Same as above.

@@ +1351,5 @@
>  
> +  // Only GetElem operations need to guard against a specific property id.
> +  if (!IsGetElemPC(pc)) {
> +    id = JSID_EMPTY;
> +  }

Same as above.

Comment 9

[André Bargull [:anba]]

(In reply to Jan de Mooij [:jandem] from comment #8)

Can we add this:

} else {
MOZ_ASSERT(IsGetPropPC(pc) || IsSetPropPC(pc));
}

Then if someone tries to inline setters at SETELEM they'll hit this.

Adding extra assertions for the correct JSOp will quickly end up with a rather large list of ops which need to be checked.

In BaselineInspector::commonGetPropFunction:

  MOZ_ASSERT(IsGetPropPC(pc) || IsGetElemPC(pc) || JSOp(*pc) == JSOP_GETGNAME);

BaselineInspector::commonSetPropFunction will get:

  MOZ_ASSERT(IsSetPropPC(pc) || JSOp(*pc) == JSOP_INITGLEXICAL ||
             JSOp(*pc) == JSOP_INITPROP || JSOp(*pc) == JSOP_INITLOCKEDPROP ||
             JSOp(*pc) == JSOP_INITHIDDENPROP);

And finally for BaselineInspector::megamorphicGetterSetterFunction:

  MOZ_ASSERT(IsGetPropPC(pc) || IsGetElemPC(pc) || IsSetPropPC(pc) ||
             JSOp(*pc) == JSOP_GETGNAME || JSOp(*pc) == JSOP_INITGLEXICAL ||
             JSOp(*pc) == JSOP_INITPROP || JSOp(*pc) == JSOP_INITLOCKEDPROP ||
             JSOp(*pc) == JSOP_INITHIDDENPROP);

Comment 10

[Jan de Mooij [:jandem]]

Ah nevermind then :)

Comment 11

[Jan de Mooij [:jandem]]

Or maybe we could assert or use IsElemPC...

Comment 12

[André Bargull [:anba]]

I think I'll go with the long list of assertions, better safe than sorry. And it also matches how we assert every possible JSOp in the fallback stubs.

Comment 13

[André Bargull [:anba]]

Attachment: bug1520759.patch

Updated per review comments, carrying r+.

Comment 14

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b45fd5eeea7c0d061944bbc0352de371b85846dc

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fe2d9456e6b
Inline getters for jsop_getelem operations with constant property keys. r=jandem

Comment 16

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/9fe2d9456e6b