848199 - GC: rename the verifier nursery

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Attachment: v0

These changes will allow the verifier to co-exist with the real nursery.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 721548 [details] [diff] [review]
v0

Review of attachment 721548 [details] [diff] [review]:
-----------------------------------------------------------------

Could you explain this patch a little better? I'm having trouble understanding everything that's going on.

::: js/src/jsobjinlines.h
@@ +746,5 @@
>  /* static */ inline bool
>  JSObject::setSingletonType(JSContext *cx, js::HandleObject obj)
>  {
>  #if defined(JSGC_GENERATIONAL)
> +    JS_ASSERT_IF(cx->runtime->gcVerifyPostData, !cx->runtime->gcVerifierNursery.isInside(obj.get()));

It seems like you could use some sort of generic IsInside function that works for either nursery, depending on which one is in use. It could just be a global thing that would take a runtime and an object.

::: js/src/jstypedarrayinlines.h
@@ +204,5 @@
>       * on private Values.
>       */
>      obj->initPrivate(buffer->dataPointer() + byteOffset);
>  #ifdef JSGC_GENERATIONAL
> +    if (obj->runtime()->gcVerifierNursery.isInside(buffer))

If we're not using the verifier nursery, won't this skip the barrier entirely?

::: js/src/jswatchpoint.cpp
@@ +53,5 @@
>  WatchpointWriteBarrierPost(JSRuntime *rt, WatchpointMap::Map *map, const WatchKey &key,
>                             const Watchpoint &val)
>  {
>  #ifdef JSGC_GENERATIONAL
> +    if (rt->gcVerifyPostData) {

Why only invoke the write barrier when running the verifier?

Comment 2

[Terrence Cole [:terrence]]

(In reply to Bill McCloskey (:billm) from comment #1)
> Could you explain this patch a little better? I'm having trouble
> understanding everything that's going on.

Yes, absolutely. There are 4-8 relevant configurations we want to support, depending on how you count it: --{en,dis}able-gczeal, --{en,dis}able--gcgenerational, and --ggc togglable on the CLI. The tricky bit here is that we cannot use our real nursery to verify the post barriers: it is impossible to do a MinorGC if we don't trust the validity of the store buffer. Thus, we need to keep the HashSet that tracks the "this object is in the nursery" state and we need to be able to transition from using our real nursery to using the fake nursery at runtime when we enable post-barrier verification in the shell.

This patch does (or at least attempts to do) 4 things:

1) Currently the fake it is called |Nursery gcNursery;|. I don't really want to make |RealNursery gcRealNursery;|, I'd rather rename the fake nursery to |VerifierNursery gcVerifierNursery| and re-use the |gcNursery| name for the real |Nursery|. This patch does this straightforward renaming.

2) When we go to compact the store buffer, we need to know what is in the Nursery. Which nursery is active will change where and how we need to check. This patch templatizes the relevant algorithms on |typename NurseryType| where required.
 
3) Support the verifier and real nursery in barriers where this matters. Most barriers will just continue putting their data in the store buffer regardless of the current configuration/mode. In some of our generic barriers, however, we check the nursery at call time rather than stuffing unnecessary stuff in the slow generic buffer. When we do this, it should usually look like some variation of:

#ifdef JS_GC_ZEAL
if (rt->gcVerifyPostData) { // Currently verifying.
    // Use rt->gcVerifierNursery.
    return;
}
#endif
#ifdef JS_GC_GENERATIONAL
// Use rt->gcNursery: currently will be empty since we don't have a real Nursery yet.
#endif

4) The last thing this patch is doing -- in gc/Verifier.cpp -- is fairly subtle. Specifically, the code here attempts to support the state transition between the real and fake nurseries when entering and leaving the pre-barrier and post-barrier verifiers. Entering involves checking if we are already verifying, doing a minor collection if gcNursery is enabled, turning off the nursery, then turning on the verifier nursery. Exiting is similar but we additionally need to check if gcGenerationalEnabled before turning the real nursery back on.

I initially tried to minimize the number of state transitions: this proved to be extremely tricky. One example of a bug I hit was StartVerifyPreBarriers would disable gcNursery, but wouldn't touch gcVerifierNursery, then StartVerifyPostBarriers would enable gcVerifierNursery, then EndVerifyPostBarriers would re-enable both gcNursery and gcVerifierNursery, breaking the pre-barrier verifier. In fixing all of the bugs I ended up with a total disaster. It ended up requiring impossible-to-understand state checking pretty much everywhere and it was a huge amount of code as well.

Instead of asking for review on that, I rewrote it to the state it's currently in, sans ~10 lines managing gcNursery in {En,Dis}ableGGCForVerification. Now there are exactly two states we can be in and only one piece of code that handles each state transition. That code is called at the start and end of {Start,End}Verify{Pre,Post}Barriers respectively. The states are:

  Inside verification:
    gcNursery=disabled
    gcVerifierNursery=enabled
    gcStoreBuffer=enabled
  Outside verification:
    gcNursery=gcGenerationalEnable
    gcVerifierNursery=disabled
    gcStoreBuffer=enabled

> ::: js/src/jsobjinlines.h
> @@ +746,5 @@
> >  /* static */ inline bool
> >  JSObject::setSingletonType(JSContext *cx, js::HandleObject obj)
> >  {
> >  #if defined(JSGC_GENERATIONAL)
> > +    JS_ASSERT_IF(cx->runtime->gcVerifyPostData, !cx->runtime->gcVerifierNursery.isInside(obj.get()));
> 
> It seems like you could use some sort of generic IsInside function that
> works for either nursery, depending on which one is in use. It could just be
> a global thing that would take a runtime and an object.

That's an excellent idea -- I can probably make it solve the bug you noted in watchpoint as well.

> ::: js/src/jstypedarrayinlines.h
> @@ +204,5 @@
> >       * on private Values.
> >       */
> >      obj->initPrivate(buffer->dataPointer() + byteOffset);
> >  #ifdef JSGC_GENERATIONAL
> > +    if (obj->runtime()->gcVerifierNursery.isInside(buffer))
> 
> If we're not using the verifier nursery, won't this skip the barrier
> entirely?
>
> ::: js/src/jswatchpoint.cpp
> @@ +53,5 @@
> >  WatchpointWriteBarrierPost(JSRuntime *rt, WatchpointMap::Map *map, const WatchKey &key,
> >                             const Watchpoint &val)
> >  {
> >  #ifdef JSGC_GENERATIONAL
> > +    if (rt->gcVerifyPostData) {
> 
> Why only invoke the write barrier when running the verifier?

Uhg, yes, thanks for spotting this! Both of these bugs were introduced by me when pulling these bits out of the giant Nursery patch. I apparently re-tested the verifier with the nursery still pushed. Sorry, I'll make sure and test with the right configuration this time and push a patch which actually passes.

Comment 3

[Terrence Cole [:terrence]]

Attachment: v1

(In reply to Terrence Cole [:terrence] from comment #2)
> (In reply to Bill McCloskey (:billm) from comment #1)
> >
> > Why only invoke the write barrier when running the verifier?
> 
> Uhg, yes, thanks for spotting this! Both of these bugs were introduced by me
> when pulling these bits out of the giant Nursery patch. I apparently
> re-tested the verifier with the nursery still pushed. Sorry, I'll make sure
> and test with the right configuration this time and push a patch which
> actually passes.

Yikes, nevermind. We do pass the post-verifier with this: we only need the store buffer entries to be present when verifying. I have hunks in the Nursery patch to make them work correctly with ggc. However, with a generic IsInsideNursery we don't need any modifications when we add the Nursery.

This a nice cleanup, thanks for the help!

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Review of attachment 722923 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. By the way, I've kinda come to the conclusion that single-character variable names are annoying (except maybe i). I often want to find all uses of a var inside a function, and if I search for all uses of |n| or |o| or whatever, I'll get a lot of matches inside longer names, which I don't want. In the future, can you use |nursery| instead of |n|?

::: js/src/gc/Verifier.cpp
@@ -28,5 @@
>  using namespace js::gc;
>  using namespace mozilla;
>  
>  #if defined(DEBUG) && defined(JS_GC_ZEAL) && defined(JSGC_ROOT_ANALYSIS) && !defined(JS_THREADSAFE)
> -

If you take out this line, you should take out the one before the #endif as well.

@@ +312,5 @@
>  
>  /*
> + * Pre barrier and post barrier verifications done through the API can overlap
> + * with a different verifier running through the environment variable. Thus,
> + * we need to carefully check our state when transitioning.

I'm not sure this comment contributes much. Is there anything in particular that we need to be careful about?

@@ +333,5 @@
> +#ifdef JSGC_GENERATIONAL
> +    if (rt->gcVerifyPreData || rt->gcVerifyPostData)
> +        return;
> +
> +    if (rt->gcGenerationalEnabled) {

No braces.

@@ +859,5 @@
>      MaybeVerifyPreBarriers(cx->runtime, always);
>      MaybeVerifyPostBarriers(cx->runtime, always);
>  }
>  
> +

Did you mean to put in this extra line?

@@ +865,5 @@
>  js::gc::FinishVerifier(JSRuntime *rt)
>  {
>      if (VerifyPreTracer *trc = (VerifyPreTracer *)rt->gcVerifyPreData) {
> +        js_delete(trc);
> +        rt->gcVerifyPreData = NULL;

Oops. Nice catch.

Comment 5

[Terrence Cole [:terrence]]

(In reply to Bill McCloskey (:billm) from comment #4)
> Comment on attachment 722923 [details] [diff] [review]
> v1
> 
> Review of attachment 722923 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. By the way, I've kinda come to the conclusion that
> single-character variable names are annoying (except maybe i). I often want
> to find all uses of a var inside a function, and if I search for all uses of
> |n| or |o| or whatever, I'll get a lot of matches inside longer names, which
> I don't want. In the future, can you use |nursery| instead of |n|?

I renamed |n| to |nursery|. In vim you can use \< and \> to match on the begin and end of a word, respectively, so fixing these was not hard.
 
> ::: js/src/gc/Verifier.cpp
> @@ +312,5 @@
> >  
> >  /*
> > + * Pre barrier and post barrier verifications done through the API can overlap
> > + * with a different verifier running through the environment variable. Thus,
> > + * we need to carefully check our state when transitioning.
> 
> I'm not sure this comment contributes much. Is there anything in particular
> that we need to be careful about?

Well, I wrote the comment before the function. I guess the refactor was a success. :-)
 
> @@ +859,5 @@
> >      MaybeVerifyPreBarriers(cx->runtime, always);
> >      MaybeVerifyPostBarriers(cx->runtime, always);
> >  }
> >  
> > +
> 
> Did you mean to put in this extra line?

Nope! Thanks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d65f437c771

Comment 6

[Nicholas Nethercote [inactive]]

> > Looks good. By the way, I've kinda come to the conclusion that
> > single-character variable names are annoying (except maybe i). I often want
> > to find all uses of a var inside a function, and if I search for all uses of
> > |n| or |o| or whatever, I'll get a lot of matches inside longer names, which
> > I don't want. In the future, can you use |nursery| instead of |n|?
> 
> I renamed |n| to |nursery|. In vim you can use \< and \> to match on the
> begin and end of a word, respectively, so fixing these was not hard.

vim also has the '*' and '#' searches, which make it even easier.

Some regexp-y things don't support \< and \>, but you can use \b at the start and end instead.

This is basic regexp stuff and therefore a bad reason to avoid single-char names, IMO.

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8d65f437c771