removal of JSRuntime::gcLevel

RESOLVED FIXED

Status

()

Core
JavaScript Engine
--
enhancement
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

Other Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 attachments, 7 obsolete attachments)

(Assignee)

Description

8 years ago
+++ 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.
(Assignee)

Comment 1

8 years ago
Created attachment 441041 [details] [diff] [review]
v1

Here is completely untested patch
Assignee: general → igor
(Assignee)

Comment 2

8 years ago
Created attachment 441139 [details] [diff] [review]
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.
Attachment #441041 - Attachment is obsolete: true

Comment 3

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

Comment 4

8 years ago
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.
(Assignee)

Updated

8 years ago
Attachment #441139 - Flags: review?(jorendorff)
(Assignee)

Comment 5

8 years ago
Created attachment 442066 [details] [diff] [review]
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.
Attachment #441139 - Attachment is obsolete: true
Attachment #442066 - Flags: review?(jorendorff)
Attachment #441139 - Flags: review?(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.
Attachment #442066 - Flags: review?(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.)
(Assignee)

Comment 8

8 years ago
(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.
(Assignee)

Comment 9

8 years ago
Created attachment 442680 [details] [diff] [review]
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.
Attachment #442066 - Attachment is obsolete: true
Attachment #442680 - Flags: review?(jorendorff)
(Assignee)

Comment 10

8 years ago
Created attachment 442682 [details] [diff] [review]
v5

v4 had some unrelated comment text changes. v5 does not do that to get a smaller patch.
Attachment #442680 - Attachment is obsolete: true
Attachment #442682 - Flags: review?(jorendorff)
Attachment #442680 - Flags: review?(jorendorff)
(Assignee)

Comment 11

8 years ago
Created attachment 442683 [details]
changed part of jsgc.cpp

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

Comment 12

8 years ago
Created attachment 443054 [details] [diff] [review]
v6

One-liner change to fix compilation problem with !JS_THREADSAFE.
Attachment #442682 - Attachment is obsolete: true
Attachment #443054 - Flags: review?(jorendorff)
Attachment #442682 - Flags: review?(jorendorff)
(Assignee)

Comment 13

8 years ago
Created attachment 444051 [details] [diff] [review]
v7

Updating the patch for the TM tip changes (merge conflicts were mostly due to the bug 552560).
Attachment #443054 - Attachment is obsolete: true
Attachment #444051 - Flags: review?(jorendorff)
Attachment #443054 - Flags: review?(jorendorff)
(Assignee)

Comment 14

8 years ago
Created attachment 444627 [details] [diff] [review]
v8

Another TM tip sync
Attachment #444051 - Attachment is obsolete: true
Attachment #444627 - Flags: review?(jorendorff)
Attachment #444051 - Flags: review?(jorendorff)
(Assignee)

Comment 15

8 years ago
jorendorff - any chance to review this soon? My changes for the bug 516832 are based on top of this.
Yes, I'll review it today.
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 on attachment 444627 [details] [diff] [review]
v8

r=me with the nits addressed.
Attachment #444627 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 19

8 years ago
http://hg.mozilla.org/tracemonkey/rev/33d8fac07e74
Whiteboard: fixed-in-tracemonkey
(Assignee)

Updated

8 years ago
Blocks: 565894

Comment 20

8 years ago
http://hg.mozilla.org/mozilla-central/rev/33d8fac07e74
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.