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

[Mikael Gerdin]

On 2015-07-21 17:22, Thomas Schatzl wrote:
> Hi Mikael,
>
> On Tue, 2015-07-21 at 14:57 +0200, Mikael Gerdin wrote:
>> Hi Thomas,
>>
>> On 2015-07-15 12:47, 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/
>>
>> The change looks good to me.
>> Have you double-checked that the agent changes work (with jmap -heap)?
>
> Thanks for the review.
>
> Apparently not :(. I checked now, and there needs to be some further tiny
> change in the SA code:
>
> http://cr.openjdk.java.net/~tschatzl/8131319/webrev.0_to_1 (diff)
> http://cr.openjdk.java.net/~tschatzl/8131319/webrev.1 (full)

Great! Thanks for verifying the SA code.
Looks good now.

/Mikael

>
> Thanks,
>    Thomas
>
>