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

[Thomas Schatzl]

Hi,

On Thu, 2015-07-16 at 11:16 +0200, Thomas Schatzl wrote:
> Hi,
> 
> On Wed, 2015-07-15 at 11:42 -0400, Tom Benson wrote:
> > Hi Thomas,
> [..]

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

  so I looked through the code again, and one way of passing on the
amount of memory actually used would be to return that value in
G1Allocator::end_archive_alloc_range(), to be added by the caller to the
_summary_bytes_used.

What is problematic, and what I did not know was that
StringTable::copy_shared_string() may not actually call
G1Allocator::end_archive_alloc_range() in case some allocation was not
successful.
I would actually think that this is some conceptual flaw in the use of
the archive allocator, i.e. allow a begin() call without a mandatory
end(), but that is up to you to decide.
That could be fixed though. What do you think?

At the moment the VM will just exit, but it seems unsafe to me to not
terminate the scope in all cases.

Thanks,
  Thomas