Last Comment Bug 751396 - Fire slice callbacks only for "outer" GCs
: Fire slice callbacks only for "outer" GCs
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Steve Fink [:sfink] [:s:] (PTO Sep23-28)
:
:
Mentors:
Depends on:
Blocks: 751398
  Show dependency treegraph
 
Reported: 2012-05-02 15:50 PDT by Steve Fink [:sfink] [:s:] (PTO Sep23-28)
Modified: 2012-05-04 09:26 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fire slice callbacks only for "outer" GCs (4.82 KB, patch)
2012-05-02 15:51 PDT, Steve Fink [:sfink] [:s:] (PTO Sep23-28)
no flags Details | Diff | Splinter Review
Fire slice callbacks only for "outer" GCs (4.87 KB, patch)
2012-05-02 16:20 PDT, Steve Fink [:sfink] [:s:] (PTO Sep23-28)
wmccloskey: review+
Details | Diff | Splinter Review

Description Steve Fink [:sfink] [:s:] (PTO Sep23-28) 2012-05-02 15:50:56 PDT
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:] (PTO Sep23-28) 2012-05-02 15:51:04 PDT
Created attachment 620493 [details] [diff] [review]
Fire slice callbacks only for "outer" GCs
Comment 2 Bill McCloskey (:billm) 2012-05-02 16:03:28 PDT
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:] (PTO Sep23-28) 2012-05-02 16:20:20 PDT
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.
Comment 4 Steve Fink [:sfink] [:s:] (PTO Sep23-28) 2012-05-03 14:01:20 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d53129e8414
Comment 5 Ed Morley [:emorley] 2012-05-04 09:26:30 PDT
https://hg.mozilla.org/mozilla-central/rev/6d53129e8414

Note You need to log in before you can comment on or make changes to this bug.