916986 - AutoAssertNoGC

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

We need an RAII assertion that no gc happened within a scope, usable from both inside and outside spidermonkey.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Implement a JSAutoAssertNoGC for the analysis to pay attention to

I don't know if this should be JS::AutoAssertNoGC or what.

Comment 2

[Terrence Cole [:terrence]]

Comment on attachment 805562 [details] [diff] [review]
Implement a JSAutoAssertNoGC for the analysis to pay attention to

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

::: js/src/jsapi.h
@@ +4270,5 @@
>  #endif
>  
> +#ifdef DEBUG
> +/*
> + * Get the current "gcNumber", which is the number of GCs that have occurred.

If we absolutely need to expose this (and I don't think we do: see below), we should document it as a UID identifying a specific GC. The extra semantics lose most of their meaning when we start doing major and minor GCs on the same counter.

@@ +4276,5 @@
> +extern JS_PUBLIC_API(size_t)
> +JS_GetGCNumber();
> +#endif
> +
> +class JSAutoAssertNoGC {

Yes, namespaced as JS::AutoAssertNoGC would be better.

Also, move it to js/public/GCAPI.h.

@@ +4289,5 @@
> +#endif
> +    }
> +    ~JSAutoAssertNoGC() {
> +#ifdef DEBUG
> +        MOZ_ASSERT(gcNumber == JS_GetGCNumber(), "GC called within an AutoAssertNoGC scope");

Why are we not doing this like we did before: set a flag/counter on the runtime (or the shadow, if you want it inlined in the API), then assert that it is not set in MinorGC and Collect.

Comment 3

[Steve Fink [:sfink] [:s:]]

(In reply to Terrence Cole [:terrence] from comment #2)
> Comment on attachment 805562 [details] [diff] [review]
> Implement a JSAutoAssertNoGC for the analysis to pay attention to
> 
> Review of attachment 805562 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jsapi.h
> @@ +4270,5 @@
> >  #endif
> >  
> > +#ifdef DEBUG
> > +/*
> > + * Get the current "gcNumber", which is the number of GCs that have occurred.
> 
> If we absolutely need to expose this (and I don't think we do: see below),
> we should document it as a UID identifying a specific GC. The extra
> semantics lose most of their meaning when we start doing major and minor GCs
> on the same counter.

Yes, that's true. JS_GetGCUID or JS_GetGCToken or JS_GetGCGeneration or something does seem better. (I only added this after I realized I would need to call it from Gecko. I was originally thinking of it as a JS-internal thing.)

> @@ +4276,5 @@
> > +extern JS_PUBLIC_API(size_t)
> > +JS_GetGCNumber();
> > +#endif
> > +
> > +class JSAutoAssertNoGC {
> 
> Yes, namespaced as JS::AutoAssertNoGC would be better.
> 
> Also, move it to js/public/GCAPI.h.

Aha! Yes, that would be a better place for it.

> @@ +4289,5 @@
> > +#endif
> > +    }
> > +    ~JSAutoAssertNoGC() {
> > +#ifdef DEBUG
> > +        MOZ_ASSERT(gcNumber == JS_GetGCNumber(), "GC called within an AutoAssertNoGC scope");
> 
> Why are we not doing this like we did before: set a flag/counter on the
> runtime (or the shadow, if you want it inlined in the API), then assert that
> it is not set in MinorGC and Collect.

I can't say I put a lot of thought into it, but (1) this seemed to be the simplest implementation on first thought since it doesn't modify JSRuntime or whatever, and (2) using a flag/count means that you have to be sure that you catch all GC entry points (currently MinorGC and Collect, and I would've only known about MinorGC because I added it to the analysis.) Counters might need to be threadsafe and you'd need to convince yourself they never wrap etc. This way, only reading rt->gcNumber needs to be atomic. So this just seemed simpler. But the whole thing is pretty minor, so I can do it that way if you'd prefer.

Comment 4

[Terrence Cole [:terrence]]

(In reply to Steve Fink [:sfink] from comment #3)
> (In reply to Terrence Cole [:terrence] from comment #2)
> > Comment on attachment 805562 [details] [diff] [review]
> > Implement a JSAutoAssertNoGC for the analysis to pay attention to
> > 
> > Review of attachment 805562 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: js/src/jsapi.h
> > @@ +4270,5 @@
> > >  #endif
> > >  
> > > +#ifdef DEBUG
> > > +/*
> > > + * Get the current "gcNumber", which is the number of GCs that have occurred.
> > 
> > If we absolutely need to expose this (and I don't think we do: see below),
> > we should document it as a UID identifying a specific GC. The extra
> > semantics lose most of their meaning when we start doing major and minor GCs
> > on the same counter.
> 
> Yes, that's true. JS_GetGCUID or JS_GetGCToken or JS_GetGCGeneration or
> something does seem better. (I only added this after I realized I would need
> to call it from Gecko. I was originally thinking of it as a JS-internal
> thing.)

Actually, if we're going to namespace this, we might as well put it in namespace JS::shadow:: as whatever we want. I think we should expose this as JS::shadow::GetGCNumber and leave it undocumented.
 
> > @@ +4289,5 @@
> > > +#endif
> > > +    }
> > > +    ~JSAutoAssertNoGC() {
> > > +#ifdef DEBUG
> > > +        MOZ_ASSERT(gcNumber == JS_GetGCNumber(), "GC called within an AutoAssertNoGC scope");
> > 
> > Why are we not doing this like we did before: set a flag/counter on the
> > runtime (or the shadow, if you want it inlined in the API), then assert that
> > it is not set in MinorGC and Collect.
> 
> I can't say I put a lot of thought into it, but (1) this seemed to be the
> simplest implementation on first thought since it doesn't modify JSRuntime

Good point: that's never fun.

> or whatever, and (2) using a flag/count means that you have to be sure that
> you catch all GC entry points (currently MinorGC and Collect, and I would've
> only known about MinorGC because I added it to the analysis.)

Likewise, we have to remember to update gcNumber in the right place. The first few drafts of GGC were missing this.

> Counters might
> need to be threadsafe and you'd need to convince yourself they never wrap
> etc. This way, only reading rt->gcNumber needs to be atomic. So this just
> seemed simpler. But the whole thing is pretty minor, so I can do it that way
> if you'd prefer.

I don't think we even need these to be atomic, really. In any case, you've convinced me that this approach is superior.

Comment 5

[Steve Fink [:sfink] [:s:]]

Attachment: Implement a JSAutoAssertNoGC for the analysis to pay attention to

(In reply to Terrence Cole [:terrence] from comment #4)
> (In reply to Steve Fink [:sfink] from comment #3)
> > Counters might
> > need to be threadsafe and you'd need to convince yourself they never wrap
> > etc. This way, only reading rt->gcNumber needs to be atomic. So this just
> > seemed simpler. But the whole thing is pretty minor, so I can do it that way
> > if you'd prefer.
> 
> I don't think we even need these to be atomic, really.

Yeah, I don't either. I just felt guilty claiming that the flag/count requires atomicity and yet this doesn't. I'm not 100% confident of the latter. Although as long as it's 32-bit, it seems fine.

> In any case, you've convinced me that this approach is superior.

Wait, don't give in yet! I totally forgot about the coup de grâce -- with this approach, the assert happens in the relevant AutoAssertNoGC scope. With the flag/counter approach, you have to walk up the stack to find the problematic enclosing AutoAssertNoGC scope (and I've had multiple in the past, which resulted in some lost time in thinking through the wrong thing. I suppose I should've noticed the count was greater than 1...)

> > or whatever, and (2) using a flag/count means that you have to be sure that
> > you catch all GC entry points (currently MinorGC and Collect, and I would've
> > only known about MinorGC because I added it to the analysis.)
> 
> Likewise, we have to remember to update gcNumber in the right place. The
> first few drafts of GGC were missing this.

Oh, good point. That makes me feel, once again, that we ought to split up gcNumber. gcStuffMightHaveMovedNumber (minor GC + major GC + compacting GC), gcLikelyBigPauseNumber (major GC only?), etc. Please don't use those names. We'll probably be playing enough games with different flavors of GC that we ought to say why we're asking. But I guess it isn't needed yet.

Comment 6

[Terrence Cole [:terrence]]

Comment on attachment 805646 [details] [diff] [review]
Implement a JSAutoAssertNoGC for the analysis to pay attention to

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

r=me

::: js/public/GCAPI.h
@@ +208,5 @@
>  extern JS_FRIEND_API(bool)
>  WasIncrementalGC(JSRuntime *rt);
>  
> +extern JS_FRIEND_API(size_t)
> +GetGCNumber();

Don't forget to put this in namespace shadow.

::: js/src/jsfriendapi.cpp
@@ +836,5 @@
> +    JSRuntime *rt = js::TlsPerThreadData.get()->runtimeFromMainThread();
> +    if (!rt)
> +        return 0;
> +    return rt->gcNumber;
> +}

I think a better place for this implementation would be either vm/Runtime.cpp, it is an accessor on the runtime after all, or jsgc.cpp, as it is defined in GCAPI.h.

Comment 7

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/e626543f0a79

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e626543f0a79