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

[Tom Benson]

Hi,

On 7/16/2015 6:23 AM, Thomas Schatzl wrote:
> 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.

I did test that scenario (where the archive is left 'un-finalized') 
somewhat, while testing with hardcoded calls to the archive region code, 
so I believe it would work.  Collections still happened without failure 
- the active archive region is parseable, and is already marked 
'archive.'  But it's true that the shared string support will just exit 
the VM if an allocation fails, so it's certainly not a problem that 
'end' is not called today.

I doesn't strike me as a 'conceptual flaw.'  You can keep allocating in 
the current archive region until you call 'end'. If you don't call 
'end,' you can keep doing so until the JVM exits.  That is, if you never 
need the summary of region addresses that end_ provides. I can't think 
of a use for that off the top of my head, but I guess it could happen. 8^)
Tom

>
> Thanks,
>    Thomas
>
>