891010 - Track causes of ion bailouts, both in sequential and parallel code

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Niko Matsakis [:nmatsakis]]

We don't currently have any way to easily determine what caused any particular bailout (for example, a failed type guard vs stumbling onto some unimplemented hot path, etc). My particular use case is providing feedback to users for parallel bailouts, but this kind of information is also useful in sequential code that is trying to stay on the Hot Path, as well as internal debugging etc. Patch forthcoming.

Comment 1

[Nicolas B. Pierron [:nbp] {back on 17/11}]

I think this is a good idea

Still, this only make sense in sequential mode as long as we are not able to answer Bug 825268, which is a mean to react to *frequent* bailouts.

In parallel mode, I guess you want to pin-point the code path which is causing side-effects, right?

Comment 2

[Niko Matsakis [:nmatsakis]]

Attachment: Track origins of bailouts

Track the causes of bailouts during JIT execution. The bailout causes are only
    used right now in parallel execution. One downside of this commit is that we
    currently have no "pretty printed" version of the bailout enum, we simply
    return the enum value itself.  This is good enough for the time being but
    obviously needs improvement before parallel execution is used by actual users.

Comment 3

[Hannes Verschore [:h4writer]]

(In reply to Nicolas B. Pierron [:nbp] from comment #1)
> Still, this only make sense in sequential mode as long as we are not able to
> answer Bug 825268, which is a mean to react to *frequent* bailouts.

It also makes sense without bug 825268. It would really be awesome the have that informations returned for IONFLAGS=bailouts .

Comment 4

[Nicolas B. Pierron [:nbp] {back on 17/11}]

Comment on attachment 773578 [details] [diff] [review]
Track origins of bailouts

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

Can you provide more context to the patch.

::: js/src/parjs-benchmarks/liquid-resize-par.js
@@ +324,5 @@
>        }
>      }
>  
> +    var result = (Math.abs(totalX) + Math.abs(totalY))/8.0;
> +    return result | 0;

should this change be part of this patch?

::: js/src/vm/ForkJoin.cpp
@@ +999,5 @@
>  
> +void
> +js::ParallelDo::issueBailoutWarning()
> +{
> +    const char *causeStr = BailoutCauseStrings[bailoutCause];

Where is BailoutCauseStrings defined?  It would be good if it were defined next to the enum definition.

::: js/src/vm/ForkJoin.h
@@ +11,4 @@
>  #include "jsgc.h"
>  #include "ion/Ion.h"
> +#include "vm/ThreadPool.h"
> +#include "vm/BailoutCause.h"

Where is this file defined In do not see it in my working copy nor in this patch.

Comment 5

[Niko Matsakis [:nmatsakis]]

Attachment: Track origins of bailouts

Sorry about the missing files. Should be better now.

Comment 6

[Nicolas B. Pierron [:nbp] {back on 17/11}]

Comment on attachment 774192 [details] [diff] [review]
Track origins of bailouts

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

This looks good so far, I haven't checked if all bailouts were correctly associated, but the logic sounds right.
Still I will prefer that the strings are defined with the enum, almost as we do for the documentation of all shell primitive, such as nobody can add a new bailout cause without taking care of providing a bit of documentation for it.

Q: How do we localize these strings, are they localized later as part of the UI?
Q: What is the API for discovering the bailout reasons?  Do we attach the reason to the JSScript?

::: js/src/ion/shared/CodeGenerator-shared.h
@@ +362,3 @@
>                                               MBasicBlock *basicBlock,
>                                               jsbytecode *bytecode);
> +    OutOfLineParallelAbort *oolParallelAbort(BailoutCause cause,

This is not part of this patch, but I will recommend that you try to optimize the size of the code generated by visitOutOfLineParallelAbort.

::: js/src/vm/BailoutCause.cpp
@@ +1,3 @@
> +#include "BailoutCause.h"
> +
> +#define JS_BAILOUT_STRING(x) #x

(see Bailout.h before these comments)
and this becomes:

#define JS_BAILOUT_STRING(x, cause) cause,

Attention, you need the comma otherwise you will have a table of only one entry.

@@ +1,4 @@
> +#include "BailoutCause.h"
> +
> +#define JS_BAILOUT_STRING(x) #x
> +const char *js::BailoutCauseStrings[] = {

Use BailoutLastCause to specify the size of the table:

const char *js::BailoutCauseStrings[BailoutLastCause + 1] = {
   JS_BAILOUT_CAUSES(JS_BAILOUT_STRING)
   "last bailout cause"
};

::: js/src/vm/BailoutCause.h
@@ +3,5 @@
> +
> +///////////////////////////////////////////////////////////////////////////
> +// Bailout tracking
> +
> +#define JS_BAILOUT_CAUSES(_)                                                        \

The Reason of the bailout should be part of this macro:

#define JS_BAILOUT_CAUSES(_) \
  […]
  _(BailoutNegativeZero, "Optimized integral operation cannot handle -0 double values.") \
  […]

If needed, you can span the text on multiple line such as what us done in shell/js.cpp for primitive functions declared with JS_FN_HELP.

@@ +106,5 @@
> +namespace js {
> +
> +enum BailoutCause {
> +#define JS_BAILOUT_ENUM(x) x
> +    JS_BAILOUT_CAUSES(JS_BAILOUT_ENUM)

and this becomes:

#define JS_BAILOUT_ENUM(name, reason) x
  JS_BAILOUT_CAUSES(JS_BAILOUT_ENUM)

#undef JS_BAILOUT_ENUM

@@ +108,5 @@
> +enum BailoutCause {
> +#define JS_BAILOUT_ENUM(x) x
> +    JS_BAILOUT_CAUSES(JS_BAILOUT_ENUM)
> +#undef JS_BAILOUT_ENUM
> +};

before the end of the enum, I suggest that you add a BailoutLastCause.

::: js/src/vm/ForkJoin.cpp
@@ +973,2 @@
>      for (uint32_t i = 0; i < bailoutRecords_.length(); i++) {
> +        if (bailoutRecords_[i].cause == BailoutNone)

Shouldn't be this renamed BailoutUnkonwn instead of BailoutNone?

Comment 7

[Niko Matsakis [:nmatsakis]]

I think this is bitrotted, going to close.