RFR (M): 8073052: Rename and clean up the allocation manager hierarchy in g1Allocator.?pp

[Mikael Gerdin]

Hi Thomas,

On 2015-07-28 10:46, Thomas Schatzl wrote:
> Hi all,
>
>    can I have reviews for this change that cleans up G1Allocator related
> code in preparation for actual PLAB sizing changes?
>
> The change tries to decrease the number of dependencies between
> g1allocator, g1collectedheap and g1allocregion.
>
> It is motivated by existing circular calls between the classes,
> particularly G1CollectedHeap and G1Allocator.
>
> E.g. G1Allocator::allocate_direct_or_plab() calls
> G1CollectedHeap::par_allocate_during_gc(), which after calling a few
> helper functions calls G1Allocator::old/survivor_gc_alloc_region() and a
> few G1AllocRegion methods.
>
> This change makes the procedure much more straightforward by moving the
> methods that deal with actual allocation (not policy) into G1Allocator,
> allowing us to hide G1AllocRegion from G1CollectedHeap completely.
> (G1AllocRegion is a helper class anyway).
>
> Further, this moving around of methods showed that there is an include
> cycle between g1CollectedHeap.inline.hpp, i.e.
> G1CollectedHeap::obj_in_cs() referencing HeapRegion::in_collection_set()
> in heapRegion.inline.hpp that references G1CollectedHeap::is_in_cset()
> in g1CollectedHeap.inline.hpp again.
>
> For whatever reason compilers do not complain, but moving methods made
> that show up.
>
> This has been fixed by moving more method implementatations into
> the .inline.hpp files, and putting the implementation of
> G1CollectedHeap::obj_in_cs() from the g1CollectedHeap.hpp into the .cpp
> file.
>
> Finally, the change aligns the naming of G1ParGCAllocator with other
> classes that use "PLAB" in their names instead of "ParGC".
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8073052
>
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8073052/webrev.3/

Overall a nice cleanup!

g1Allocator.cpp:
In unsafe_max_tlab_alloc you get the heap via G1H::heap() instead of _g1h.

G1PLABAllocator seems to look up the G1H::heap() fairly often,
did you consider passing on the G1H* to the G1PLABAllocator to reduce 
the clutter from accessing through the static getter in multiple 
locations? Another option could be to expose the G1H* from G1Allocator, 
yielding _allocator->heap()->...
instead of _g1h->...

otherwise it looks good to me.

/Mikael


>
> Testing:
> jprt
>
> Note that there has already been an RFR with that number earlier,
> however at that time a lot of other changes in this area were done by
> others, and the problems with these changes have only been fixed a few
> weeks ago.
>
> Thanks,
>    Thomas
>