RFR [L][3/7]: 8197850: Calculate liveness in regions during marking

[Stefan Johansson]

Thanks Thomas,

This looks good to me, reviewed.

Stefan

On 2018-03-07 09:48, Thomas Schatzl wrote:
> Hi,
>
> On Tue, 2018-03-06 at 15:03 +0100, Stefan Johansson wrote:
>> On 2018-03-06 14:33, Thomas Schatzl wrote:
>>> On Tue, 2018-03-06 at 13:31 +0100, Stefan Johansson wrote:
>>>> Hi Thomas,
>>>>
>>>> On 2018-03-05 15:41, Thomas Schatzl wrote:
>>>>> Hi all,
>>>>>
>>>>> ...
>>>>>
>>>>> CR:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8197850
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~tschatzl/8197850/webrev/index.html
>>>> Look good, just some minor things.
>>>>
>>>> src/hotspot/share/gc/g1/g1ConcurrentMark.cpp
>>>> 394   _region_mark_stats(NEW_C_HEAP_ARRAY(G1RegionMarkStats,
>>>> max_regions, mtGC))
>>>>
>>>> max_regions is passed into the constructor but everywhere else in
>>>> G1ConcurrentMark we use g1h->max_regions(). I would consider
>>>> either
>>>> calling g1h->max_regions() in the constructor too, or save the
>>>> value
>>>> in a member and use it everywhere.
>>> Done using option 1), use g1h->max_regions() everywhere.
>> Thanks.
>>>> ---
>>>> src/hotspot/share/gc/g1/g1_globals.hpp
>>>>     262   experimental(size_t, G1RegionMarkStatsCacheSize, 1024,
>>>>     ...
>>>>     265           range(128, (max_juint / 2) + 1)
>>>>
>>>> Do we need this flag or should we scale it against the number of
>>>> regions? If we want to keep it I think we should have a somewhat
>>>> more restrictive range.
>>> I would not want this to scale according to the number of heap
>>> regions,
>>> because that is going to have a significant impact on pause times
>>> when
>>> all of the thread's caches are evicted (The O(#regions) part).
>> True, just a thought since 1024 is roughly half the number regions
>> we aim for maybe this could be used together with a reasonable upper
>> bound.
>>
>>> The flag has only been added (rather late, admittedly) to fix
>>> potential issues with marking time with really large heaps with
>>> tens of thousands of regions.
>>> Let me do some more measurements, I will get back to you with them
>>> before giving you a new webrev.
>> Sound good.
> I did some measurements with 100k regions, and I think we can just
> remove the flag. There is always logging available (gc+mark+stats) that
> shows cache hit ratio to diagnose issues.
>
>>>> ---
>>>> src/hotspot/share/gc/g1/g1RegionMarkStatsCache.hpp
>>>>
>>>> In G1RegionMarkStatsCacheEntry _region_idx is set to 0 when
>>>> clearing. I don't see a problem with it right now, but it feels a
>>>> bit wrong since index 0 is a valid region index. Maybe we could
>>>> use another cleared marker.
>>> Unfortunately there is none (one could use something like -1, but
>>> that one is valid too), and it seems a waste of memory to reserve
>>> more space for it.
>>>
>>> I see your point that this is a bit ugly, but there should never be
>>> any issue because of the initialization of the value part of an
>>> entry with zero. It simply has no impact on totals if you add zero.
>>>
>>> The code could initialize the cache with region indices from 0 to
>>> cache size - 1, which are valid values given the hash function,
>>> what do you think?
>>>
>>> However that would slow down clearing a little (the compiler will
>>> just compile it to a memset() in the loop right now).
>> You are correct that -1 is a valid index, but we seldom see heaps
>> with so many regions. I'm fine with leaving it as is, if you dislike
>> using -1 and if it will slow down clearing.
> Just a bit. It seems as bad to use -1 or another random value, so I
> kept it.
>
> Webrevs:
> http://cr.openjdk.java.net/~tschatzl/8197850/webrev.0_to_1/index.html
> (diff)
> http://cr.openjdk.java.net/~tschatzl/8197850/webrev.1/index.html (full)
>
> Testing:
> This change ran, with some changes to 8180415 due to internal comments
> (I will post in a few minutes) through hs-tier 1-5 over night.
>
> Thanks,
>    Thomas