737364 - replace the cx argument with rt in the GC-related API

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

Currently to run the GC Spider Monkey requires a JSContext instance. However, this leads to the following problems:

1. During the runtime shutdown we cannot simply run the finalizers as for that we need JSContext instance whose creation and destruction is not controlled by the engine itself. In addition there are complications in case another JSContext is released during the GC. This forces the embedding to keep an artificial JSContext instance during the shutdown like in the bug 666963.

2. Similar the code is forced to keep/create a cx instance just to run the GC which in turn mey require extra steps to undo the effects of that cx creation. https://hg.mozilla.org/mozilla-central/file/9ff494dfc9b0/js/xpconnect/src/nsXPConnect.cpp#l405 is an example of this.

3. As the context is used on the background thread, that requires to wait in js_DestroyContext for the background GC to finish. This in turn may block the main thread for some time, like, for example, when one closes a tab or window.

4. The presence of cx in the finalizers leads to code that for convenience may call a fallible API potentially leading to incomplete finalization and related bugs.

So the idea is to remove the cx from the GC-related calls.

Comment 1

[Igor Bukanov]

Attachment: part 1 - stop using JSContext in the GC implementation

This part removes JSContext::gcBackgroundFree, moves all mark-related
cleanup code to run right after the marking is done for clear mark/sweep
separation and eliminates all JSContext references in the GC
implementation. That allowed to remove a wait for the bakground
finalization to finish in js_DestroyContext. As a followup for the bug
737365 the patch also replaces in few cases the JSContext argument with
FreeOp in infalible code that only free/destroy things.

Comment 2

[Igor Bukanov]

Attachment: part 2 - replace JSContext with JSRuntime in the GC-related API

This part replaces the JSContext *cx argument in most GC-related API
with JSRuntime *rt. When possible, the patch removes the code to obtain
a temporary cx just to run the GC.

The patch also removes JS_DestroyContextMaybeGC. That function is not
used in FF code base and its implementation is broken. It requires that
the context has an enetered compartment when it is destroyed, which in
turns implies a missing leave compartment call.

Comment 3

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

Comment on attachment 612854 [details] [diff] [review]
part 2 - replace JSContext with JSRuntime in the GC-related API

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

Excellent!

::: js/src/jsfriendapi.cpp
@@ +725,5 @@
>      return false;
>  }
>  
>  JS_FRIEND_API(void)
> +NotifyDidPaint(JSRuntime *rt)

Please merge WantGCSlice with NotifyDidPaint. The reason they were separate is that we usually don't need to run the code in NotifyDidPaint, and it required a context, which is slow to obtain. Now that it only needs a runtime, we can do all the work in one function.

::: js/xpconnect/idl/xpccomponents.idl
@@ -260,5 @@
>       * To be called from JS only.
>       *
>       * Force an immediate garbage collection cycle.
>       */
> -    [implicit_jscontext]

I'm not an expert, but I think maybe you need to update the UUID?

Comment 4

[Igor Bukanov]

(In reply to Bill McCloskey (:billm) from comment #3)
> >       * To be called from JS only.
> >       *
> >       * Force an immediate garbage collection cycle.
> >       */
> > -    [implicit_jscontext]
> 
> I'm not an expert, but I think maybe you need to update the UUID?

This is not necessary as the method is supposed to be called only from JS and in that case removal of implicit_jscontext and the cx argument from the method signature is fully binary compatible.

Comment 5

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

Comment on attachment 612852 [details] [diff] [review]
part 1 - stop using JSContext in the GC implementation

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

This patch looks fine to me. However, I'm wondering why you made the sweep phase a special case. Is this in preparation for making sweeping incremental? It seems like it would be easier to do with the existing code, but maybe I haven't thought about it enough.

::: js/src/jsgc.cpp
@@ +2899,5 @@
> +    rt->gcMarker.start(rt);
> +    JS_ASSERT(!rt->gcMarker.callback);
> +    JS_ASSERT(IS_GC_MARKING_TRACER(&rt->gcMarker));
> +
> +    if (incremental) {

You don't need an extra flag here. You can check if rt->gcIncrementalState != NO_INCREMENTAL.

Comment 6

[Igor Bukanov]

(In reply to Bill McCloskey (:billm) from comment #5)
> 
> This patch looks fine to me. However, I'm wondering why you made the sweep
> phase a special case. Is this in preparation for making sweeping
> incremental?

I am not sure what you mean by "special case". Is it related to the elimination of gc::State::SWEEP? The reason behind it is that currently it is used just for flow control in IncrementalGCSlice and explicit parameter passing IMO makes things clear. And you are right about incremental sweep - I hope it would not need any special state. It is just some things would not be sweept immediately but rather wait for events like allocation or perhaps other. And in that case the sweeping may run in parallel with incremental marking.

Comment 7

[Igor Bukanov]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7dffb77aabcd
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5589821fc04

Comment 8

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/7dffb77aabcd
https://hg.mozilla.org/mozilla-central/rev/d5589821fc04

Comment 9

[Matt Brubeck (:mbrubeck)]

It looks like this made bug 743127 near-permaorange on Linux64 debug builds.  (Sorry, I didn't notice this before merging it to mozilla-central.)

Comment 10

[Andrew McCreight [:mccr8]]

That's odd.  What that test does is create a cycle that goes through DOM and a watchpoint, then runs Components.Utils.schedulePreciseGC, followed by a number of cycleCollect and garbageCollects, then checks if the cycle has gone away.  The liveness of the cycle is observed using a weak reference kind of thing implemented using a weak map and Components.Utils.nondeterministicGetWeakMapKeys().

There's a similar test for weak maps, so it seems odd one would be affected but not the other.

Comment 11

[Igor Bukanov]

(In reply to Andrew McCreight [:mccr8] from comment #10)
> There's a similar test for weak maps, so it seems odd one would be affected
> but not the other.

As the orange is platform specific it may well be caused by the conservative GC false positives.

Comment 12

[Igor Bukanov]

(In reply to Igor Bukanov from comment #11)
> (In reply to Andrew McCreight [:mccr8] from comment #10)
> > There's a similar test for weak maps, so it seems odd one would be affected
> > but not the other.
> 
> As the orange is platform specific it may well be caused by the conservative
> GC false positives.

The test contains:

  Cu.schedulePreciseGC(function () {
    window.QueryInterface(Ci.nsIInterfaceRequestor)
         .getInterface(Ci.nsIDOMWindowUtils)
         .cycleCollect();
    window.QueryInterface(Ci.nsIInterfaceRequestor)
         .getInterface(Ci.nsIDOMWindowUtils)
         .garbageCollect();
    window.QueryInterface(Ci.nsIInterfaceRequestor)
         .getInterface(Ci.nsIDOMWindowUtils)
         .garbageCollect();
  
    ok(weak_ref_dead(cycle_ref), "Garbage gray watchpoint cycle should be collected.");

    SimpleTest.finish();
  });

There are two problems here. First schedulePreciseGC may still run the conservative scan due to the implementation deficiency - the code in PreciseGCRunnable::Run from XPCComponents.cpp should not assume that if JS_IsRunning is false, then the conservative stack scanner would not run. Second the test itself does not use PreciseGC for cycleCollect and garbageCollect.

Comment 13

[Andrew McCreight [:mccr8]]

That does sound like it could be a problem.  I did have a weird failure in the weak map version of the test that just went away at one point.  It shouldn't be hard to try out nesting the PreciseGCs (followed by a cycleCollect) instead of using garbageCollect().  The cycle collection shouldn't use the stack scanner.

I'm not very familiar with the PreciseGCRunnable code, so I'm not sure how bad that would be.

Comment 14

[Andrew McCreight [:mccr8]]

Well, since it sounds like this failure is bogosity due to the conservative stack scanner, so I guess we should disable the test until I get around to fixing the test.