RFR (S): 8131319: Move G1Allocator::_summary_bytes_used back to G1CollectedHeap

[Tom Benson]

Hi Thomas,
Makes sense and looks good to me, with one trivial comment. 
G1Allocator::used() was renamed to ::used_in_alloc_regions(), presumably 
to indicate it only returns the amount used in the current active 
allocation region(s).   But at present, at least, it only returns the 
size for the current mutator region.  Is the plural name looking forward 
to an expected future change?

Also an observation..  G1ArchiveAllocator still has its own 
_summary_bytes_used.  This has to be cleared when recalculate_used is 
used to reset the heap's _summary_bytes_used, in a couple of places, EG:

   4106           set_used(recalculate_used());
   4107           if (_archive_allocator != NULL) {
   4108             _archive_allocator->clear_used();
   4109           }

That's because recalculate used walks all the G1 regions so its total 
includes the space allocated by an ArchiveAllocator.   Now that 
_summary_bytes_used is in _g1h rather than the _allocator, we could 
consider having the ArchiveAllocator update that total, rather than its 
local one.  That should allow the somewhat odd clear_used() calls to go 
away, but we would then have ArchiveAllocator calling 
_g1h->increase_used, which also doesn't seem ideal.   What do you think?

Tom

On 7/15/2015 6:47 AM, Thomas Schatzl wrote:
> Hi all,
>
>    can I have comments and reviews for the following change to move
> G1Allocator::_summary_bytes_used to a more fitting place?
>
> The problem from my POV is, that the member
> G1Allocator::_summary_bytes_used, that contains the number of bytes held
> by the heap outside of the allocators is located within G1Allocator.
>
> That sounds odd, particularly because G1Allocator actually never uses
> it, it's managed completely by G1CollectedHeap.
>
> Before the addition of the G1ArchiveAllocator one could have made the
> argument that G1Allocator holds all memory contained in the heap, but
> this is not true any more: the memory occupied by G1ArchiveAllocator
> needs to be added to the total sum of all managed space anyway, so
> moving _summary_bytes_used out of G1Allocator seems sensible (to me).
>
> I talked to StefanJ about this change who originally moved
> _summary_used_bytes into G1Allocator, and we could not find an argument
> for keeping _summary_used_bytes in G1Allocator.
>
> This allows us to also refine the responsibilities of G1Allocator a
> little more, to be the owner of the current allocation areas (regions)
> only.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8131319
>
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8131319/webrev/
>
> Testing:
> jprt
>
> Thanks,
>    Thomas
>
>