561364 - removal of JSRuntime::gcLevel

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

+++ This bug was initially created as a clone of Bug #560471 +++

Currently gcLevel is used for two purposes. First together with gcRunning it ensures the GC serialization. Second together with gcPoke it is used to restart the marking/sweep cycle to collect more garbage. It would be nice to remove this complexity. 

As far as I can see the first usage and gcRunning can be replaced by single 3-state variable that transitions from gc_starting->gc_running->gc_done. The second usage can be completely taken by gcPoke.

Comment 1

[Igor Bukanov]

Attachment: v1

Here is completely untested patch

Comment 2

[Igor Bukanov]

Attachment: v2

The patch replaces gcRunning and gcLevel by single tree-state gcState and uses gcPoke to restart the GC. To restart the GC when js_GC is called recursively or from another thread the patch uses gcPoke.

Also the patch completely separates the set slot requests from the ordinary GC. If the set slot happens when a normal GC waits for all requests to finish, the set slot thread lets the GC to run. Then the set slot proceeds. It eliminates JSRuntime::setSlotRequests as now JSSetSlotRequest is directly passed to the GC.

Comment 3

[Andreas Gal :gal]

Could we briefly discuss the rationale for gcPoke. What real world test case motivated its introduction, and are we still sure we need it?

Comment 4

[Igor Bukanov]

The original reason for gcPoke existance is to restart the GC cycle if one of the finalizers would remove a root using JS_RemoveRoot API so more garbage can be collected. The patch above reuses that to eliminate gcLevel. There is no point to increase the latter if we can simply set the former. But I do not know the logic behind restarting the GC cycle if js_GC is called from two threads. Brendan, could comment on that?

Note that the patch also uses gcPoke to start the real GC if another thread is doing setProto. So if we decide to eliminate the above users of gcPoke this one still would be necessary.

Comment 5

[Igor Bukanov]

Attachment: v3

The new patch have not used 3-state JSRuntime::gcState. Instead it keeps boolean JSRuntime::gcRunning to indicate as before that the GC is running. As a replacement for gcLevel it uses JSThread::gcThread. The latter field together with !gcRunning indicates a GC thread that is waiting for other requests to finish.

Comment 6

[Jason Orendorff [:jorendorff]]

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

I think removing all the horrible hacks for GC_SET_SLOT_REQUEST means that if one thread enters js_GC to do a slot request, and another thread enters js_GC immediately afterwards to do GC, no GC will occur. It seems like that would be bad. Am I missing something?

In any case I think the specific change proposed in comment 0 is a great idea. Can you post a patch containing just that change so we can get it in as soon as possible? I'll review it speedily.

Hacking out the craziness around SetSlotRequests is a great idea too, but a separate bug and patch for that would be much appreciated.

Comment 7

[Jason Orendorff [:jorendorff]]

One more note -- it seems like you could simplify a bunch more. For example, this:

        JSSetSlotRequest ssr;
        ssr.obj = obj;
        ssr.pobj = pobj;
        ssr.slot = (uint16) slot;
        ssr.cycle = false;

        js_GC(cx, GC_NORMAL, &ssr);
        if (ssr.cycle) {

could just say:

        if (!js_CheckAndSetProto(cx, obj, pobj))

It makes sense for this to be a separate function from js_GC since the intent is so different; they already (in v3) have practically no code in common.

(JSSetSlotRequests are only ever used when setting __proto__, as far as I can tell.)

Comment 8

[Igor Bukanov]

(In reply to comment #6)
> (From update of attachment 442066 [details] [diff] [review])
> I think removing all the horrible hacks for GC_SET_SLOT_REQUEST means that if
> one thread enters js_GC to do a slot request, and another thread enters js_GC
> immediately afterwards to do GC, no GC will occur.

No - the patch set gcPoke to true when a GC thread is waiting for other requests and js_GC calls to finish. With the patch set slot request does not touch the gcPoke, so the GC call would see that gcPoke is still set and start the normal GC.

> In any case I think the specific change proposed in comment 0 is a great idea.
> Can you post a patch containing just that change so we can get it in as soon as
> possible? I'll review it speedily.

Doing that would mean more work as before the set proto request changes I AFAICS cannot use gcPoke reliably to restart the GC. And unfortunately the changes for the bug 553671 makes this patch bigger. Sorry I have not pointed it out sufficiently early during the development of that bug that the biggest obstacle IMO for untangling the goto mess in a simple way was restarting of the GC after the set proto slot request.

Comment 9

[Igor Bukanov]

Attachment: v4

The new patch uses separated js_SetProtoOrParentCheckingForCycles function to implement __proto__/__parent__ changes. I have not removed the __parent__ support since it is better to do that in a separated bug.

Comment 10

[Igor Bukanov]

Attachment: v5

v4 had some unrelated comment text changes. v5 does not do that to get a smaller patch.

Comment 11

[Igor Bukanov]

Attachment: changed part of jsgc.cpp

For references this is how jsgc.cpp looks with the patchg applied starting after the static GC function.

Comment 12

[Igor Bukanov]

Attachment: v6

One-liner change to fix compilation problem with !JS_THREADSAFE.

Comment 13

[Igor Bukanov]

Attachment: v7

Updating the patch for the TM tip changes (merge conflicts were mostly due to the bug 552560).

Comment 14

[Igor Bukanov]

Attachment: v8

Another TM tip sync

Comment 15

[Igor Bukanov]

jorendorff - any chance to review this soon? My changes for the bug 516832 are based on top of this.

Comment 16

[Jason Orendorff [:jorendorff]]

Yes, I'll review it today.

Comment 17

[Jason Orendorff [:jorendorff]]

I really dig this patch. Every change makes me want to cheer. A few comment
nits and one very minor behavior nit:

In jsgc.cpp:
> + * This thread becomes the GC thread. Wait for all other threads to quiesce.
> + * Then set rt->gcRunning and return. The caller must call
> + * EndGCSession when GC work is done.
>   */

Re-wrap that to move EndGCSession onto the previous line.

In js_GC:
> +        if (callback) {
> +            Conditionally<AutoUnlockGC> unlockIf(gckind & GC_LOCK_HELD, rt);

Previously we re-sampled rt->gcCallback after GC. Not re-sampling is an
API-visible change. I don't know that it would break anyone -- but is there any
reason not to keep the old behavior?

In jsgc.h:
> +/*
> + * Set object's prototype or parent while checking that doing that would not
> + * cycle the proto or parent chain. The cycle check and proto change are done

"that doing so would not create a cycle in"

> + * only when all other request are finished or suspended to ensure an

"all other requests" (plural)

> + * exclusive access to the chain.

"to ensure exclusive access" (remove the word "an")

>                                    The function returns true if there were no
> + * cycles and the proto or proto is changed. It returns false if there is a
> + * cycle. No errors are reported in this case.

"If there is a cycle, return false without reporting an error. Otherwise, set
the proto or parent and return true."

Comment 18

[Jason Orendorff [:jorendorff]]

Comment on attachment 444627 [details] [diff] [review]
v8

r=me with the nits addressed.

Comment 19

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/33d8fac07e74

Comment 20

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/33d8fac07e74