482038 - Removal of JSRuntime.gcPoke checks from js_NewGCThing

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

[This is a followup for bug 481922 comment 11]

Currently js_NewGCThing triggers the last ditch GC when it hits the malloc allocation limit but only when the JSRuntime.gcPoke flag is set. The idea behind the flag check is to prevent triggering too-frequent GC when it is known that no new garbage was added to the system. For example, without this check the following code fragment will run a useless GC 3 times:

var a = Array(40<<20);
var b = [];
b.push(a.join('.'), a.join('.'), a.join('.'));

Here a.join('.') creates a char array with 40MB chars resulting in JS_malloc() call to get 80MB bytes before allocating the string. This single malloc exceeds the default 64MB malloc limit that the JS shell uses. Thus the following allocation of a new JS string (that represents the join call result) in js_NewGCThing would trigger the GC. Without the gcPoke checks this GC would happen for each join call, but with the checks the following 2 calls are avoided since gcPoke would remain false after the first GC.

So far so good. The problem with this optimization is that the code must ensure that each and every write to a location that can store a GC thing must set gcPoke. As bug 477576, bug 481922, bug 482016 and bug 482018 demonstrates, it is well too easy to forget about this requirement resulting in excessive memory usage or even out-of-memory conditions when most of the memory is filled with collectable garbage.

Moreover, this optimization does not get much in terms of performance as the situations when it applies the area filled with GC things would be typically much smaller then the malloc-allocated memory. Thus the execution would be dominated not by the time the GC spends to mark alive GC things, but rather by the time to fill that malloc memory.

So, given the extremely synthetic nature of the examples when the optimization can get a small performance improvement, I suggest to remove JSRuntime.gcPoke checks from the allocation code. This not only fixes all current and future missing-gcPoke-setting bugs but also would improve performance as the interpreter and jited code would not need to constantly set gcPoke.

Comment 1

[Igor Bukanov]

Attachment: v1

The patch removes gcPoke checks from the allocation functions and GC_POKE() from the interpreter loop. It is possible to remove more GC_POKE() calls but that would require to check which part of the code cannot run from a finalizer during the GC.

Comment 2

[David Mandelin [:dmandelin]]

Sounds like a good idea to me. I just tested your patch on SS and v8 and found no slowdown on either one.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 366101 [details] [diff] [review]
v1

>Index: tm/js/src/jsgc.cpp
>===================================================================
>--- tm.orig/js/src/jsgc.cpp    2009-03-07 15:57:12.000000000 +0100
>+++ tm/js/src/jsgc.cpp    2009-03-07 15:59:08.000000000 +0100
>@@ -1862,20 +1862,20 @@ js_NewGCThing(JSContext *cx, uintN flags
> #endif
>     JS_ASSERT(!rt->gcRunning);
>     if (rt->gcRunning) {
>         METER(rt->gcStats.finalfail++);
>         JS_UNLOCK_GC(rt);
>         return NULL;
>     }
> 
>-    doGC = (rt->gcMallocBytes >= rt->gcMaxMallocBytes && rt->gcPoke) ||
>+    doGC = (rt->gcMallocBytes >= rt->gcMaxMallocBytes) ||

No need for parens around >= against ||.

r=me, thanks -- great to be rid of this old GC_POKE hack!

/be

Comment 4

[Igor Bukanov]

Attachment: v2

The new patch moves all GC-heuristics checks into IsGCThresholdReached for simpler code.

Comment 5

[Igor Bukanov]

landed to TM - http://hg.mozilla.org/tracemonkey/rev/6373919ecd37

Comment 6

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/6373919ecd37

Comment 8

[Robert Sayre]

fixed1.9.1 a while back:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/487ed396a288