Closed Bug 794667 Opened 7 years ago Closed 7 years ago

GC: implement an AutoAssertCanGC guard object

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: terrence, Assigned: terrence)

References

Details

(Whiteboard: [js:t])

Attachments

(1 file, 3 obsolete files)

Currently, it is very hard to know, even by inspection, if a function can GC.  To find this out, one must inspect all possible call paths under a function.  For awhile, we have been adding MaybeCheckStackRoots to assert that we are not in an AutoAssertNoGC zone, but this is quite expensive and not clear to non GC people.

Instead we should add an AutoAssertCanGC guard that does the same check as MaybeCheckStackRoots, but without the other work.  By adding an AutoAssertNoGC or AutoAssertCanGC guard to the top of every method in SpiderMonkey, we will be able to very quickly figure out what regions of SpiderMonkey code are actually safe from a GC perspective (rather than guessing, as we are frequently forced to do now) and at the same time annotate with that knowledge to make it harder to make GC-unsafe changes.

Getting these guards into all methods should not take long to implement in practice because each time we annotate a method, we make it that much easier to annotate all callers of that method.  However, this work is only to implement the guard and augment MaybeCheckStackRoots.
Ideally we'd be able to tell from a callsite whether the called function can GC, without having to inspect its code.  The presence/lack of a |cx| argument currently is a reasonable indication of this... at least, the lack of a |cx| argument almost guarantees non-GC-triggerability.
Yeah, it's a shame we need to bury the [100% certain] info under the covers, but I don't see any good way to put it in the signature.  I think holding off on doing this right now will put us on the wrong side of "perfect is the enemy of good."
Attachment #665673 - Flags: review?(wmccloskey)
I just spent quite a lot of time experimenting with splitting JSContext into two classes:  JSConNoGC and a sub-class JSContext.  The idea is that functions which can trigger GC would take a JSContext, and those that cannot would take a JSConNoGC (and something similar for JSRuntime).

I didn't end up finishing it because it was a huge number of changes, but from the parts I did I concluded that something like 90% of the functions that take a JSContext can trigger a GC.  So I JSContext is a pretty good indicator.

Having experimented with this, I think adding an assertion to the top of every method in SpiderMonkey is impractical.  There are a *lot* of them.

What I would like to see instead is a guarantee that a function which doesn't take a JSContext (or JSRuntime) arg, either explicitly or implicitly, *cannot* GC.  I think TlsRuntime is the one exception to that at the moment, though I don't think it's visible outside jsapi.cpp at the moment.
(In reply to Nicholas Nethercote [:njn] from comment #4)
> Having experimented with this, I think adding an assertion to the top of
> every method in SpiderMonkey is impractical.  There are a *lot* of them.

We don't need full coverage to get major benefits out of this.  With this patch it will give us equivalent benefits to the DEBUG asserts in MaybeCheckStackRoots.  Unlike MaybeCheckStackRoots, however, we can sprinkle these all over without making the rooting analysis slower.  I expect the benefits will grow as we get more coverage, but I don't think we need anything like full coverage to make it worthwhile.

I don't think this is necessarily orthogonal to the JSConNoGC work either.  I'd suggest making AutoAssertCanGC take a JSContext (and not a JSConNoGC), but we'd need the inverse coverage to make it useful.  Also, it would be nice to be able to remove the context and rely on the TlsRuntime, eventually.
 
> What I would like to see instead is a guarantee that a function which
> doesn't take a JSContext (or JSRuntime) arg, either explicitly or
> implicitly, *cannot* GC.  I think TlsRuntime is the one exception to that at
> the moment, though I don't think it's visible outside jsapi.cpp at the
> moment.

It was, briefly, when we had the empty constructor on Rooted, but I removed it a few days ago.
Bill suggested something interesting at lunch that I want to throw in here.  Many of the engine's simpler functions are fallible because they call JS_ReportError, presumably extremely rarely.  If we could add enough ballast to the context or runtime, we could make JS_ReportError infallible.  This might dramatically expand the number of infallible methods, and therefore dramatically reduce the amount of exact rooting we need to do.
Indeed, error reporting did cause me a lot of JSConNoGC-to-JSContext conversions.
> If we could add enough ballast
> to the context or runtime, we could make JS_ReportError infallible.

AFAICT the hardest part of this is that cx->errorReporter and cx->runtime->debugHooks.debugErrorHook can both be set via the API, and they both are pass |JSContext*| parameters.
Comment on attachment 665673 [details] [diff] [review]
v0: implement AutoAssertCanGC and use it in obvious places

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

::: js/src/gc/Root.h
@@ +585,5 @@
> +class AutoAssertCanGC
> +{
> +    MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER
> +
> +public:

public: should be indented two spaces.
Attachment #665673 - Flags: review?(wmccloskey) → review+
In-browser kraken regressed by >12%.  The prior tested commit is 447a46c78bac.  The pushloghtml [1] responds with "unknown revision '447a46c78bac'", so it looks like I'm the only one that got a regression notice for this.  If the regression does not go away tomorrow with this backed out, I'll forward the mail to all of the people with commits in this range and re-land.

[1] - http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=447a46c78bac&tochange=ed626654fe56
Also of note: this regression is WinXP /only/, not even Win7 is affected.
https://hg.mozilla.org/mozilla-central/rev/ed626654fe56
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch v1 (obsolete) — Splinter Review
I ran a few Try runs and this does actually appear to be the regressing patch: WinXP is Bizzaro Land.

I think it should be fine to just disable the assertion in WinXP.  Bill, is the method I'm using to disable the assertions acceptable?
Attachment #665673 - Attachment is obsolete: true
Attachment #667273 - Flags: review+
Attachment #667273 - Flags: feedback?(wmccloskey)
Why not use #ifdef DEBUG instead of #ifndef XP_WIN?
This is gross, but unregresses WinXP and suppresses the warnings.
Attachment #667273 - Attachment is obsolete: true
Attachment #667273 - Flags: feedback?(wmccloskey)
Attachment #668043 - Flags: review+
Attachment #668043 - Flags: feedback?(wmccloskey)
Comment on attachment 668043 [details] [diff] [review]
v2: with feedback and regression fixed

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

Can you try the function call approach?
Attachment #668043 - Flags: feedback?(wmccloskey)
Bill and I discussed this and just decided to go with a function for now.
Attachment #668043 - Attachment is obsolete: true
Attachment #668170 - Flags: review+
Putting AutoAssertNoGC in functions that don't take a |cx| or |rt| doesn't seem useful.  (Putting it in functions that do take a |cx| or |rt|, on the other hand, is very useful.)
(In reply to Nicholas Nethercote [:njn] from comment #23)
> Putting AutoAssertNoGC in functions that don't take a |cx| or |rt| doesn't
> seem useful.  (Putting it in functions that do take a |cx| or |rt|, on the
> other hand, is very useful.)

That's a good point. I agree.
https://hg.mozilla.org/mozilla-central/rev/049c17d2954c
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
I mostly agree.  However, there are lots of places in our code base that store the current context as part of their class or use it implicitly thorough something like fp.cx().  In places that do this, I think it makes sense to be more aggressive with our annotations.
You're right;  I was imprecise with my use of "take" in comment 23.  I meant that to include implicit accesses via class members and the like.  But AFAICT the examples you've added in this patch don't fit that pattern.
Ah, I see.  No, the ones I added here are the places where we have a MaybeCheckStackRoots and core choke-points for the GC: i.e. places I didn't have to think about if it was valid :-).  The patch I'm working on currently to add a Return<T> to HeapPtrScript accessors has many more ambiguities.  In particular, there are a bunch of places in IonMonkey where they carry around an implicit |cx| that doesn't get used directly, but gets used extensively in sub-calls, frequently in different files.  In those cases AssertNoGC() and AutoAssertNoGC were /extremely/ helpful in ensuring that all accesses to HeapPtrScript are safe.

This specifically ties into Return<T> in a couple ways.  I found that making Return<T> totally opaque until rooted was impractical.  First, We have very frequent uses of things like: |JS_ASSERT(cx->stackSomething.fp->script()->property == foo);|.  Adding an getter on top of script() in all the places where we do this would have been extremely ugly.  Furthermore, I'd guess about half of these happen in places where we cannot GC, so Rooting them would be a terrible solution anyway.

OTOH, we still want to minimize the potential for accidents.  What I've done is split the accessors into two categories: explicit through |operator->| and |get()| and implicit through |operator const T&|.  The explicit operators have a lighter assertion than the implicit (i.e. possibly accidental) access.  The explicit accessor assertion is that no GC happens while the JSScript* is on the stack.  The implicit accessor assertion is much stronger: we must be inside an AutoAssertNoGC block.

With this split available, I was able to save myself from needing to uglify a bunch of methods, just by dumping an AutoAssertNoGC at the top.  [In reality I also changed storage of these scripts to RawScript, but njn's recent patch pulled these out of my patch with no conflict, so thanks!]  I'm really liking how the Return<T> patch is shaping up and part of the reason for that success is that it is able to lean so heavily on the AutoAssertNoGC/AssertCanGC scope annotations.
Depends on: 804558
You need to log in before you can comment on or make changes to this bug.