The default bug view has changed. See this FAQ.

make tooMuchGC dynamic (runtime gczeal option)

VERIFIED FIXED

Status

()

Core
JavaScript Engine
--
enhancement
VERIFIED FIXED
12 years ago
8 years ago

People

(Reporter: timeless, Assigned: Brian Crowder)

Tracking

({dev-doc-complete, verified1.8.1.15})

Trunk
x86
Windows XP
dev-doc-complete, verified1.8.1.15
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1.15 +
wanted1.8.1.x +
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:want P4])

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

12 years ago
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.
(Reporter)

Comment 1

12 years ago
Created attachment 196015 [details] [diff] [review]
draft
Attachment #196015 - Flags: review?(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
Attachment #196015 - Flags: review?(brendan) → review-
Or, if we want this for DEBUG, let's get rid of the other ifdefs and just use
DEBUG for compile-time conditioning.

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

/be

Updated

12 years ago
Flags: testcase-

Updated

10 years ago
Whiteboard: [sg:want P4]
(Assignee)

Comment 5

10 years ago
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?
Assignee: general → crowder
Attachment #196015 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #260420 - Flags: review?(igor)

Comment 6

10 years ago
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

10 years ago
(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.
(Assignee)

Comment 8

10 years ago
Created attachment 260427 [details] [diff] [review]
with igor's feedback

Great feedback, thanks.  How about 0xdddddddd as the "bogus object ptr" for freed slots?
Attachment #260420 - Attachment is obsolete: true
Attachment #260427 - Flags: review?(igor)
Attachment #260420 - Flags: review?(igor)

Comment 9

10 years ago
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.
(Assignee)

Comment 10

10 years ago
Created attachment 260430 [details] [diff] [review]
64-bit bogus jsval

I have no way of trying this out in a 64-bit environment.
Attachment #260427 - Attachment is obsolete: true
Attachment #260430 - Flags: review?(igor)
Attachment #260427 - Flags: review?(igor)

Updated

10 years ago
Attachment #260430 - Flags: review?(igor) → review+
(Assignee)

Comment 11

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

Brendan:  how do you feel about something like this?
Attachment #260430 - Flags: superreview?(brendan)
(Assignee)

Comment 12

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

Updated

10 years ago
Summary: make tooMuchGC dynamic → make tooMuchGC dynamic (runtime gcvigor option)
(Assignee)

Comment 13

10 years ago
brendan: ping?
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
Attachment #260430 - Flags: superreview?(brendan) → superreview-
(Assignee)

Comment 15

10 years ago
Created attachment 263689 [details] [diff] [review]
addressing review issues

Thanks, Brendan, this should address everything.
Attachment #260430 - Attachment is obsolete: true
Attachment #263689 - Flags: review?(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
(Assignee)

Comment 17

10 years ago
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
Attachment #263689 - Attachment is obsolete: true
Attachment #264931 - Flags: review+
Attachment #263689 - Flags: review?(brendan)
(Assignee)

Comment 18

10 years ago
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
Summary: make tooMuchGC dynamic (runtime gcvigor option) → make tooMuchGC dynamic (runtime gczeal option)

Updated

10 years ago
Depends on: 380833
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?
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
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
Attachment #283647 - Flags: superreview?(jst)
Attachment #283647 - Flags: review?(jst)

Updated

10 years ago
Attachment #283647 - Flags: superreview?(jst)
Attachment #283647 - Flags: superreview+
Attachment #283647 - Flags: review?(jst)
Attachment #283647 - Flags: review+
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.
Attachment #283647 - Flags: approval1.9?
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
Depends on: 401188

Comment 23

10 years ago
(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?
(Assignee)

Comment 24

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

Comment 25

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

clearing the approval flag based on the last comment
Attachment #283647 - Flags: approval1.9?
(Assignee)

Comment 26

10 years ago
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).
(Assignee)

Updated

10 years ago
Attachment #283647 - Flags: approval1.9?

Updated

10 years ago
Attachment #283647 - Flags: approval1.9? → approval1.9+
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
Attachment 283647 [details] [diff] now landed on trunk. I believe this is now FIXED.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: wanted1.8.1.x-
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...
It would also be great if there were some documentation for this (e.g. in all.js).
Keywords: dev-doc-needed
(Assignee)

Updated

9 years ago
Depends on: 414982
It's looking like we want this on the 1.8 branch in order to better test for and prevent regressions.
Flags: wanted1.8.1.x-
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.15?
Blocks: 426628
Flags: blocking1.8.1.15? → blocking1.8.1.15+
Depends on: 427185
(Assignee)

Updated

9 years ago
No longer depends on: 427185
(Assignee)

Comment 31

9 years ago
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.
(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.
(Assignee)

Updated

9 years ago
Keywords: fixed1.8.1.15

Comment 33

9 years ago
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.
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1.15 → verified1.8.1.15
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?
(Assignee)

Comment 35

8 years ago
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.
(Assignee)

Comment 36

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

Comment 37

8 years ago
(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.
Turns out someone had already documented most of this.  Finished it up.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.