Closed Bug 1131403 Opened 5 years ago Closed 5 years ago

Optimize uses of ObjectOrNull values better

Categories

(Core :: JavaScript Engine: JIT, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: bhackett, Assigned: bhackett)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

Attached patch WIP (obsolete) — Splinter Review
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.
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...
Flags: needinfo?(bhackett1024)
(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).
Flags: needinfo?(bhackett1024)
Attached patch patchSplinter Review
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).
Assignee: nobody → bhackett1024
Attachment #8561790 - Attachment is obsolete: true
Attachment #8562056 - Flags: review?(jdemooij)
In CodeGenerator::visitIsNullOrLikeUndefinedAndBranchT you want to make compareType a DebugOnly.  Otherwise it hits a fatal warning about unused variable in opt builds...
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?
(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 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.
Attachment #8562056 - Flags: review?(jdemooij) → review+
Keywords: leave-open
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
Attachment #8564628 - Flags: review?(jdemooij)
Attached patch remainderSplinter Review
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)'
Attachment #8564720 - Flags: review?(jdemooij)
Attachment #8564628 - Flags: review?(jdemooij) → review+
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).
Attachment #8564720 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/a3cabc94db73
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Depends on: 1151401
Depends on: 1207210
You need to log in before you can comment on or make changes to this bug.