PJS: Use BailoutKind to report bailouts

RESOLVED FIXED in mozilla33

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: shu, Assigned: shu)

Tracking

unspecified
mozilla33
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
n/t
(Assignee)

Updated

4 years ago
Assignee: nobody → shu
Status: NEW → ASSIGNED
(Assignee)

Comment 1

4 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

4 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

4 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+
(Assignee)

Comment 4

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

Comment 5

4 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

4 years ago
Backed out from mozilla-inbound for Jit failures
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae5b77cbf30d
https://hg.mozilla.org/mozilla-central/rev/1d5c7fdfe307
Status: ASSIGNED → RESOLVED
Last Resolved: 4 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.