Last Comment Bug 723286 - Remove JSContext * parameter from heap-traversal and related API
: Remove JSContext * parameter from heap-traversal and related API
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla13
Assigned To: Igor Bukanov
:
Mentors:
Depends on: 675078
Blocks: 722345 731618 743436
  Show dependency treegraph
 
Reported: 2012-02-01 14:02 PST by Igor Bukanov
Modified: 2012-04-07 03:08 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (299.28 KB, patch)
2012-02-01 15:26 PST, Igor Bukanov
no flags Details | Diff | Review
v2 (151.24 KB, patch)
2012-02-05 13:02 PST, Igor Bukanov
no flags Details | Diff | Review
v (68.17 KB, patch)
2012-02-29 07:42 PST, Igor Bukanov
no flags Details | Diff | Review
v3 (68.17 KB, patch)
2012-02-29 07:59 PST, Igor Bukanov
wmccloskey: review+
Details | Diff | Review
addressing comments (4.23 KB, patch)
2012-02-29 11:47 PST, Igor Bukanov
wmccloskey: review+
Details | Diff | Review

Description Igor Bukanov 2012-02-01 14:02:20 PST
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 [:njn] (on vacation until July 11) 2012-02-01 14:23:00 PST
Nice!
Comment 2 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2012-02-01 14:46:56 PST
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 2012-02-01 15:26:36 PST
Created attachment 593630 [details] [diff] [review]
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 2012-02-05 13:02:21 PST
Created attachment 594589 [details] [diff] [review]
v2

Here is much smaller patch taking advantage of changes from bug 724310 and bug 723517.
Comment 5 Igor Bukanov 2012-02-29 07:42:12 PST
Created attachment 601610 [details] [diff] [review]
v
Comment 6 Igor Bukanov 2012-02-29 07:59:27 PST
Created attachment 601615 [details] [diff] [review]
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 (:billm) 2012-02-29 09:58:08 PST
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 2012-02-29 11:31:13 PST
(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 2012-02-29 11:37:46 PST
(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 2012-02-29 11:47:21 PST
Created attachment 601688 [details] [diff] [review]
addressing comments

Here is an extra patch on top of v3 to address the comments.
Comment 11 Bill McCloskey (:billm) 2012-02-29 11:49:02 PST
Comment on attachment 601688 [details] [diff] [review]
addressing comments

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

Thanks.
Comment 12 :Ms2ger 2012-02-29 12:48:18 PST
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 2012-02-29 12:58:59 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/2778f515ed5a - landed without the extra { } in js/xpconnect/src/XPCJSRuntime.cpp
Comment 14 Matt Brubeck (:mbrubeck) 2012-02-29 16:20:33 PST
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 2012-03-01 02:23:31 PST
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] 2012-03-02 06:09:09 PST
https://hg.mozilla.org/mozilla-central/rev/2bf1f0b762b2

Note You need to log in before you can comment on or make changes to this bug.