PJS: Use BailoutKind to report bailouts

RESOLVED FIXED in mozilla33



5 years ago
4 years ago


(Reporter: shu, Assigned: shu)



Firefox Tracking Flags

(Not tracked)



(1 attachment)



5 years ago


5 years ago
Assignee: nobody → shu

Comment 1

5 years ago
Created attachment 8444858 [details] [diff] [review]
Use BailoutKind to report more detailed PJS bailout warnings.

Lars, do you mind reviewing since Niko is on vacation?
Attachment #8444858 - Flags: review?(lhansen)

Comment 2

5 years ago
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

5 years ago
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.
Attachment #8444858 - Flags: review?(lhansen) → review+

Comment 4

5 years ago
You accidentally reviewed some deleted code. ;)

Comment 5

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

When I'm thorough, I'm thorough!

Comment 7

5 years ago
Backed out from mozilla-inbound for Jit failures
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Depends on: 1037657
No longer depends on: 1037657
You need to log in before you can comment on or make changes to this bug.