1028331 - PJS: Use BailoutKind to report bailouts

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Shu-yu Guo [:shu]]

n/t

Comment 1

[Shu-yu Guo [:shu]]

Attachment: Use BailoutKind to report more detailed PJS bailout warnings.

Lars, do you mind reviewing since Niko is on vacation?

Comment 2

[Lars T Hansen [:lth]]

Comment on attachment 8444858 [details] [diff] [review]
Use BailoutKind to report more detailed PJS bailout warnings.

Preliminary comments (nits).

>diff --git a/js/src/vm/ForkJoin.cpp b/js/src/vm/ForkJoin.cpp
>index 0961d5f..c892324 100644
>--- a/js/src/vm/ForkJoin.cpp
>+++ b/js/src/vm/ForkJoin.cpp
>@@ -1037,51 +1037,107 @@ ForkJoinOperation::fatalError(ExecutionStatus *status)
[...]
>       case ParallelBailoutOverRecursed:
>-        return "over recursed";
>+        return "over-recursion";

See below re this name, "over-recursed" is less descriptive than "stack limit exceeded".

>       case ParallelBailoutRequestedGC:
>-        return "requested GC";
>+        return "requested GC of tenured heap";
>       case ParallelBailoutRequestedZoneGC:
>-        return "requested zone GC";
>+        return "requested zone GC of tenured heap";

Note the use of the term "tenured" is only correct if Generational GC is enabled, which is
not a requirement for PJS.  You could ifdef on JSGC_GENERATIONAL, I guess.  But I'd suggest
replacing "tenured" with "common" or "general".

>       default:
>-        return "no known reason";
>+        return "??";

I think since you bother to catch an unknown ID here you should print a useful message, eg, "Unknown ParallelBailout ID".  Possibly there should be an assert to catch this?

[...]
>+      case Bailout_ShapeGuard:
>+        return " (saw unexpected shape)";
>+      default:
>+        // Bailout_IonExceptionDebugMode cannot occur in parallel execution.
>+        MOZ_ASSUME_UNREACHABLE("Invalid BailoutKind");

One could turn the comment into a case and place it above default with a fallthrough, it might be clearer.

>@@ -1130,34 +1186,40 @@ ForkJoinOperation::reportBailoutWarnings()
>     for (uint32_t threadId = 0; threadId < bailoutRecords_.length(); threadId++) {
[...]
> 
>-            sp.printf("\n  in thread %d due to %s", threadId, BailoutExplanation(cause));
>+            sp.printf("\n  in thread %d: %s%s",
>+                      threadId, BailoutExplanation(cause),
>+                      IonBailoutKindExplanation(cause, ionBailoutKind));

%d is arguably wrong for 'uint32_t' threadId (two occurrences).

>diff --git a/js/src/vm/ForkJoin.h b/js/src/vm/ForkJoin.h
>index a57ae17..08e6178 100644
>--- a/js/src/vm/ForkJoin.h
>+++ b/js/src/vm/ForkJoin.h
>@@ -276,75 +277,76 @@ bool ForkJoin(JSContext *cx, CallArgs &args);
[...]
>+    // Bailed out of Ion code during execution. The specific reason is found
>+    // in the ionBailoutKind field in ParallelBailoutRecrod below.

Nit: "Recrod"

>-    // An IC update failed
>-    ParallelBailoutFailedIC,

Nit: you use periods consistently at the end of comments elsewhere

>-    // Heap busy flag was set during interrupt
>-    ParallelBailoutHeapBusy,

Nit: ditto

>+    // Went over the stack limit.
>     ParallelBailoutOverRecursed,

I know we use this name throughout but frankly "StackLimitExceeded" would have been more descriptive.

Comment 3

[Lars T Hansen [:lth]]

Comment on attachment 8444858 [details] [diff] [review]
Use BailoutKind to report more detailed PJS bailout warnings.

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

OK.  The only tricky thing here for me is the removal of the four calls to reportError().  For reportReadOnly() and reportNotExtensible() it's not yet clear to me why this is correct, but I'm going to take it on faith.

Comment 4

[Shu-yu Guo [:shu]]

You accidentally reviewed some deleted code. ;)

Comment 5

[Lars T Hansen [:lth]]

(In reply to Shu-yu Guo [:shu] from comment #4)
> You accidentally reviewed some deleted code. ;)

When I'm thorough, I'm thorough!

Comment 6

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/567152f907a1

Comment 7

[Nigel Babu [:nigelb]]

Backed out from mozilla-inbound for Jit failures
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae5b77cbf30d

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1d5c7fdfe307