Keep track of per-cycle mutator/collector allocs. Fix mutator/collector alloc region overlap in traversal.

[Roman Kennke]

Am 13.03.2018 um 17:24 schrieb Aleksey Shipilev:
> On 03/09/2018 03:29 PM, Roman Kennke wrote:
>> Turns out I introduced a bug when extracting it from my traversal-degen
>> work. I made it such that the ShenandoahConcurrentThread (can we maybe
>> rename it to ShenandoahControlThread or something please?!) resets the
>> counters. However, this runs concurrently with Java allocs pre-cycle, so
>> it will be racy. This little change moves the resetting to the op_init_
>> phases, so that it happens during STW instead.
>>
>> Differential:
>> http://cr.openjdk.java.net/~rkennke/traversal-alloc-region/webrev.01.diff/
>> Full patch:
>> http://cr.openjdk.java.net/~rkennke/traversal-alloc-region/webrev.01/
> 
> I think this is a simple solution, but its simplicity is deceptive.
> 
> See, this solution:
>  1. Adds pause time, because we walk regions during the pause to reset counters now;
>  2. Turns the statistics counters into functionality-critical bits of data, and e.g. their racy
> updates would cause functional bugs;
>  3. Makes weird tests on allocation fast-path;
>  4. Undermines current free set performance mechanics. Current code relies on density of bitmaps
> performance-wise: if we cannot allocate in the region, we move the bitmap boundaries right away.
> Current patch requires scanning the freeset for fitting regions when GC and mutator allocs meet each
> other, because there can now be full regions of different type within the boundaries.
> 
> The better solution, in my mind, would be biasing the freeset regions to the particular type of
> allocation. That is, freeset can maintain two bitmaps and two sets of boundaries instead of one, and
> use those for mutator/GC allocations. Once one bitmapped-type is depleted, the allocation path may
> decide to "borrow" from another-type bitmap, if the borrowed region is empty, or even borrow
> non-empty region if runtime option allows that (and that option would be "false" for traversal). It
> solves all four problems with the patch above.

This seems overly rigid to me.

It seems easier to do what I had in mind originally, way back when I
first implemented the separate handling of collector vs. mutator allocs
in traversal: set a flag in ShHeapRegion to indiciate whether a region
has been used for mutator or collector alloc, or no alloc at all, and
then test that in the allocation search-loop.

Like that:
http://cr.openjdk.java.net/~rkennke/traversal-alloc-region/webrev.02/

WDYT?

Cheers, Roman