589771 - use gcPoke only during shutdown

Status: RESOLVED DUPLICATE of bug 652416

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

Currently js_GC may run several GC loops if a finalizer calls js_RemoveRoot or similar API and presumably creates more garbage. But this very leaky as this assumes that the object that directly or indirectly stores the root can not be reached from an object stored in the root itself. To avoid such leaks the code should avoid explicit roots and use the GC tracing API to properly mark only reachable objects.

For this reason I suggest to remove support for several GC loops to simplify code and discourage the above leakage. Note that would not affect FF since for the reasons above FF code does not rely on GC looping. Also the effect on current embeddings that assumes gcPoke-induced extra GC loops would not be that strong as it would merely delay the garbage collection until the next GC cycle.

Comment 1

[Igor Bukanov]

Attachment: v1

In the patch I made the GC to loop until gcPoke is unset only on shutdown. On Linux an initial version of the patch that did just that regressed testConservativeGC. Apparently the simplifications made GCC to inline more the GC implementation. That lead the second GC run during the test to scan the stack area that includes the garbage left by the previous finalization code.

To workaround that I pass to ConservativeGCThreadData::recordStackTop a stack pointer from the most shallow stack frame that does not include any conservative GC roots. That fixed the regression.

In the patch I also removed debug-only JSRuntime::gcEmptyArenaList. That was a leftover from a pre-big GC chunks time. The corresponding debug functionality can be trivially substituted with explicit memset(a, JS_FREE_PATTERN, GC_ARENA_SIZE).

Comment 2

[Igor Bukanov]

Attachment: v2

rebased patch

Comment 3

[Igor Bukanov]

Attachment: v3

rebased patch

Comment 4

[Igor Bukanov]

Gregor: review ping

Comment 5

[Gregor Wagner [:gwagner]]

(In reply to comment #4)
> Gregor: review ping

tomorrow!

Comment 6

[Gregor Wagner [:gwagner]]

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

>-    do {
>-        rt->gcPoke = false;
>-
>+    {
>         AutoUnlockGC unlock(rt);
>-        if (firstRun) {
>-            PreGCCleanup(cx, gckind);
>-            TIMESTAMP(startMark);


Make sure that the Timestamp is at the right place again.


So a corner case would be that we perform a normal GC and a Last_Context GC comes along but
this can not happen right?
Another corner case is that we perform a GC and a root is deleted after we mark them.
You implied in your first comment that we don't rely on immediate finalization right?

Comment 7

[Igor Bukanov]

(In reply to comment #6)
> So a corner case would be that we perform a normal GC and a Last_Context GC
> comes along but
> this can not happen right?

Even if this happen this is fine as the last context GC still loops until gcPoke is unset. If that GC races with a normal GC, then the last GC would first wait for a normal GC to finish and then gcPoke check would trigger a new GC loop.

> Another corner case is that we perform a GC and a root is deleted after we mark
> them.
> You implied in your first comment that we don't rely on immediate finalization
> right?

The browser does not use the rooting API that sets gcPoke from a finalizer. It is way to easy to create a leak with that.

Comment 8

[Igor Bukanov]

Attachment: v3 rebased

Comment 9

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

What's the status of this? It would be fantastic to get rid of gcPoke!

Comment 10

[Igor Bukanov]

The patch rotten too much before I could land it. As gcPoke removal simplifies quite a few things for my patch in bug 652416 I moved the essential bits of the patch there.

Comment 11

[Igor Bukanov]

The relevant changes are in the bug 652416.