Last Comment Bug 308429 - make tooMuchGC dynamic (runtime gczeal option)
: make tooMuchGC dynamic (runtime gczeal option)
Status: VERIFIED FIXED
[sg:want P4]
: dev-doc-complete, verified1.8.1.15
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Windows XP
: -- enhancement (vote)
: ---
Assigned To: Brian Crowder
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 380833 401188 414982
Blocks: 426628
  Show dependency treegraph
 
Reported: 2005-09-13 23:55 PDT by timeless
Modified: 2009-02-17 09:32 PST (History)
15 users (show)
dveditz: blocking1.8.1.15+
dveditz: wanted1.8.1.x+
bob: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
draft (2.69 KB, patch)
2005-09-14 00:00 PDT, timeless
brendan: review-
Details | Diff | Splinter Review
something to shoot down (8.08 KB, patch)
2007-04-02 23:48 PDT, Brian Crowder
no flags Details | Diff | Splinter Review
with igor's feedback (8.09 KB, patch)
2007-04-03 00:20 PDT, Brian Crowder
no flags Details | Diff | Splinter Review
64-bit bogus jsval (8.08 KB, patch)
2007-04-03 00:45 PDT, Brian Crowder
igor: review+
brendan: superreview-
Details | Diff | Splinter Review
addressing review issues (9.34 KB, patch)
2007-05-03 20:40 PDT, Brian Crowder
no flags Details | Diff | Splinter Review
long live the Protoss (9.12 KB, patch)
2007-05-15 16:13 PDT, Brian Crowder
crowderbt: review+
Details | Diff | Splinter Review
Enable GCZeal via prefs (1.97 KB, patch)
2007-10-04 17:59 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
jst: review+
jst: superreview+
mtschrep: approval1.9+
Details | Diff | Splinter Review

Description timeless 2005-09-13 23:55:51 PDT
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 2005-09-14 00:00:01 PDT
Created attachment 196015 [details] [diff] [review]
draft
Comment 2 Brendan Eich [:brendan] 2005-09-14 10:55:30 PDT
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] 2005-09-14 15:23:02 PDT
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] 2005-09-14 15:30:08 PDT
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 2007-04-02 23:48:12 PDT
Created attachment 260420 [details] [diff] [review]
something to shoot down

Not sure if this is what we're after or not; Igor, mind taking a look?
Comment 6 Igor Bukanov 2007-04-03 00:08:59 PDT
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 2007-04-03 00:11:48 PDT
(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 2007-04-03 00:20:04 PDT
Created attachment 260427 [details] [diff] [review]
with igor's feedback

Great feedback, thanks.  How about 0xdddddddd as the "bogus object ptr" for freed slots?
Comment 9 Igor Bukanov 2007-04-03 00:33:02 PDT
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 2007-04-03 00:45:55 PDT
Created attachment 260430 [details] [diff] [review]
64-bit bogus jsval

I have no way of trying this out in a 64-bit environment.
Comment 11 Brian Crowder 2007-04-03 00:56:10 PDT
Comment on attachment 260430 [details] [diff] [review]
64-bit bogus jsval

Brendan:  how do you feel about something like this?
Comment 12 Brian Crowder 2007-04-16 10:59:39 PDT
brendan:  still hoping you get a chance to take a look at this before I land it.  Thanks
Comment 13 Brian Crowder 2007-05-03 14:58:40 PDT
brendan: ping?
Comment 14 Brendan Eich [:brendan] 2007-05-03 16:21:23 PDT
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 2007-05-03 20:40:08 PDT
Created attachment 263689 [details] [diff] [review]
addressing review issues

Thanks, Brendan, this should address everything.
Comment 16 Brendan Eich [:brendan] 2007-05-14 16:45:38 PDT
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 2007-05-15 16:13:23 PDT
Created attachment 264931 [details] [diff] [review]
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 2007-05-15 16:27:52 PDT
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] 2007-06-28 13:45:59 PDT
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!) 2007-10-04 17:59:50 PDT
Created attachment 283647 [details] [diff] [review]
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!) 2007-10-05 13:19:00 PDT
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] 2007-10-25 19:34:24 PDT
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 2007-11-09 10:42:52 PST
(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 2007-11-09 10:47:38 PST
This bug will be fixed by the landing of the patch in bug 401188
Comment 25 Mike Schroepfer 2007-11-09 11:37:44 PST
Comment on attachment 283647 [details] [diff] [review]
Enable GCZeal via prefs

clearing the approval flag based on the last comment
Comment 26 Brian Crowder 2007-11-09 12:00:41 PST
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!) 2007-11-11 11:01:08 PST
Attachment 283647 [details] [diff] now landed on trunk. I believe this is now FIXED.
Comment 28 Boris Zbarsky [:bz] (still a bit busy) 2008-01-30 08:47:03 PST
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 [:bz] (still a bit busy) 2008-01-30 08:48:07 PST
It would also be great if there were some documentation for this (e.g. in all.js).
Comment 30 Daniel Veditz [:dveditz] 2008-04-02 15:05:10 PDT
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 2008-06-03 08:30:35 PDT
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) 2008-06-03 10:17:19 PDT
(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:] 2008-06-11 07:33:25 PDT
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] 2009-02-17 09:10:11 PST
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 2009-02-17 09:16:10 PST
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 2009-02-17 09:16:56 PST
0 is gc as normal.  2 is gc whenever possible (ie., before allocating any new thing).
Comment 37 Igor Bukanov 2009-02-17 09:28:25 PST
(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] 2009-02-17 09:32:14 PST
Turns out someone had already documented most of this.  Finished it up.

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