437865 - Enable JS_GC_ZEAL by default

Status: RESOLVED WONTFIX

(Core :: JavaScript: GC, enhancement)

Description

[Havoc Pennington]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b5) Gecko/2008050509 Firefox/3.0b5
Build Identifier: 

For applications using SpiderMonkey to test for proper rooting, they really need JS_SetGCZeal().

Right now Linux distributions are compiling xulrunner without it though, so to  enable gc zeal in "make check" everyone has to build their own SpiderMonkey, which is pretty painful.

It looks like the runtime overhead of this feature is limited to:

#ifdef JS_GC_ZEAL
    jsrefcount          gcZeal;
#endif

#ifdef JS_GC_ZEAL
    doGC = doGC || rt->gcZeal >= 2 || (rt->gcZeal >= 1 && rt->gcPoke);
#endif

Which seems negligible.

It would also be helpful, even if JS_GC_ZEAL is not set at compile-time, to define JS_SetGCZeal() anyway but make it a no-op; right now applications have to do something like check in configure whether JS_SetGCZeal is present and then #ifdef the use of it. It would be easier to just have JS_SetGCZeal always present but sometimes not work, which is what the configure check ends up amounting to anyway.


Reproducible: Always

Comment 1

[Mike Shaver (:shaver emeritus)]

We'll be making building spidermonkey less painful via autoconfery, but I agree that we should just strip out the #ifdefs there (it would also simplify the code in Gecko that conditionally twiddles gcZeal).

Wanna whip up a patch vs hg trunk?

Comment 2

[Havoc Pennington]

When you say "hg trunk" what do you mean? Is that hg.mozilla.org/mozilla-central ?

The patch you want is just "remove all #ifdef JS_GC_ZEAL" right?

(as a side note, the build pain I meant is just "not using the system package," not any particular aspect of the build process, though making that nicer is welcome too of course.)

Comment 3

[Jesse Ruderman]

Who builds applications that interface with the JS GC rooting API, but can't easily build Spidermonkey themselves?

This code runs on every JS allocation, which I imagine is quite often.

Comment 4

[Brendan Eich [:brendan]]

We're working on speeding up allocations, toward the mythical HotSpot JVM three instruction limit with inlining and copying GC. Jesse's poing is more acute over time, not less. TOO_MUCH_GC or JS_GC_ZEAL has always required recompiling, which is not onerous considering the development challenges of exact GC (static checker under construction). Suggest WONTFIX.

/be

Comment 5

[Havoc Pennington]

All apps using SpiderMonkey interface with the GC rooting API, pretty much. If you allocate more than one JavaScript object in a function you can't rely on the newborn roots feature. And if you want to effectively test rooting in your unit tests (or app itself), you need the GC zeal, or the bugs almost never appear, certainly not in a reproducible way.

On Linux most people develop apps vs. distribution packages, they don't build every library from source. There's no reason to build from source unless you need to modify the library. Distribution packages even come with debug symbols and source code that gdb automatically finds, these days.

Even if the developers build the library from source, the app needs to compile and ideally run its test suite against the distribution package, since that's how the consumers of the app would run it and run the tests. For example if an app ships with a distribution, it would be best practice for the distribution vendor to run "make check" while creating the application package.

In my app's case, there's the additional issue that the SpiderMonkey-using code runs both inside xulrunner and outside, so we need to use exactly the libmozjs used by the system copy of xulrunner, we can't suck in an internal copy of libmozjs.

Looking at js_NewGCThing(), it's doing enough work that I don't think the test for rt->gcZeal could possibly show up in a profile, even one that just ran js_NewGCThing() in a tight loop. If it did, I can come up with ways to optimize the #ifdef'd line, but I think it's probably premature to start doing that.

Comment 6

[Havoc Pennington]

To add on a bit in light of Brendan's comment, the issue here is to have 'make check' (i.e. the unit test suite) standardly and always use SetGCZeal(), which is a bit different from a developer building spidermonkey one time to track down a GC problem. The issue isn't that I personally don't feel like building spidermonkey one time, rather there's a qualitative difference between a developer occasionally checking for GC root problems by hand, and having it automatically checked whenever anyone on the team patches the code.

js_NewGCThing() is nowhere remotely near three instructions at the moment ... and in a JIT you'd have other ways to implement this that would avoid a check, surely.

fwiw I think it's fine to make this API a weak guarantee or "hint", and break it when the GC is no longer a simple mark-and-sweep thing. It would also be fine to require calling SetGCZeal() before using the runtime and not changing it thereafter. For that matter I think it's fine if this isn't an API at all but just an environment variable, like JS_DEBUG_GC=2 or something.

In a JIT type of scenario, surely you could generate code at runtime that includes or does not include the GC zealousness.

Worst case, the check is "if (__builtin_expect(rt->gcZeal, 0))" - I'm no compiler guy, but I'm pretty sure that's tough to get at the top of your profile, if gcZeal is set to 0.

In any case, even if not turning it on by default, conditional symbol presence is a lot of extra typing to deal with in an application, since it requires build boilerplate to create an #ifdef to use.

So, if JS_SetGCZeal() needs to leave js_NewGCThing() unaffected by default for performance reasons, it would still be nice to only #ifdef the *body* of JS_SetGCZeal() instead of removing the symbol from header and library.

All details aside the bottom line is that this is fairly important API to have for an app using SpiderMonkey so some solution would be welcome.

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #6)
> To add on a bit in light of Brendan's comment, the issue here is to have 'make
> check' (i.e. the unit test suite) standardly and always use SetGCZeal(), which
> is a bit different from a developer building spidermonkey one time to track
> down a GC problem.

'make check' can compile with -DJS_GC_ZEAL then. (Better yet, the build system can run the static checker for GC exact rooting safety.)

> The issue isn't that I personally don't feel like building
> spidermonkey one time, rather there's a qualitative difference between a
> developer occasionally checking for GC root problems by hand, and having it
> automatically checked whenever anyone on the team patches the code.

Developer requirements need not affect release builds. Assertions compile out of release code, for good reason.

> js_NewGCThing() is nowhere remotely near three instructions at the moment ...
> and in a JIT you'd have other ways to implement this that would avoid a check,
> surely.

Surely -- but then:

> fwiw I think it's fine to make this API a weak guarantee or "hint", and break
> it when the GC is no longer a simple mark-and-sweep thing.

1. Even though unifdef'ed, the JS_SetGCZeal API could become meaningless over time and it would be fine.

> In any case, even if not turning it on by default, conditional symbol presence
> is a lot of extra typing to deal with in an application, since it requires
> build boilerplate to create an #ifdef to use.

Build boilerplate is trivially cheap compared to the death by a thousand cuts of special-pleading such as this bug.

> So, if JS_SetGCZeal() needs to leave js_NewGCThing() unaffected by default for
> performance reasons, it would still be nice to only #ifdef the *body* of
> JS_SetGCZeal() instead of removing the symbol from header and library.

Sure, that is a great suggestion, and how other APIs such as JS_BeginRequest etc. are done.

> All details aside the bottom line is that this is fairly important API to have
> for an app using SpiderMonkey so some solution would be welcome.

1. The unifdef'ed JS_SetGCZeal API could become meaningless over time.
2. The JS_SetGCZeal API is important.

These are not consistent. I'd rather we make the API meaningful unconditionally (independent of compile-time decisions). We should argue about how to make the test cheaper, but I don't see the point in making the API a "weak guarantee".

/be

Comment 8

[Havoc Pennington]

> 1. The unifdef'ed JS_SetGCZeal API could become meaningless over time.
> 2. The JS_SetGCZeal API is important.
>
> These are not consistent.

What I'm getting at here is that there may be better solutions such as the static checker in the future that obsolete the functionality. But at the moment there are not other solutions that I know of, and the maximum performance cost for now (with zeal disabled) is one correctly-branch-predicted test for 0. In fact that's the maximum cost of *all* developer/debug features one could invent (since you could keep one flag indicating whether any of them are enabled, and look at specifics only if any are). Which avoids the "slippery slope" phenomenon.

Offering the API makes sense now because the performance overhead is unlikely to even be possible to show in a profile, and it either makes sense in the future because the JIT can nuke the overhead, or it makes sense to reduce to no-op in the future because of a static checker replacing it, or ideally both are options; a static checker and a JIT would both be welcome.

> Developer requirements need not affect release builds. Assertions compile out
> of release code, for good reason.

The C library for example supports the MALLOC_CHECK_ environment variable which is quite comparable to the GC zeal feature, and those guys are performance fanatics. GLib supports similar things (also via environment variable).

It's well-precedented to have developer features in the release build when the feature is very useful and the performance cost is reasonable. It is common and typical for developers to use release builds and SDKs rather than building all libs from scratch.

Comment 9

[Mike Shaver (:shaver emeritus)]

I think that you want "make check" to run on what you're going to ship, so that you don't miss side-effect-in-ASSERT and other such bug habitat in your testing.  In a JIT environment we'd obviously look at ways to optimize away that branch, but I think the utility of SetGCZeal is high enough that we should take it for now.  People are building extensions and other apps that use the JSAPI, and making them rebuild all of Firefox or XULRunner with JS_GC_ZEAL (once they figure out how to do that) seems excessive.

Comment 10

[Brendan Eich [:brendan]]

Too many words in comment 8.... Pointing to precedent such as MALLOC_CHECK_ to rebut precedent such as assert.h is not going to get us anywhere. I will continue to push back on the notion that developers should necessarily be catered to by the release version of a shared library. It ain't necessarily so. 

That aside, the good news is that everyone agrees that JS_SetGCZeal should be unconditionally defined as an exported API function. The simplest patch for this bug would just unifdef and worry about optimizing down the road. Someone please write the patch and save us more tendentious word wars.

/be

Comment 11

[Havoc Pennington]

Sounds good. Getting back to comment 2, what goes after "hg clone ..." so I have the appropriate tree to patch against?

If it's appropriate I can also update the docs wiki once the patch is incorporated
(meaning http://developer.mozilla.org/en/docs/JS_SetGCZeal)

Comment 12

[Jason Orendorff [:jorendorff]]

(In reply to comment #11)
> Sounds good. Getting back to comment 2, what goes after "hg clone ..." so I
> have the appropriate tree to patch against?

Nothing, unless you mean:  cd mozilla-central/js/src

Comment 13

[Havoc Pennington]

I meant "which branch do I clone," sorry for asking it in a confusing way. mozilla-central it is.

Comment 14

[Havoc Pennington]

Attachment: patch removing ifdef entirely

In addition to removing the ifdef, slightly rearranged the code to decrease instructions executed on the zeal == 0 codepath, here is the relevant change:

-    doGC = rt->gcMallocBytes >= rt->gcMaxMallocBytes && rt->gcPoke;
-#ifdef JS_GC_ZEAL
-    doGC = doGC || rt->gcZeal >= 2 || (rt->gcZeal >= 1 && rt->gcPoke);
-#endif
+    if (rt->gcZeal == 0) {
+        doGC = (rt->gcMallocBytes >= rt->gcMaxMallocBytes && rt->gcPoke);
+    } else if (rt->gcZeal == 1) {
+        doGC = rt->gcPoke;
+    } else {
+        doGC = JS_TRUE;
+    }

Comment 15

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Also ref bug 1514346, do we want to consider turning on gczeal by default anyway?

Comment 16

[Nicolas B. Pierron [:nbp]]

I presume that based on the cost of zealous mode of GC, I presume this is unlikely to be the case.
Jon, Steve feel free to correct me and to reopen this bug.