Do not mark a page with any live object as GC_MARKED during quick sweep

[yuyichao]

There could be young objects in it!

This fixes the memory leak for my minimum repro in #11814 (comment). I have yet to check if this actually fix (github don't close it yet) #11814 itself.

@carnaval

design note for a alternative scheme that also solves this issue

[yuyichao]

Also updated the free page fast path logic since pg->gc_bits could be GC_QUEUED when entering the sweep now (when the page only contains free cells and to be promoted but dead cells).

[carnaval]

sorry for the lag. Yep the logic was flawed here, thanks for continuing to give this code the review time it probably lacked before the merge :-)

This fix should work but IIUC we are losing some of the fastpath advantage. I vaguely remember discussing this with you in person, but we should probably move away from the weird sweep-mark-sweep old gen collection. The easiest would be to have a flag that basically says "interpret GC_MARKED as GC_QUEUED for now". It looks like it would allow us to get rid of the sweep_mask things and probably clean up the code a little bit since all sweeps are now the same. I didn't think this through yet. Thoughts ?

[yuyichao]

This fix should work but IIUC we are losing some of the fastpath advantage.

Which fast path? (old page with no new objects?)

we should probably move away from the weird sweep-mark-sweep old gen collection

I think that could work but I also need to think about it more. We still need to distinguish bewteen for states during the full collection so I guess we could just flip the meaning of GC_MARKED and GC_QUEUED during the full collection and let the full sweep fix it?

[carnaval]

Which fast path? (old page with no new objects?)

yep, we'll be sweeping it for nothing on the first quick sweep after a long one right ?

The more I think about it the more I like the flag thing :

mark: (same as now)
gc_marked_noesc => cannot happen
gc_clean => gc_marked_noesc (we need to change this name btw :-))
gc_queued => gc_marked
gc_marked => stop scan

sweep:
gc_marked_noesc && young => gc_clean
gc_marked_noesc && old => the "gc_queued" that will be after this sweep (i.e., use gc_marked if we are swapping the meanings after the sweep)
gc_clean => freelist
gc_queued => freelist
gc_marked => gc_marked

flip: (semantically, really we only flip a flag)
gc_marked => gc_queued
gc_queued => gc_marked

We get rid of sweep_mask. I think we can also get rid of the "pre-sweep memory accounting" ugliness since we can decide to flip the flag after the sweep when we know the correct memory stats.

Let's see if this still sounds legit after a couple cups of coffee :-)

[carnaval]

I think we can also get rid of the "pre-sweep memory accounting" ugliness since we can decide to flip the flag after the sweep when we know the correct memory stats.

strike that, since

(i.e., use gc_marked if we are swapping the meanings after the sweep)

oh well.

[yuyichao]

So we do you want to flip the meaning and when do you decide whether a full collection is necessary?

I think the flip shouldn't last longer than a full collection because the WB won't be accurate otherwise. (therefore the (only and last) sweep in the full collection should fix it back).

And since we are not walking though the GC_MARKED objects at during the first (normal, fast) mark before we decide to do a full collection, I guess we still need to do two marking for the full collection? (and in that case we will need to ignore do some trick to make sure the previously marked object doesn't mess up the next full marking)

[carnaval]

also, this does not solve the problem that this PR is addressing since we can't know if a page had all it's queued cells marked after the mark phase.
So, unrelated to the above, we could keep a count of "non-GC_MARKED-cells" per page and update it as we mark instead of just the pg_gc_bits we have now.

[carnaval]

I'm not sure I understand the point about the WB. In what I had in mind, the WB code also uses the flag to correctly recognize if a specific bit pattern happens to mean gc_queued or gc_marked at the moment.

In fact, I think it's equivalent to separating full sweeps into two "phases" where the first one is a quick sweep and the other one is something that happens to be implementable without walking the heap at all.

(in this scheme, the fact that a sweep is long or short only depends on the pg_gc_bits optimization)

[yuyichao]

I'm not sure I understand the point about the WB. In what I had in mind, the WB code also uses the flag to correctly recognize if a specific bit pattern happens to mean gc_queued or gc_marked at the moment.

I'm hoping that we don't need to change the WB code..... Will this make it more expensive since we need to read a global variable everytime?

[carnaval]

Yep. We could mitigate this a little by doing a first check not_clean(parent) && clean(child) without loading global state. In any case we should definitely check that the additional cost is not significant.

[yuyichao]

So, unrelated to the above, we could keep a count of "non-GC_MARKED-cells" per page and update it as we mark instead of just the pg_gc_bits we have now.

What about uint16_t n_marked, n_queued, n_marked_noesc; would this book keeping be too costy for marking?

[tkelman]

Bump, is this still needed?

[yuyichao]

Yes but shouldnt hold up the next 0.4 release

[tkelman]

Bump again, why hasn't this been merged into master yet?

[JeffBezanson]

@carnaval @yuyichao Should we merge this? Is there evidence that it carries an unacceptable slowdown?

[JeffBezanson]

Bump.

[yuyichao]

Replacement coming soon.