RFR(M): 8218049: Survivor MemoryMXBean used() size granularity is region based

[sangheon.kim at oracle.com]

Hi Thomas,

On 4/3/19 3:19 AM, Thomas Schatzl wrote:
> Hi all,
>
> On Wed, 2019-04-03 at 01:05 -0700, sangheon.kim at oracle.com wrote:
>> Hi all,
>>
>> Here's the second webrev which includes some recommendations from
>> Thomas:
>> 1) Fixing memory ordering issue
>    just to make this clear to other reviewers: there is and have been
> issues where the calculation can result in values that are not what the
> GC sees due to memory ordering (i.e. negative amount of memory usage in
> old gen changed to zero internally for consumption). My suggestion
> adding memory barriers avoids the need for the "subtract_up_to_zero"
> method (I believe ;))
Thanks for the summary!

>
> It is not the goal of this change, and at this time there is no intent
> to make all these values reflect the internal state atomically (i.e. is
> a pre-existing problem). I have some idea how this could be done, but
> it seems a lot of work; if somebody is interested I could write up
> something, so contributors welcome.
>
> Another suggestion was to make these values storing the various "used"
> bytes volatile to avoid the compiler re-reading them during the
> calculations.
I forgot to mention.
Yes, changed some members to volatile.

>
>> 2) Changing how G1SurvivorRegions collect data.
>> 3) Some minor refactoring.
>>
>> http://cr.openjdk.java.net/~sangheki/8218049/webrev.1
>> http://cr.openjdk.java.net/~sangheki/8218049/webrev.1_inc_0/
>> Testing: hs-tier 1 ~ 5, survivorAlignment/* tests
> Some further minor comments:
>
> - g1CollectedHeap.cpp:4650: please add an assert(dest.is_young(),
> "...") here.
>
> - g1EdenRegions.hpp:36: s/i.e. Updated/I.e. updated"
>
> - g1MonitoringSupport.cpp:213: s/always doesn't/never :) (or just
> remove the "always")
>
> - g1MonitoringSupport.cpp:282: the comment is not up-to-date with
> regards what is actually done (for survivor values). Survivor values
> should still be consistent as they are written during a safepoint, so
> the assert must still hold. Just the comment is wrong now.
All fixed.

http://cr.openjdk.java.net/~sangheki/8218049/webrev.2
http://cr.openjdk.java.net/~sangheki/8218049/webrev.2_inc_1/

Thanks,
Sangheon
>
> Looks good to me otherwise.
>
> Thanks,
>    Thomas
>
>