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

[Stefan Johansson]

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

Thanks,
Stefan

>
> Thanks,
>    Thomas
>