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

[Tom Benson]

Hi Thomas,

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

OK.  Sounds like mostly yes, it's intentionally named that way for 
possible futures.

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

I agree it's worth a look.  Tnx,
Tom

> Thanks,
>    Thomas
>
>