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

[Thomas Schatzl]

Hi,

On Wed, 2015-07-15 at 11:42 -0400, Tom Benson wrote:
> 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?

Yes and no.

One further change (actually the next in line) is going to tighten the
G1Allocator interface so that it would be more easily possible to have
multiple mutator allocation regions at the same time by just hiding all
the details about how mutator allocation regions are managed from
G1CollectedHeap. We know of some applications where the contention on
the single mutator allocation region seems to be a problem (I created
JDK-8131668 for that because I could not find a completely suitable
existing one). So yes, there may be multiple allocation regions in the
future or some other mechanism to avoid that.

Such a change is not planned as part for the investigation about PLAB
fragmentation/waste in G1 (JDK-8030849) this change is about.

Also, there are not multiple regions to consider at the moment, because
the allocation regions for PLABs are actually temporary, at the end of
GC they are flushed to be regular regions, only remembering old gen
regions that may be reused in the next gc. I.e. at the time the
information is queried, these region allocators are always empty.

So, no in some aspects. :)

I have no particular opinion about the name, but G1Allocator at the
moment manages multiple allocation regions already, so it seems more
fitting. Because otherwise someone might ask why not be even more
specific and explicitly mention that at the moment there is exactly one
(mutator) alloc region we consider.

I was thinking of splitting the PLAB and TLAB allocator into separate
instances, but did not think it through. Maybe something that should be
considered in more detail in a separate CR.

> 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           }

Yes, I saw that. I had thought about changing this, but then refrained
from it to make the change more straightforward.

> 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?

Actually, the PLAB allocators do exactly that every time a region is
retired via updating the actual bytes copied in the collector policy
which is then later added to the total.

I will look into this since it seems it is not only me not liking the
"if (archive_allocator != NULL) { ... }" copy&paste code here as it
might be easy to forget.

Thanks,
  Thomas