1131403 - Optimize uses of ObjectOrNull values better

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: WIP

Currently whenever we load an object-or-null property of an unboxed object that has actually seen null values before, we box the result into a value representation.  In many cases this can be improved, by just loading the property directly and using MIRType_ObjectOrNull.  While it would be hard to change the entire compiler to handle this type, we can still do spot fixes for common uses of ObjectOrNull values that are easy to handle: ToObjectOrNull (identity), Unbox, IsNullOrLikeUndefinedAndBranch, StoreFixedSlot and StoreSlot.

The attached WIP patch makes this change, for the above ops.  These are (not coincidentally) the ops which we end up using LoadUnboxedObjectOrNull instructions with on octane-richards.  Avoiding boxes in our compilation of these cases improves my score (x86 10.9) on this benchmark by several thousand points, fixing the regression with unboxed objects on this benchmark and improving on our base performance by a couple thousand points.

Incidentally, even with this patch there are still quite a lot of IsNullOrLikeUndefinedAndBranch instructions that take boxed values.  These are coming from LoadFixedSlot/LoadSlot instructions where we don't use an unboxed representation for the object (richards has some classes it only allocates one instance of, which we decide not to unbox.)  So after this gets in there will be further opportunities for improvement.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Would it be useful to update jitinfo for DOM getters/methods that know they return object-or-null (a fairly common case, actually) to claim MIRType_ObjectOrNull?  We'd still need to unbox to that type and do some type barriering, of course...

Comment 2

[Brian Hackett [Laid off!]]

(In reply to Boris Zbarsky [:bz] from comment #1)
> Would it be useful to update jitinfo for DOM getters/methods that know they
> return object-or-null (a fairly common case, actually) to claim
> MIRType_ObjectOrNull?  We'd still need to unbox to that type and do some
> type barriering, of course...

Yes, having that information would be good.  The way this patch works is that we initially generate a LoadUnboxedObjectOrNull that produces a boxed value.  Right near the end of the MIR optimization passes, we scan for these instructions and look at their uses.  If those uses are all able to deal with MIRType_ObjectOrNull then we specialize the original LoadUnboxedObjectOrNull to produce a value of that type.  This approach should be easy to extend to both new sinks (new instructions which can consume MIRType_ObjectOrNull) and new sources (LoadSlot, DOM calls).

Comment 3

[Brian Hackett [Laid off!]]

Attachment: patch

Complete patch.  After this gets in I'll look into doing this for LoadSlot/LoadFixedSlot.  I'd rather not do everything together because a) that's more complicated, and b) LoadSlot will need some tuning I think to avoid regressions.  While this optimization always improves things in the LoadUnboxedObjectOrNull case, if there is a LoadSlot followed by a StoreSlot and no other uses then we are probably better off leaving things boxed rather than doing this optimization (at least on x64; on other platforms the optimization might still reduce register pressure).

Comment 4

[Boris Zbarsky [:bzbarsky]]

In CodeGenerator::visitIsNullOrLikeUndefinedAndBranchT you want to make compareType a DebugOnly.  Otherwise it hits a fatal warning about unused variable in opt builds...

Comment 5

[Boris Zbarsky [:bzbarsky]]

So a question.  If I have a Value that is known to be object-or-null, trying to unbox to MIRType_ObjectOrNull fails right now (because MUnbox::New crashes on this MIRType).

Should I not be trying to use this stuff so far for the case of boxed-object-or-null?

Comment 6

[Brian Hackett [Laid off!]]

(In reply to Boris Zbarsky [:bz] from comment #5)
> So a question.  If I have a Value that is known to be object-or-null, trying
> to unbox to MIRType_ObjectOrNull fails right now (because MUnbox::New
> crashes on this MIRType).
> 
> Should I not be trying to use this stuff so far for the case of
> boxed-object-or-null?

MIRType_ObjectOrNull is only handled correctly in a few places in the compiler.  So you should continue generating boxed values in IonBuilder, and once this patch is in we'll be able to start specializing those generating instructions to produce ObjectOrNull pointers instead, provided that all their uses can handle MIRType_ObjectOrNull.

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8562056 [details] [diff] [review]
patch

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

::: js/src/jit/CodeGenerator.cpp
@@ +5457,5 @@
> +        masm.bind(&done);
> +    } else {
> +        Label isNull, done;
> +
> +        masm.branchTestPtr(Assembler::Zero, objreg, objreg, &isNull);

Can you MOZ_ASSERT(lhsType == MIRType_ObjectOrNull); here? The if-condition doesn't make it obviously-true so it'd be good to double check.

@@ +5514,5 @@
> +        // Objects that emulate undefined are loosely equal to null/undefined.
> +        Register scratch = ToRegister(lir->temp());
> +        testObjectEmulatesUndefined(input, ifTrueLabel, ifFalseLabel, scratch, ool);
> +    } else {
> +        testZeroEmitBranch(Assembler::Equal, input, ifTrue, ifFalse);

Same here.

::: js/src/jit/LIR-Common.h
@@ +2497,5 @@
>          return getOperand(1);
>      }
>  };
>  
> +// Takes a value and tests whether it is null or is an object that emulates

is null or undefined

::: js/src/jit/MIR.h
@@ +613,5 @@
>      }
>      bool emptyResultTypeSet() const;
>  
>      bool mightBeType(MIRType type) const {
> +        MOZ_ASSERT(type != MIRType_Value && type != MIRType_ObjectOrNull);

Nit: use separate asserts to make it easier to debug this when it fails.

::: js/src/jit/shared/CodeGenerator-shared.cpp
@@ +384,5 @@
>        case MIRType_Float32:
>        {
>          LAllocation *payload = snapshot->payloadOfSlot(*allocIndex);
> +        JSValueType valueType =
> +            (type == MIRType_ObjectOrNull) ? JSVAL_TYPE_OBJECT : ValueTypeFromMIRType(type);

Doesn't this mean we'll get a Value with object type tag and null payload?

::: js/src/jit/shared/CodeGenerator-x86-shared.h
@@ +149,5 @@
>  
> +    void testZeroEmitBranch(Assembler::Condition cond, Register reg,
> +                            MBasicBlock *ifTrue, MBasicBlock *ifFalse)
> +    {
> +        masm.cmpPtr(reg, ImmWord(0));

Nit: MOZ_ASSERT(cond == Assembler::Equal || cond == Assembler::NotEqual); and on ARM.

Comment 8

[Brian Hackett [Laid off!]]

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

Comment 9

[Brian Hackett [Laid off!]]

Attachment: normal slots patch

Extend this optimization to LoadSlot and LoadFixedSlot instructions.  This still doesn't eliminate all possible boxed value loads on octane-richards, as there are some other cases that can be optimized (e.g. TypeBarrier), but this patch is small and provides a nice speedup to octane-richards.  Approximate scores on my machine:

x86 old: 29500
x86 new: 33500
x86 new --unboxed-objects: 33500

x64 old: 26600
x64 new: 27800
x64 new --unboxed-objects: 30500

Comment 10

[Brian Hackett [Laid off!]]

Attachment: remainder

The type barrier instructions were only being used for reads that only produced null (since we can't do an unbox to null) and had no uses.  This patch just handles that case since the general case is more complicated (need to consider how the MTypeBarrier is used).  This patch also handles MTest uses for ObjectOrNull instructions, since it's kind of weird for 'if (x != null)' to be optimized more than 'if (x)'

Comment 11

[Carsten Book [:Tomcat]]

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

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 8564720 [details] [diff] [review]
remainder

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

::: js/src/jit/CodeGenerator.cpp
@@ +2567,5 @@
> +    Label miss, ok;
> +
> +    if (lir->mir()->type() == MIRType_ObjectOrNull) {
> +        Label *nullTarget = lir->mir()->resultTypeSet()->mightBeMIRType(MIRType_Null) ? &ok : &miss;
> +        masm.branchPtr(Assembler::Equal, obj, ImmWord(0), nullTarget);

branchTestPtr(Assembler::Zero, obj, obj, nullTarget); might be shorter/more efficient (assuming the macro assembler doesn't special case this).

Comment 13

[Brian Hackett [Laid off!]]

Parts 2 and 3:

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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