nonincrementalByAPI GCs are too restrictive
Categories
(Core :: JavaScript: GC, enhancement, P2)
Tracking
()
People
(Reporter: sfink, Unassigned)
References
(Blocks 1 open bug)
Details
Mixing incremental and nonincremental GCs with GC callbacks is a pain:
- if a nonincremental GC is requested and a JSGC_END callback starts a new incremental GC, then the GC will get reset and rerun. This causes problems when the retriggered GC finishes, because it'll call the callback again (problem 1) and unless some state is kept, the same thing could easily happen and it will loop (problem 2)
- similar to the above, the callback is the same for nonincremental and incremental GCs, and has no straightforward way of knowing which one it is within. Yet it matters, because starting a new incremental GC makes sense for one and (currently) not the other.
- the whole resetting business is checked after the
JSGC_BEGIN
callback fires, which means that if it starts an incremental GC, that GC will always get reset. (This is not a problem for my actual code, since starting a GC within a beginning of GC hook is kinda weird, but it makes a funky special case for test code.) nonincrementalByApi
is sort of a weird value that has a special case forGCReason::ALLOC_TRIGGER
- it's still not clear to me whether resetting a
nonincrementalByApi
GC is even the right thing. I don't totally understand the comment:
// Reset any in progress incremental GC if this was triggered via the
// API. This isn't required for correctness, but sometimes during tests
// the caller expects this GC to collect certain objects, and we need
// to make sure to collect everything possible.
If we're going to run a GC to completion, why not finish off the current one and skip the new one? Is it because iGC can sometimes retain more than synchronous GC?
And shouldn't this happen earlier anyway, when the nonincremental GC is requested, certainly before its JSGC_BEGIN callback fires? Then the weird situation would be: (1) JS::NonincrementalGC() called; (2) if iGC is running, it is reset; (3) JSGC_BEGIN starts a new incremental GC; (4) nonincrementalByApi
is consulted. If we want to keep (2) instead of just finishing the ongoing GC, then it seems like nonincrementalByApi
should probably finish this new GC (because we definitely want at least one GC to complete by the time JS::NonincrementalGC()
returns. JSGC_END could always start yet another GC, but at least one should finish.)
I guess there's the case where a GC has different options (eg Shrinking) or a different set of Zones.
Anyway, this bug is for doing something to improve the situation.
Reporter | ||
Comment 1•2 months ago
|
||
Right now, I'm trying out a patch that moves up the reset to before the JSGC_BEGIN callback invocation. It made my test (which is a nonincremental GC with callbacks that start incremental GCs) succeed most of the time, but triggers various assertions about 10% of the time. I'm debugging those now. It might not be the right approach.
This is on top of patches that move JSGC_END callbacks to fully after the outer GC is over, and weakening some assertions (though with the reset, I'm now doubting whether they needed to be weakened.)
Updated•2 months ago
|
Description
•