Closed
Bug 589771
Opened 14 years ago
Closed 14 years ago
use gcPoke only during shutdown
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
DUPLICATE
of bug 652416
People
(Reporter: igor, Assigned: igor)
References
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
igor
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•14 years ago
|
||
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).
Attachment #470281 -
Flags: review?(anygregor)
Assignee | ||
Comment 2•14 years ago
|
||
rebased patch
Attachment #470281 -
Attachment is obsolete: true
Attachment #471083 -
Flags: review?(anygregor)
Attachment #470281 -
Flags: review?(anygregor)
Assignee | ||
Comment 3•14 years ago
|
||
rebased patch
Attachment #471083 -
Attachment is obsolete: true
Attachment #472792 -
Flags: review?(anygregor)
Attachment #471083 -
Flags: review?(anygregor)
Assignee | ||
Comment 4•14 years ago
|
||
Gregor: review ping
Comment 5•14 years ago
|
||
(In reply to comment #4)
> Gregor: review ping
tomorrow!
Comment 6•14 years ago
|
||
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?
Attachment #472792 -
Flags: review?(anygregor) → review+
Assignee | ||
Comment 7•14 years ago
|
||
(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.
Assignee | ||
Comment 8•14 years ago
|
||
Attachment #472792 -
Attachment is obsolete: true
Attachment #479013 -
Flags: review+
What's the status of this? It would be fantastic to get rid of gcPoke!
Assignee | ||
Comment 10•14 years ago
|
||
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.
Assignee | ||
Comment 11•14 years ago
|
||
The relevant changes are in the bug 652416.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → DUPLICATE
You need to log in
before you can comment on or make changes to this bug.
Description
•