308429 - make tooMuchGC dynamic (runtime gczeal option)

Status: VERIFIED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[timeless]

it takes a really really long time for me to start mozilla w/ toomuchgc, but i'd
like to be able to test other pieces under toomuchgc.

Comment 1

[timeless]

Attachment: draft

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 196015 [details] [diff] [review]
draft

I don't think we want this for DEBUG.

Also, we could unify things a bit, combine TOO_MUCH_GC and WAY_TOO_MUCH_GC to
be one macro with two levels of runtime control.

/be

Comment 3

[Brendan Eich [:brendan]]

Or, if we want this for DEBUG, let's get rid of the other ifdefs and just use
DEBUG for compile-time conditioning.

/be

Comment 4

[Brendan Eich [:brendan]]

Should we take the runtime hit for DEBUG?  It'll slow things down ever so
slightly, but DEBUG builds already crawl, right?

/be

Comment 5

[Brian Crowder]

Attachment: something to shoot down

Not sure if this is what we're after or not; Igor, mind taking a look?

Comment 6

[Igor Bukanov]

Comment on attachment 260420 [details] [diff] [review]
something to shoot down

>--- jsgc.c	1 Apr 2007 08:14:01 -0000	3.212
>+++ jsgc.c	3 Apr 2007 06:46:25 -0000
>@@ -1411,24 +1411,26 @@ js_NewGCThing(JSContext *cx, uintN flags
>-#ifdef TOO_MUCH_GC
>-#ifdef WAY_TOO_MUCH_GC
>-    rt->gcPoke = JS_TRUE;
>-#endif
>-    doGC = JS_TRUE;
>-#else
>+#ifdef TUNABLE_GC
>+    if (rt->gcVigor >= 2)
>+        rt->gcPoke = JS_TRUE;
>+    if (rt->gcVigor >= 1) 
>+        doGC = JS_TRUE;
>+    else
>+        doGC = (rt->gcMallocBytes >= rt->gcMaxMallocBytes);
>+#else  /* !TUNABLE_GC */
>     doGC = (rt->gcMallocBytes >= rt->gcMaxMallocBytes);
>-#endif
>+#endif /* !TUNABLE_GC */

For less source lines I suggest to write this as

    doGC = (rt->gcMallocBytes >= rt->gcMaxMallocBytes);
#ifdef TUNABLE_GC
    if (rt->gcVigor >= 1) {
        doGC = JS_TRUE;
        if (rt->gcVigor >= 2)      
            rt->gcPoke = JS_TRUE;
    }
#endif

>-#ifdef TOO_MUCH_GC
>-    STOBJ_SET_SLOT(obj, map->freeslot, JSVAL_VOID);
>+#ifdef TUNABLE_GC
>+    if (cx->runtime->gcLevel >= 1)
>+        STOBJ_SET_SLOT(obj, map->freeslot, JSVAL_VOID);
> #endif
>     *slotp = map->freeslot++;
>     return JS_TRUE;
> }

The if should be  if (cx->runtime->gcVigor >= 1)

I also think that we should drop that STOBJ_SET_SLOT(obj) altogether or replace it with  

#ifdef DEBUG
    STOBJ_SET_SLOT(obj, map->freeslot,
                   ObjectJSValThatCrashesonOnAccess);
#endif

If the vigorous GC requires that, then lets find out why.

Comment 7

[Igor Bukanov]

(In reply to comment #4)
> Should we take the runtime hit for DEBUG?  It'll slow things down ever so
> slightly, but DEBUG builds already crawl, right?

It would be nice to have this in DEBUG builds. The only hit AFAICS is a couple of extra "if" in the normal mode.

Comment 8

[Brian Crowder]

Attachment: with igor's feedback

Great feedback, thanks.  How about 0xdddddddd as the "bogus object ptr" for freed slots?

Comment 9

[Igor Bukanov]

Comment on attachment 260427 [details] [diff] [review]
with igor's feedback

>-#ifdef TOO_MUCH_GC
>-    STOBJ_SET_SLOT(obj, map->freeslot, JSVAL_VOID);
>+#ifdef DEBUG
>+    STOBJ_SET_SLOT(obj, map->freeslot, OBJECT_TO_JSVAL(0xdddddddd));
> #endif
>     *slotp = map->freeslot++;
>     return JS_TRUE;
> }

OBJECT_TO_JSVAL(0xdddddddd) does not work as it would be interpreted as a jsval meaning tagged int since this is an odd number. Using (jsval)0xddddddd8 should do a trick as any other hex that is dividable by 8. Plus with 64 bits one probably want 0xddddddddddddddd8L.

Comment 10

[Brian Crowder]

Attachment: 64-bit bogus jsval

I have no way of trying this out in a 64-bit environment.

Comment 11

[Brian Crowder]

Comment on attachment 260430 [details] [diff] [review]
64-bit bogus jsval

Brendan:  how do you feel about something like this?

Comment 12

[Brian Crowder]

brendan:  still hoping you get a chance to take a look at this before I land it.  Thanks

Comment 13

[Brian Crowder]

brendan: ping?

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 260430 [details] [diff] [review]
64-bit bogus jsval

>+#ifdef TUNABLE_GC
>+static JSBool
>+GCVigor(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
>+{
>+    unsigned int vigor;

Blank line here, prevailing style and all that.

>+    if (!JS_ConvertArguments(cx, argc, argv, "i", &vigor))

Then vigor must be int32, not unsigned int, for the "i" format (unsigned int could be a 64 bit int, so you'd pass the address of the wrong end on big and "pdp" endian systems). Use "u" for unsignedness. Also, there's no need for JS_ConvertAguments here -- with only one argument, use JS_ValueToECMAUint32.

>@@ -2118,16 +2130,19 @@ static JSFunctionSpec shell_functions[] 
>     {"version",         Version,        0,0,0},
>     {"options",         Options,        0,0,0},
>     {"load",            Load,           1,0,0},
>     {"readline",        ReadLine,       0,0,0},
>     {"print",           Print,          0,0,0},
>     {"help",            Help,           0,0,0},
>     {"quit",            Quit,           0,0,0},
>     {"gc",              GC,             0,0,0},
>+#ifdef TUNABLE_GC
>+    {"gcvigor",         GCVigor,        1,0,0},
>+#endif
>     {"trap",            Trap,           3,0,0},
>     {"untrap",          Untrap,         2,0,0},
>     {"line2pc",         LineToPC,       0,0,0},
>     {"pc2line",         PCToLine,       0,0,0},
>     {"stringsAreUTF8",  StringsAreUTF8, 0,0,0},
>     {"testUTF8",        TestUTF8,       1,0,0},
>     {"throwError",      ThrowError,     0,0,0},

You didn't follow the comment above this table and add a row to the help messages array to match.

> JS_PUBLIC_API(void)
> JS_MaybeGC(JSContext *cx)
> {
>-#ifdef WAY_TOO_MUCH_GC
>-    JS_GC(cx);
>-#else
>     JSRuntime *rt;
>     uint32 bytes, lastBytes;
> 
>     rt = cx->runtime;
>+
>+#ifdef TUNABLE_GC
>+    if (rt->gcVigor > 0) {
>+        JS_GC(cx);
>+        return;
>+    }
>+#endif

TUNABLE_GC is too vague a name (lots of GC parameters could be tuned depending on the GC alg and impl). It's also not distinguished by JS_ (neither was WAY_TOO_MUCH_GC, but that's old and arguably less generic a name, so less likely to collide, e.g., with some macro for Tamarin's MMgc).

Also, "vigor" as a metaphor isn't the same as "tunable", or as descriptive as "too much"/"way too much". Suggest GC_EXCESSIVENESS / JS_SetGCExcessiveness or something like that.

>+#ifdef TUNABLE_GC
>+JS_PUBLIC_API(void)
>+JS_SetGCVigor(JSContext *cx, unsigned int vigor)

Use uintN or perhaps even uint8 for the parameter. uintN is the way to shorten unsigned int in SpiderMonkey.

>@@ -191,16 +191,19 @@ struct JSRuntime {
>      * NB: do not pack another flag here by claiming gcPadding unless the new
>      * flag is written only by the GC thread.  Atomic updates to packed bytes
>      * are not guaranteed, so stores issued by one thread may be lost due to
>      * unsynchronized read-modify-write cycles on other threads.
>      */
>     JSPackedBool        gcPoke;
>     JSPackedBool        gcRunning;
>     uint16              gcPadding;
>+#ifdef TUNABLE_GC
>+    uint8               gcVigor;
>+#endif

Packing means shrinking the padding -- this change just adds another whole word, without using a byte from gcPadding.

/be

Comment 15

[Brian Crowder]

Attachment: addressing review issues

Thanks, Brendan, this should address everything.

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 263689 [details] [diff] [review]
addressing review issues


>+#ifdef JS_GC_EXCESSIVENESS
>+JS_PUBLIC_API(void)
>+JS_SetGCExcessiveness(JSContext *cx, uint8 vigor)
>+{
>+    JSRuntime *rt = cx->runtime;
>+    rt->gcExcessiveness = vigor;

Thought I noted this last time -- no blank line between decl and stmt, but bigger nit: no need for single-use rt variable.

One thought, tardy I'm afraid, but what do you think? "Zeal" instead of the overlong "Excessiveness" for the name of this GC quality.

r=me with these comments addressed.

/be

Comment 17

[Brian Crowder]

Attachment: long live the Protoss

I still like vigor best, but don't feel inclined to defend it vigorously.  This is the patch I will land shortly

Comment 18

[Brian Crowder]

Leaving unFIXED until I do the about:config code.

Checking in js.c;
/cvsroot/mozilla/js/src/js.c,v  <--  js.c
new revision: 3.141; previous revision: 3.140
done
Checking in jsapi.c;
/cvsroot/mozilla/js/src/jsapi.c,v  <--  jsapi.c
new revision: 3.320; previous revision: 3.319
done
Checking in jsapi.h;
/cvsroot/mozilla/js/src/jsapi.h,v  <--  jsapi.h
new revision: 3.150; previous revision: 3.149
done
Checking in jscntxt.h;
/cvsroot/mozilla/js/src/jscntxt.h,v  <--  jscntxt.h
new revision: 3.146; previous revision: 3.145
done
Checking in jsgc.c;
/cvsroot/mozilla/js/src/jsgc.c,v  <--  jsgc.c
new revision: 3.218; previous revision: 3.217
done
Checking in jsgc.h;
/cvsroot/mozilla/js/src/jsgc.h,v  <--  jsgc.h
new revision: 3.69; previous revision: 3.68
done
Checking in jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.336; previous revision: 3.335
done

Comment 19

[Daniel Veditz [:dveditz]]

Although this does not strictly meet the "security fix" criteria for branch releases, having this on the branch would help us verify GC-related fixes (like bug 381374) we do take there. The risk looks almost non-existent, does that seem like a reasonable plan?

Comment 20

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Enable GCZeal via prefs

Here's a patch to enable runtime switching of the GCZeal flag via about:config

Comment 21

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 283647 [details] [diff] [review]
Enable GCZeal via prefs

Very low risk patch to add support for changing the GC zeal at runtime through prefs/about:config.

Comment 22

[Brendan Eich [:brendan]]

Argh, how did I miss my own warning in jscntxt.h:

     * NB: do not pack another flag here by claiming gcPadding unless the new
     * flag is written only by the GC thread.  Atomic updates to packed bytes
     * are not guaranteed, so stores issued by one thread may be lost due to
     * unsynchronized read-modify-write cycles on other threads.

See bug 401188.

/be

Comment 23

[Mike Schroepfer]

(In reply to comment #22)
> Argh, how did I miss my own warning in jscntxt.h:
> 
>      * NB: do not pack another flag here by claiming gcPadding unless the new
>      * flag is written only by the GC thread.  Atomic updates to packed bytes
>      * are not guaranteed, so stores issued by one thread may be lost due to
>      * unsynchronized read-modify-write cycles on other threads.
> 
> See bug 401188.
> 
> /be
> 

Sorry - not sure i follow - is this patch ok to go or does it require some changes?

Comment 24

[Brian Crowder]

This bug will be fixed by the landing of the patch in bug 401188

Comment 25

[Mike Schroepfer]

Comment on attachment 283647 [details] [diff] [review]
Enable GCZeal via prefs

clearing the approval flag based on the last comment

Comment 26

[Brian Crowder]

Ahhh, woops.  My fault; the "Enable GCZeal via prefs" attachment is the DOM portion of this bug, and does need approval to land (unrelated to bug 401188, to which comment 22 refers).

Comment 27

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment 283647 [details] [diff] now landed on trunk. I believe this is now FIXED.

Comment 28

[Boris Zbarsky [:bzbarsky]]

It looks like this patch missed a WAY_TOO_MUCH_GC consumer in nsJSEnvironment, so now we have to both set the pref _and_ set the define...

Comment 29

[Boris Zbarsky [:bzbarsky]]

It would also be great if there were some documentation for this (e.g. in all.js).

Comment 30

[Daniel Veditz [:dveditz]]

It's looking like we want this on the 1.8 branch in order to better test for and prevent regressions.

Comment 31

[Brian Crowder]

Should clear the wanted/blocking flags on these bugs, as they are replaced for the 1.8 branch by the rollup patch in bug 426628.

Comment 32

[Samuel Sidler (old account; do not CC)]

(In reply to comment #31)
> Should clear the wanted/blocking flags on these bugs, as they are replaced for
> the 1.8 branch by the rollup patch in bug 426628.

We should actually mark all of those bugs (and this one) as fixed1.8.1.15 (when that patch lands). QA will need to go through and verify they're all indeed fixed on the branch to ensure that nothing go missed in the rollup.

Comment 33

[Bob Clary [:bc] (inactive)]

verified1.8.1.15 that javascript.options.gczeal works on the 1.8.1 branch now as well as on the 1.9.0 branch.

Comment 34

[Eric Shepherd [:sheppy]]

I'm trying to distill this down into documentation.  Let me summarize what I believe here, and someone please tell me if I'm close to right:

If SpiderMonkey is built with the JS_GC_ZEAL option, a new built-in function is added to SpiderMonkey that lets you configure zealous garbage collection (although I don't know what legal values are).

In addition, it adds a new about:config pref, javascript.options.gczeal, which lets you set this value as well.

Correct?

Comment 35

[Brian Crowder]

Valid values are 0, 1, and 2, but I'm not sure if 1 is now (or ever was) really very meaningful.  Otherwise, you've got it right.

Comment 36

[Brian Crowder]

0 is gc as normal.  2 is gc whenever possible (ie., before allocating any new thing).

Comment 37

[Igor Bukanov]

(In reply to comment #36)
> 0 is gc as normal.  2 is gc whenever possible (ie., before allocating any new
> thing).

1 is gc when SpiderMonkey thinks that some garbage could be produced since the last GC. In practice this is only useful to test the rooting in the embedding, like missed js_AddRoot. It is not useful for stress-testing SpiderMonkey itself as GC bugs happens when SM misses that something became unrooted.

Comment 38

[Eric Shepherd [:sheppy]]

Turns out someone had already documented most of this.  Finished it up.