1019310 - Actually use BailoutKind and make it more detailed

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Shu-yu Guo [:shu]]

BailoutKind is pretty coarse grained at the moment. It'd be nice to be more specific, as well as actually use it.

This should be a matter of:

 1) Looking for calls to |assignSnapshot| during Lowering.
 2) Inspecting the context.
 3) Add a new BailoutKind kind if necessary, and threading that value through.

This will allow more detailed reporting of bailouts.

Comment 1

[Shu-yu Guo [:shu]]

It occurs to me that |assignSafepoint| will need a similar treatment as above, as that builds a snapshot currently, but always passes in |Bailout_Normal| for the kind, which isn't detailed enough.

Comment 2

[Shu-yu Guo [:shu]]

Assigning to Vincent, who has agreed to work on this.

Comment 3

[Vincent St-Amour [:vstamour]]

Attachment: 0001-Remove-comment-about-x86-in-ARM-specific-code.patch

Comment 4

[Vincent St-Amour [:vstamour]]

Attachment: 0002-Make-bailout-kinds-more-precise.patch

Comment 5

[Vincent St-Amour [:vstamour]]

Also included a minor comment fix I did along the way, as a separate patch.

Comment 6

[Vincent St-Amour [:vstamour]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=fce4bdc1011f

Comment 7

[Shu-yu Guo [:shu]]

Comment on attachment 8436146 [details] [diff] [review]
0002-Make-bailout-kinds-more-precise.patch

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

Looks pretty good overall!

I'd like to see the Bailout_Inevitable and assignSafepoint comments addressed first before r+.

::: js/src/jit/IonBuilder.cpp
@@ +9921,4 @@
>          return false;
>  
>      if (!nonStringIteration_ && !inspector->hasSeenNonStringIterNext(pc)) {
> +        ins = MUnbox::New(alloc(), ins, MIRType_String, MUnbox::Fallible, Bailout_NonStringInputInvalidate);

Nit: SpiderMonkey style is 100-chars max.

::: js/src/jit/IonTypes.h
@@ +35,5 @@
>  // the bits reserved for bailout kinds in Bailouts.h
>  enum BailoutKind
>  {
> +    // Normal bailouts, that don't need to be handled specially when restarting
> +    // in baseline and don't cause immediate invalidation.

I feel like this comment is misleading. Once we bail out, something or other will most likely trigger an invalidation anyways, even if it doesn't go directly to the invalidation bailout.

@@ +46,5 @@
> +    // Call to a non-JSFunction (problem for |apply|)
> +    Bailout_NonJSFunctionCallee,
> +
> +    // An inevitable bailout (MBail instruction or type barrier that always bails)
> +    Bailout_Inevitable,

Perhaps this can be first?

@@ +101,5 @@
> +
> +    Bailout_GuardThreadExclusive,
> +
> +    // For the initial snapshot when entering a function.
> +    Bailout_VisitStart,

Maybe just Bailout_InitialState?

::: js/src/jit/Lowering.cpp
@@ +512,4 @@
>  LIRGenerator::visitBail(MBail *bail)
>  {
>      LBail *lir = new(alloc()) LBail();
> +    return assignSnapshot(lir, Bailout_Inevitable) && add(lir, bail);

MBail isn't exclusively used for inevitable bailouts (we'll start using it in PJS, for instance).

Could you make MBail itself keep a BailoutKind, and thread it through?

@@ +555,4 @@
>                                                          tempFixed(CallTempReg3),
>                                                          tempFixed(CallTempReg4));
>  
> +    return assignSnapshot(lir, Bailout_UndefinedOutput) && defineReturn(lir, ins);

Hm, this is probably more descriptive as a "dynamic name not found" rather than "undefined output". What do you think?

@@ +1807,4 @@
>          LValueToInt32 *lir = new(alloc()) LValueToInt32(tempDouble(), temp(), LValueToInt32::NORMAL);
>          if (!useBox(lir, LValueToInt32::Input, opd))
>              return false;
> +        return assignSnapshot(lir, Bailout_NonPrimitiveInput) && define(lir, convert) && assignSafepoint(lir, convert);

Nit: 100 chars width.

@@ +3005,4 @@
>  LIRGenerator::visitGuardObjectIdentity(MGuardObjectIdentity *ins)
>  {
>      LGuardObjectIdentity *guard = new(alloc()) LGuardObjectIdentity(useRegister(ins->obj()));
> +    return assignSnapshot(guard, Bailout_ObjectIdentityOrTypeGuard) && add(guard, ins) && redefine(ins, ins->obj());

Nit: 100 char width.

::: js/src/jit/MIR.h
@@ +2653,5 @@
>      INSTRUCTION_HEADER(Unbox)
>      static MUnbox *New(TempAllocator &alloc, MDefinition *ins, MIRType type, Mode mode)
>      {
> +        // Unless we were given a specific BailoutKind, pick a default based on
> +        // the type we expect.

Ooh, nice.

::: js/src/jit/Snapshots.cpp
@@ +482,4 @@
>  
>  // Details of snapshot header packing.
>  static const uint32_t SNAPSHOT_BAILOUTKIND_SHIFT = 0;
> +static const uint32_t SNAPSHOT_BAILOUTKIND_BITS = 5;

I'm pretty sure 2^27 should be large enough for any offsets we have. Luckily there are asserts in place to make sure we don't lose precision when we try to encode the offset.

::: js/src/jit/shared/Lowering-shared.cpp
@@ +222,4 @@
>      ins->initSafepoint(alloc());
>  
>      MResumePoint *mrp = mir->resumePoint() ? mir->resumePoint() : lastResumePoint_;
> +    LSnapshot *postSnapshot = buildSnapshot(ins, mrp, Bailout_DuringVMCall);

What do you think about making assignSafepoint take a BailoutKind, but default it to Bailout_DuringVMCall, so users can pass in a more specific reason?

Comment 8

[Vincent St-Amour [:vstamour]]

Re "Once we bail out, something or other will most likely trigger an invalidation anyways.":
Out of curiosity, what kinds of things will? I'll adjust the comment.

Re "undefined output":
I was expecting there would be other operations that would bail for the same reason, so I kept it generic. Turns out there are none, so I'll replace it with a more specific name.

I'll update the patch.

Comment 9

[Vincent St-Amour [:vstamour]]

Attachment: 0002-Make-bailout-kinds-more-precise.patch

New version of the patch that addresses your comments.
Try run: https://tbpl.mozilla.org/?tree=Try&rev=4dc37fdb8901

Comment 10

[Shu-yu Guo [:shu]]

Comment on attachment 8437996 [details] [diff] [review]
0002-Make-bailout-kinds-more-precise.patch

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

Looks great!

::: js/src/jit/Lowering.cpp
@@ +577,5 @@
>      }
>  
> +    return assignSnapshot(lir, Bailout_StringArgumentsEval)
> +        && add(lir, ins)
> +        && assignSafepoint(lir, ins);

Style nit: here and below, line && up to assignSnapshot, i.e.

return assignSnapshot(lir, Bailout_StringArgumentsEval)
       && add(lir, ins)
       && assignSafepoint(lir, ins);

@@ +1807,5 @@
>          if (!useBox(lir, LValueToInt32::Input, opd))
>              return false;
> +        return assignSnapshot(lir, Bailout_NonPrimitiveInput)
> +            && define(lir, convert)
> +            && assignSafepoint(lir, convert);

Ditto on lining up &&

@@ +1853,4 @@
>          LValueToInt32 *lir = new(alloc()) LValueToInt32(tempDouble(), temp(), LValueToInt32::TRUNCATE);
>          if (!useBox(lir, LValueToInt32::Input, opd))
>              return false;
> +        return assignSnapshot(lir, Bailout_NonPrimitiveInput) && define(lir, truncate) && assignSafepoint(lir, truncate);

Style nit: 100-char limit.

@@ +2773,4 @@
>          LClampVToUint8 *lir = new(alloc()) LClampVToUint8(tempDouble());
>          if (!useBox(lir, LClampVToUint8::Input, in))
>              return false;
> +        return assignSnapshot(lir, Bailout_NonPrimitiveInput) && define(lir, ins) && assignSafepoint(lir, ins);

Style nit: 100-char limit.

@@ +3017,5 @@
>  {
>      LGuardObjectIdentity *guard = new(alloc()) LGuardObjectIdentity(useRegister(ins->obj()));
> +    return assignSnapshot(guard, Bailout_ObjectIdentityOrTypeGuard)
> +        && add(guard, ins)
> +        && redefine(ins, ins->obj());

Style nit: line up &&

Comment 11

[Vincent St-Amour [:vstamour]]

Attachment: 0002-Make-bailout-kinds-more-precise.patch

Fixed style issues.
Is there an emacs config available somewhere for the Mozilla style guidelines?

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3cfffd1aaa40
https://hg.mozilla.org/integration/mozilla-inbound/rev/7abab6174814

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3cfffd1aaa40
https://hg.mozilla.org/mozilla-central/rev/7abab6174814