RFR (S): 8073052: Rename and clean up the allocation manager hierarchy in g1Allocator.?pp
thomas.schatzl at oracle.com
Mon Mar 9 11:02:02 UTC 2015
thanks for the review.
There is a new webrev at
Further comments inline:
On Fri, 2015-03-06 at 10:00 +0100, Stefan Johansson wrote:
> Hi Thomas,
> On 2015-03-05 15:58, Thomas Schatzl wrote:
> > Hi all,
> > On Wed, 2015-03-04 at 16:28 +0100, Thomas Schatzl wrote:
> >> Hi all,
> >> can I have reviews for the following change that does some renamings
> >> in the allocation related class hierarchy, also adding a few lines of
> >> documentation.
> >> This change is intentionally limited to renames to keep it simple, in a
> >> subsequent patch there will be more cleanup changes, moving methods
> >> around.
> >> Also, if there is demand, I am open to rename the files to something
> >> different (suggestions?) in a follow-up change (to not confuse the
> >> webrev tool too much).
> >> CR:
> >> https://bugs.openjdk.java.net/browse/JDK-8073052
> >> Webrev:
> >> http://cr.openjdk.java.net/~tschatzl/8073052/webrev.00/
> >> Testing:
> > I talked with Stefan Johansson a bit about the change, with the result
> > that we keep the name of G1Allocator, since in the upcoming changes (see
> > below for more) that class is supposed to handle all allocations within
> > the current allocation regions.
> > Webrev:
> > http://cr.openjdk.java.net/~tschatzl/8073052/webrev.01
> Generally change looks good, but I agree with Kim that PLABAllocator
> could still use the G1 prefix for consistency. Some other comments:
> + // Allocates a new heap region instance.
> + virtual HeapRegion* new_heap_region(uint hrs_index, MemRegion mr);
> No need to have this one virtual, no one extends G1CollectedHeap.
> Since _summary_bytes_used have been moved in the native code this needs
> to be reflected in the Java agent as well. You need to move the
> functionality from G1Allocator.java to G1CollectedHeap.java.
More information about the hotspot-gc-dev