Closed Bug 1335405 Opened 3 years ago Closed 3 years ago

Use SetPropIRGenerator for Baseline SetElem stubs

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Baseline currently only has SetElem stubs for dense elements and typed array elements. It's pretty easy now to use the SetPropIRGenerator so we can also cache SetElem with string/symbol properties.

Below is a simple testcase where we use two symbol properties (a setter and a plain data property). I get the following results:

--no-ion before: 3560 ms
--no-ion after:   224 ms
<no flags>:       104 ms

So with these patches Baseline will be 15x faster than before, and only twice as slow as Ion. (To be fair, I think Ion would be much faster if we inlined the setter.) I get similar results when I use strings instead of symbols.

function f() {
    var sym1 = Symbol(), sym2 = Symbol();
    var o = {};
    Object.defineProperty(o, sym2, {set: function(i) {
        this[sym1] = i;
    }});
    for (var i=0; i<10000000; i++)
        o[sym2] = i;
}
var t = new Date;
f();
print(new Date - t);
Baseline also uses the SetElem IC for JSOP_INITELEM etc, and we shouldn't attach setter stubs for these ops.

This patch removes JOF_SET (it was unused) and replaces it with JOF_PROPINIT (for the various INITPROP, INITELEM ops) and JOF_PROPSET (for SETPROP, SETELEM, SETNAME, etc).
Attachment #8832044 - Flags: review?(arai.unmht)
(In reply to Jan de Mooij [:jandem] from comment #1)
> Baseline also uses the SetElem IC for JSOP_INITELEM etc, and we shouldn't
> attach setter stubs for these ops.

I think at some point we should consider adding a separate InitPropIRGenerator, as the init/define ops are simpler and there are fewer cases to consider. It probably doesn't matter that much though and it would duplicate a few things.
This patch adds SetPropIRGenerator::maybeEmitIdGuard and calls it for the stubs we emit. I also added tests for this.

The SetElem is special in Baseline: we pass the object/index in R0/R1 and the RHS Value is stored on the Baseline stack. To support this I had to teach the CacheRegisterAllocator about Values stored on the Baseline stack so it can just load them from there.
Attachment #8832055 - Flags: review?(hv1989)
Comment on attachment 8832044 [details] [diff] [review]
Part 1 - Add JOF_PROPINIT and JOF_PROPSET

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

r+ with or without the following change.

::: js/src/jsopcode.h
@@ +61,5 @@
>  
>      JOF_NAME            = 1 << 5,   /* name operation */
>      JOF_PROP            = 2 << 5,   /* obj.prop operation */
>      JOF_ELEM            = 3 << 5,   /* obj[index] operation */
>      JOF_MODEMASK        = 7 << 5,   /* mask for above addressing modes */

totally unrelated tho, JOF_MODEMASK should be 3 << 5, and 1 << 7 is unused.

::: js/src/vm/Opcodes.h
@@ +823,5 @@
>       *   Type: Local Variables
>       *   Operands: uint32_t localno
>       *   Stack: v => v
>       */ \
> +    macro(JSOP_SETLOCAL,  87,"setlocal",    NULL,         4,  1,  1,  JOF_LOCAL|JOF_NAME|JOF_DETECTING) \

considering the fact that JSOP_INITLEXICAL has JOF_PROPINIT and JSOP_SETINTRINSIC has JOF_PROPSET,
I feel JSOP_SETLOCAL and maybe JSOP_SETARG should have JOF_PROPSET, for consistency.
or perhaps removing JOF_PROPINIT/JOF_PROPSET from JSOP_INITLEXICAL/JSOP_SETINTRINSIC ?

if these flags are only for JIT and IC, at least there's no reason to add JOF_PROPSET to JSOP_SETINTRINSIC that isn't supported in JIT.

(of course, those flags have no effect for them, as you said, so I'm also fine with current change
Attachment #8832044 - Flags: review?(arai.unmht) → review+
Comment on attachment 8832055 [details] [diff] [review]
Part 2 - Use SetPropIRGenerator for SetElem

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

That was conceptual easy patch given that this adds support for all those cases to JSOP_SETELEM. Nice. Sorry for the wait.

::: js/src/jit/CacheIR.cpp
@@ +1321,5 @@
>          MOZ_ASSERT(&idVal_.toString()->asAtom() == JSID_TO_ATOM(id));
>          return;
>      }
>  
> +    emitIdGuard(getElemKeyValueId(), id);

Shouldn't we keep the guard that this is CacheKind::GetElem?

@@ +1335,2 @@
>      }
> +

Maybe add a guard that this is CacheKind::SetElem?

@@ +1616,5 @@
>  {
>      AutoAssertNoPendingException aanpe(cx_);
>  
>      ValOperandId lhsValId(writer.setInputOperandId(0));
> +    ValOperandId rhsValId;

Can we rename this to:
ObjectValId
ValueValId

@@ +1624,5 @@
> +        MOZ_ASSERT(cacheKind_ == CacheKind::SetElem);
> +        MOZ_ASSERT(setElemKeyValueId().id() == 1);
> +        writer.setInputOperandId(1);
> +        rhsValId = ValOperandId(writer.setInputOperandId(2));
> +    }

This part is annoying/confusing to read. Since it is not immediately visible why setInputOperand(1) and setInputOperand(2) are treated different. 

Can we use a Maybe<ValOperandId> here for the key and pass that to maybeEmitIdGuard() ?

@@ +1939,5 @@
> +        MOZ_ASSERT(cacheKind_ == CacheKind::SetElem);
> +        MOZ_ASSERT(setElemKeyValueId().id() == 1);
> +        writer.setInputOperandId(1);
> +        rhsValId = ValOperandId(writer.setInputOperandId(2));
> +    }

Ditto
Attachment #8832055 - Flags: review?(hv1989) → review+
(In reply to Tooru Fujisawa [:arai] from comment #4)
> totally unrelated tho, JOF_MODEMASK should be 3 << 5, and 1 << 7 is unused.

Ah yes. I changed this and now we can use 1 << 7 and 1 << 8. That way we don't have to take the (unused) 1 << 9 flag :)

> or perhaps removing JOF_PROPINIT/JOF_PROPSET from
> JSOP_INITLEXICAL/JSOP_SETINTRINSIC ?

Yeah, good point. I removed the flag from INITLEXICAL and SETINTRINSIC.
(In reply to Hannes Verschore [:h4writer] from comment #5)
> > +    emitIdGuard(getElemKeyValueId(), id);
> 
> Shouldn't we keep the guard that this is CacheKind::GetElem?

I removed it because getElemKeyValueId also asserts this, but I added it back in case it makes the code more readable.

> >      ValOperandId lhsValId(writer.setInputOperandId(0));
> > +    ValOperandId rhsValId;
> 
> Can we rename this to:
> ObjectValId
> ValueValId

I changed it to objValId and rhsValId. objValId because it's more consistent with all the other code that uses objId. I think rhsValId is a little clearer than valueValId because it's clear which value it is and we use rhsId/rhsValId in other places.

> Can we use a Maybe<ValOperandId> here for the key and pass that to
> maybeEmitIdGuard() ?

The key is "implicit", similar to the GetElem IC. I agree it's probably nicer to pass it to the tryAttach* methods as Maybe<ValOperandId> even though it's a bit more verbose. We should change GetPropIRGenerator at the same time. I'll file a follow-up bug today.
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8651f379530a
part 1 - Replace unused JOF_SET with JOF_PROPINIT and JOF_PROPSET. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e019f957600
part 2 - Use SetPropIRGenerator for Baseline SETELEM stubs. r=h4writer
https://hg.mozilla.org/mozilla-central/rev/8651f379530a
https://hg.mozilla.org/mozilla-central/rev/1e019f957600
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Flags: needinfo?(jdemooij)
According to AWFY there's a regression on 32-bit on typedobj-splay-typedobj. I can reproduce it locally, but not if I run just this test. A gc() call at the start of the file "fixes" it too. We're probably attaching more stubs now and triggering GC in different places...
Flags: needinfo?(jdemooij)
You need to log in before you can comment on or make changes to this bug.