Closed Bug 1019310 Opened 7 years ago Closed 7 years ago

Actually use BailoutKind and make it more detailed

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: shu, Assigned: vstamour)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
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.
Assigning to Vincent, who has agreed to work on this.
Assignee: nobody → vstamour
Blocks: 1019304
Status: NEW → ASSIGNED
Attachment #8436146 - Flags: review?(shu)
Also included a minor comment fix I did along the way, as a separate patch.
Attachment #8436143 - Flags: review?(shu) → review+
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?
Attachment #8436146 - Flags: review?(shu)
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.
New version of the patch that addresses your comments.
Try run: https://tbpl.mozilla.org/?tree=Try&rev=4dc37fdb8901
Attachment #8436146 - Attachment is obsolete: true
Attachment #8437996 - Flags: review?(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 &&
Attachment #8437996 - Flags: review?(shu) → review+
Fixed style issues.
Is there an emacs config available somewhere for the Mozilla style guidelines?
Attachment #8437996 - Attachment is obsolete: true
Attachment #8439346 - Flags: review?(shu)
Attachment #8439346 - Flags: review?(shu) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3cfffd1aaa40
https://hg.mozilla.org/mozilla-central/rev/7abab6174814
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.