396049 - Useless gcPoke checks

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

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.

Comment 1

[Igor Bukanov]

Attachment: v1

The patch moves all gcPoke checks into js_NewGCThing.

Comment 2

[Igor Bukanov]

Attachment: v2

This is a version that compiles: v1 contains ||=.

Comment 3

[Brendan Eich [: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

Comment 4

[Igor Bukanov]

(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. 

Comment 5

[Igor Bukanov]

Attachment: v3

Addressing the nit about overparenthesize in foo = (boolen_bar)

Comment 6

[Brendan Eich [: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
> 

Comment 7

[Igor Bukanov]

(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.

Comment 8

[Igor Bukanov]

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