751396 - Fire slice callbacks only for "outer" GCs

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

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.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Fire slice callbacks only for "outer" GCs

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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?

Comment 3

[Steve Fink [:sfink] [:s:]]

Attachment: 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.

Comment 4

[Steve Fink [:sfink] [:s:]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d53129e8414

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6d53129e8414