723286 - Remove JSContext * parameter from heap-traversal and related API

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

Before the bug 675078 the main reason why JSContext *cx argument was used in GC heap trace and enumeration API was to ensure exclusive access to the heap in presence of multiple threads. With single threaded runtime this is no longer necessary and the API like JSTracer-related calls that often refer to JSTracer::context can be changed to use JSRuntime * parameter instead.

This should allow to eliminate few temporary contexts that are created just to collect some statistics about the heap or when xpconnect has to create a special context just to run the cycle collection.

Comment 1

[Nicholas Nethercote [inactive]]

Nice!

Comment 2

[Andrew McCreight [:mccr8]]

Yay!  This is annoying to deal with in XPConnect.  Don't forget js::DumpHeapComplete.  I'm going to add a call to it in XPConnect (so we can actually use it...), but if you are going to do this soon I'll hold off.

Comment 3

[Igor Bukanov]

Attachment: v1

Untested patch

The removal of JSTracer::context required to update quite a few JSTracer callback implementations that were passing cx to various JS API, like JS_GetPrivate(cx, obj) or JS_GetReservedSlot. In all cases those API do not need the cx, dropping it or replacing it with rt works in single-threaded runtime. However, to minimize the patch, I just added few rt-versions of those API like JS_GetPrivateRT(obj).

Another observation is that apparently JSContext that is created for the cycle collection servers important role of being the last destroyed context in the runtime during the shutdown. That forces the last JS shutdown GC at the proper point in ~XPCJSRuntime. To workaround that the patch still creates an extra dummy JSContext instance during XPC initialization and destroy it at shutdown to properly do the last GC. On the plus side that context is also used as a cx parameter passed to JS_GC(). It eliminates the need to create an XPCCallContext instance and possibly and extra JSContext in nsXPConnect::Collect.

Comment 4

[Igor Bukanov]

Attachment: v2

Here is much smaller patch taking advantage of changes from bug 724310 and bug 723517.

Comment 5

[Igor Bukanov]

Attachment: v

Comment 6

[Igor Bukanov]

Attachment: v3

The patch eliminates JSTracer::content and moves all finalization-related code to SweepPhase allowing to remove the mark-phase dependence on a particular cx instance. 

As the preparation for the inremental sweeping in the patch I split the GC callback into separated finalization callback as our JSGC_MARK_END notification is really JSGC_FINALIZE_END and for incremental sweeping I will need to pass extra arguments to the callback.

Comment 7

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

Comment on attachment 601615 [details] [diff] [review]
v3

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

Looks great. I'm really glad you're eliminating trc->context, since it's very dangerous with incremental GC.

::: js/src/jsgc.cpp
@@ +3059,5 @@
>                           c->arenas.checkArenaListAllUnmarked());
>          }
>      }
>  #endif
> +    rt->gcMarker.stop();

I don't really like this change. It's confusing that EndMarkPhase calls stop, but BeginMarkPhase doesn't call start. Is there a reason you didn't move start to BeginMarkPhase?

In the future I want to do a patch to remove the barrier markers. That will make this change easier, since we won't have to worry about only incremental GC needing to call start on the barrier markers.

::: js/src/shell/jsheaptools.cpp
@@ +322,5 @@
>  HeapReverser::getEdgeDescription()
>  {
>      if (!debugPrinter && debugPrintIndex == (size_t) -1) {
>          const char *arg = static_cast<const char *>(debugPrintArg);
> +        char *name = static_cast<char *>(malloc(strlen(arg) + 1));

I think we're supposed to use js_malloc in these cases. Same for the others below.

Comment 8

[Igor Bukanov]

(In reply to Bill McCloskey (:billm) from comment #7)
> ::: js/src/shell/jsheaptools.cpp
> @@ +322,5 @@
> >  HeapReverser::getEdgeDescription()
> >  {
> >      if (!debugPrinter && debugPrintIndex == (size_t) -1) {
> >          const char *arg = static_cast<const char *>(debugPrintArg);
> > +        char *name = static_cast<char *>(malloc(strlen(arg) + 1));
> 
> I think we're supposed to use js_malloc in these cases. Same for the others
> below.

This is not a part of the engine but rather a part of the embedding where straight malloc/free is very ok. On the other hand one can argue that one point we may make jsheaptools a part of extended library, so ok, I will use js_something in that part as well.

Comment 9

[Igor Bukanov]

(In reply to Bill McCloskey (:billm) from comment #7)
> >  #endif
> > +    rt->gcMarker.stop();
> 
> I don't really like this change. It's confusing that EndMarkPhase calls
> stop, but BeginMarkPhase doesn't call start. Is there a reason you didn't
> move start to BeginMarkPhase?

I just missed that. I will update the patch and move the start BeginMarkPhase.

Comment 10

[Igor Bukanov]

Attachment: addressing comments

Here is an extra patch on top of v3 to address the comments.

Comment 11

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

Comment on attachment 601688 [details] [diff] [review]
addressing comments

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

Thanks.

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 601615 [details] [diff] [review]
v3

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

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +696,5 @@
> +
> +    nsTArray<JSGCCallback> callbacks(self->extraGCCallbacks);
> +    for (PRUint32 i = 0; i < callbacks.Length(); ++i) {
> +        callbacks[i](rt, status);
> +    }

XPConnect uses SpiderMonkey-style, so no braces.

Comment 13

[Igor Bukanov]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2778f515ed5a - landed without the extra { } in js/xpconnect/src/XPCJSRuntime.cpp

Comment 14

[Matt Brubeck (:mbrubeck)]

Backed out on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/65f74f6836a1

because I think it caused 64-bit OS X builds to crash during tests in multiple suites:
https://tbpl.mozilla.org/php/getParsedLog.php?id=9728297&tree=Mozilla-Inbound

Comment 15

[Igor Bukanov]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2bf1f0b762b2 - relanded with minimal chnages to jsgc.cpp. In particular, the new patch keeps the start of the background finalization in the GC lock that covers the GC session invocation so the background sweep does not start while JSRuntime::rt->gcRunning is true. I presume the background finalization somehow depends on that. 

If this sticks, I will file a separated bug to do the rest of cleanups that the initially landed patch was doing.

Comment 16

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/2bf1f0b762b2