Closed Bug 1028331 Opened 8 years ago Closed 8 years ago

PJS: Use BailoutKind to report bailouts


(Core :: JavaScript Engine: JIT, defect)

Not set





(Reporter: shu, Assigned: shu)



(1 file)

Assignee: nobody → shu
Lars, do you mind reviewing since Niko is on vacation?
Attachment #8444858 - Flags: review?(lhansen)
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 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+
You accidentally reviewed some deleted code. ;)
(In reply to Shu-yu Guo [:shu] from comment #4)
> You accidentally reviewed some deleted code. ;)

When I'm thorough, I'm thorough!
Backed out from mozilla-inbound for Jit failures
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.