Status

()

--
minor
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: igor, Assigned: igor)

Tracking

unspecified
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

11 years ago
Consider the following fragments of js_NewGCThing and js_GC from jsgc.c:

void *
js_NewGCThing(JSContext *cx, uintN flags, size_t nbytes)
{
...
    doGC = (rt->gcMallocBytes >= rt->gcMaxMallocBytes);
#ifdef JS_GC_ZEAL
    if (rt->gcZeal >= 1) {
        doGC = JS_TRUE;
        if (rt->gcZeal >= 2)
            rt->gcPoke = JS_TRUE;
    }
#endif /* !JS_GC_ZEAL */

    arenaList = &rt->gcArenaList[flindex];
    for (;;) {
        if (doGC) {
	  ...
            js_GC(cx, GC_LAST_DITCH);
         }
...
            if (rt->gcBytes >= rt->gcMaxBytes || !(a = NewGCArena(rt))) {
                if (doGC)
                    goto fail;
                rt->gcPoke = JS_TRUE;
                doGC = JS_TRUE;
                continue;
            }
}

void
js_GC(JSContext *cx, JSGCInvocationKind gckind)
{
...
    if (gckind == GC_LAST_DITCH) {
...
    } else {
        JS_CLEAR_WEAK_ROOTS(&cx->weakRoots);
        rt->gcPoke = JS_TRUE;
...
    }

    /* Do nothing if no mutator has executed since the last GC. */
*** if (!rt->gcPoke) {
        METER(rt->gcStats.nopoke++);
        if (gckind != GC_LAST_DITCH)
            JS_UNLOCK_GC(rt);
        return;
    }
...
}

It is clear from this that at the line "***" gcPoke can only be false with the last ditch GC when either rt->"gcMallocBytes >= rt->gcMaxMallocBytes" or when rt->gcZeal == 1. 

Thus I suggest to move the checks for gcPoke to js_NewGCThing to avoid redundant checks in js_GC.
(Assignee)

Comment 1

11 years ago
Created attachment 280762 [details] [diff] [review]
v1

The patch moves all gcPoke checks into js_NewGCThing.
Attachment #280762 - Flags: review?(brendan)
(Assignee)

Comment 2

11 years ago
Created attachment 281175 [details] [diff] [review]
v2

This is a version that compiles: v1 contains ||=.
Attachment #280762 - Attachment is obsolete: true
Attachment #281175 - Flags: review?(brendan)
Attachment #280762 - Flags: review?(brendan)
(In reply to comment #0)
> It is clear from this that at the line "***" gcPoke can only be false with the
> last ditch GC when either rt->"gcMallocBytes >= rt->gcMaxMallocBytes" or when
> rt->gcZeal == 1. 

Long ago, rt->gcPoke could be false in the non-last-ditch (GC_NORMAL now) case too, however, and that might have been worth the test in js_GC if it prevented useless JS_MaybeGC=>js_GC work.

However those days are gone. Now it seems gcPoke is used only to restart GC if a finalizer runs a potential mutator. This is weak too, but it could be important to avoid shutdown leaks.

Nit: don't overparenthesize (as the previous line did) the doGC = (doGC || ...); statement's right-hand side.

/be
(Assignee)

Comment 4

11 years ago
(In reply to comment #3)
> Now it seems gcPoke is used only to restart GC if
> a finalizer runs a potential mutator. This is weak too, but it could be
> important to avoid shutdown leaks.

It also "helps" to hide leaks due to reference cycles. I think any code that removes an essential root in the finalizer (like calling RemoveRoot and friends) means that there is a uncollectable cycle to happen. 
(Assignee)

Comment 5

11 years ago
Created attachment 281228 [details] [diff] [review]
v3

Addressing the nit about overparenthesize in foo = (boolen_bar)
Attachment #281175 - Attachment is obsolete: true
Attachment #281228 - Flags: review?(brendan)
Attachment #281175 - Flags: review?(brendan)
(In reply to comment #4)
> (In reply to comment #3)
> > Now it seems gcPoke is used only to restart GC if
> > a finalizer runs a potential mutator. This is weak too, but it could be
> > important to avoid shutdown leaks.
> 
> It also "helps" to hide leaks due to reference cycles. I think any code that
> removes an essential root in the finalizer (like calling RemoveRoot and
> friends) means that there is a uncollectable cycle to happen.

Likely. In the Gecko world it also could (and often does) mean that some last XPCOM refcnt was dropped to 0 by a finalizer which created a great deal more garbage to collect.

But we can get past this best in actionmonkey/mozilla2 with the work to unify DOM and other allocations with JS on top of an evolved MMgc.

/be
> 

Updated

11 years ago
Attachment #281228 - Flags: review?(brendan)
Attachment #281228 - Flags: review+
Attachment #281228 - Flags: approval1.9+
(Assignee)

Comment 7

11 years ago
(In reply to comment #6)
> (In reply to comment #4)
> > It also "helps" to hide leaks due to reference cycles. I think any code that
> > removes an essential root in the finalizer (like calling RemoveRoot and
> > friends) means that there is a uncollectable cycle to happen.
> 
> Likely. In the Gecko world it also could (and often does) mean that some last
> XPCOM refcnt was dropped to 0 by a finalizer which created a great deal more
> garbage to collect.

Which points to a potentially problematic feature of the current tracing API. Since XPConnect register the tracing callback instead of calling AddRoot/RemoveRoot, gcPoke is not updated when an XPConnect root is removed and GC is not restarted when that happens. But it is not worth to do anything about it since if some embeddings happens to rely on gcPoke updates, then it should not use the tracing callback.
(Assignee)

Comment 8

11 years ago
I checked in the patch from comment 5 to the trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1190100120&maxdate=1190100230&who=igor%mir2.org
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.