kyren / gc-arena

Incremental garbage collection from safe Rust
Creative Commons Zero v1.0 Universal
436 stars 36 forks source link

Fixup the pacing algorithm to allow proper stop-the-world collection. #84

Closed kyren closed 7 months ago

kyren commented 7 months ago

Previously, the documentation for the timing_factor gc parameter was incorrect.. Setting the timing_factor to 0.0 did not do anything reasonable, and in fact resulted in the debt being NaN (inf - inf) and collection misbehaving.

The reason for this is that all debt that was accumulated was accumulated like...

debt += amt + amt / timing_factor;

...so setting the timing factor to 0.0 results in infinities.

This change redoes the pacing math so that all of the actual math is done in a single place: the Metrics::allocation_debt method. The rest of the methods only do simple accounting by incrementing or decrementing the associated counters.

Doing this makes changing the method to avoid infinities much easier. Allocations do not count until they are past wakeup_amount, which matches the behavior from before but this calculation does not involve timing_factor so it does not go wrong in the same way. If timing_factor is 0.0, then the returned debt will be inf as soon as allocations have grown past wakeup_amount, which is what we want, and a test was added to make sure this works.

The behavior around setting timing_factor to 0.0 and freed external bytes is somewhat unintuitive, but it is consistent. We do not decide when to free external bytes, so the fact that external bytes may be freed during the sleep period is unavoidable. When wakeup occurs, we count them as collection work that was already performed, but since this is now a stop-the-world collector, this doesn't prevent full collection from immediately occuring. Effectively, when timing_factor is set to 0.0 or near 0.0, freed external bytes do not count for much. So, when timing_factor is 0.0, doing the following...

for _ in 0..100 {
    arena.metrics().mark_external_allocation(1024);
    arena.metrics().mark_external_deallocation(1024);
}

Will start a full collection if wakeup_amount is < 100KiB. This is consistent with other kinds of allocation though, and is in fact what the timing factor controls.

moulins commented 7 months ago

Now that timing_factor is only used in a single place (the let debit_factor = 1 + 1/timing_factor expression), I would suggest precomputing and storing debit_factor directly in the Pacing struct, avoiding a (somewhat expensive) division in each allocation_debt() call.

Otherwise, LGTM!