Closed Bug 1520759 Opened 2 years ago Closed 2 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox66 --- wontfix
firefox67 --- fixed

People

(Reporter: anba, Assigned: anba)

Details

(Keywords: perf)

Attachments

(1 file, 2 obsolete files)

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.

Attached patch bug1520759.patch (obsolete) — Splinter Review

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.
Attachment #9037192 - Flags: feedback?(jdemooij)
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.
Attachment #9037192 - Flags: feedback?(jdemooij) → feedback+

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

Attached patch bug1520759.patch (obsolete) — Splinter Review

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

Assignee: nobody → andrebargull
Attachment #9037192 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #9038554 - Flags: review?(jdemooij)

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

Keywords: perf
Priority: -- → P2

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

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

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.
Attachment #9038554 - Flags: review?(jdemooij) → review+

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

Ah nevermind then :)

Or maybe we could assert or use IsElemPC...

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.

Attached patch bug1520759.patchSplinter Review

Updated per review comments, carrying r+.

Attachment #9038554 - Attachment is obsolete: true
Attachment #9040742 - Flags: review+

Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fe2d9456e6b
Inline getters for jsop_getelem operations with constant property keys. r=jandem

Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.