The default bug view has changed. See this FAQ.

Fire slice callbacks only for "outer" GCs

RESOLVED FIXED in mozilla15

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sfink, Assigned: sfink)

Tracking

unspecified
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Originally encountered when JSD hit an assertion failure in its slice callback because it saw two BEGINs in a row, because it kicked off another GC from jsdService::~jsdService (called from a JSGC_END callback).

In talking to billm, it seems like the slice callbacks would make the most sense if they only applied to the "outermost" GC.
(Assignee)

Comment 1

5 years ago
Created attachment 620493 [details] [diff] [review]
Fire slice callbacks only for "outer" GCs
Attachment #620493 - Flags: review?(wmccloskey)
Comment on attachment 620493 [details] [diff] [review]
Fire slice callbacks only for "outer" GCs

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

::: js/src/gc/Statistics.cpp
@@ +560,5 @@
> +        if (GCSliceCallback cb = runtime->gcSliceCallback) {
> +            if (last)
> +                (*cb)(runtime, GC_CYCLE_END, GCDescription(!wasFullGC));
> +            else
> +                (*cb)(runtime, GC_SLICE_END, GCDescription(!wasFullGC));

Could you change this to use a ternary operator? I think these cases used to be more different than they are now.

::: js/src/jscntxt.h
@@ +421,5 @@
>      /*
> +     * GCs can't really nest, but a second GC can be triggered from within the
> +     * JSGC_END callback.
> +     */
> +    int                 gcDepth;

Could you put this field in gc/Statistics.h instead?
(Assignee)

Comment 3

5 years ago
Created attachment 620511 [details] [diff] [review]
Fire slice callbacks only for "outer" GCs

>::: js/src/jscntxt.h
>@@ +421,5 @@
>>      /*
>> +     * GCs can't really nest, but a second GC can be triggered from within the
>> +     * JSGC_END callback.
>> +     */
>> +    int                 gcDepth;
>
>Could you put this field in gc/Statistics.h instead?

Oh! Somehow I didn't realize that Statistics are already runtime-specific, which is obvious in retrospect.
Attachment #620511 - Flags: review?(wmccloskey)
(Assignee)

Updated

5 years ago
Attachment #620493 - Attachment is obsolete: true
Attachment #620493 - Flags: review?(wmccloskey)
(Assignee)

Updated

5 years ago
Blocks: 751398
Attachment #620511 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 4

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d53129e8414
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/6d53129e8414
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.