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

Tue Mar 13 21:39:21 UTC 2018

Hi Thomas,

On 03/13/2018 06:52 AM, Thomas Schatzl wrote:
> Hi Stefan,
>
> On Wed, 2018-03-07 at 11:12 +0100, Stefan Johansson wrote:
>> Thanks Thomas,
>>
>> This looks good to me, reviewed.
>    thanks for your review; unfortunately rebasing the changes to latest
> caused some additional work.
>
> In this particular case, contents of g1RegionMarkStatsCache had to be
> distributed to .cpp and .inline.hpp files (recent .inline.hpp include
> fixes), and I preemptively removed the use of one VALUE_OBJ_CLASS_SPEC.
>
> Webrevs:
> http://cr.openjdk.java.net/~tschatzl/8197850/webrev.1_to_2/index.html
> (diff)
> http://cr.openjdk.java.net/~tschatzl/8197850/webrev.2/index.html (full)
Looks good, I have several minor comments.

===============================
src/hotspot/share/gc/g1/g1CardLiveData.cpp

292 assert(!hr->is_old() || marked_bytes == 
(_cm->liveness(hr->hrm_index()) * HeapWordSize),

Don't we need to print assert message for hr->is_old()?

===============================
src/hotspot/share/gc/g1/g1CollectedHeap.cpp

4215 // Any worker id is fine here as we are in the VM thread and 
single-threaded.

Would be better to add 'valid'?
// Any _valid_ worker id ...

===============================
src/hotspot/share/gc/g1/g1ConcurrentMark.cpp

1840 log_debug(gc, stats)("Mark stats cache hits " SIZE_FORMAT " misses 
" SIZE_FORMAT " ratio %1.3lf",

gc+stats is not registered at logPrefix.hpp, so currently GC ID is not 
printed. I guess this is not intensional?

===============================
src/hotspot/share/gc/g1/g1RegionMarkStatsCache.hpp

25 #ifndef SHARE_VM_GC_G1_G1REGIONMARKSTATSCACHE_HPP

I'm okay leaving as is but we don't have 'VM' directory anymore. At some 
point we would want to clean up these stuffs at once?

   61 // logical and.

This seems incomplete sentence?

Thanks,
Sangheon

>
> Testing:
> hs-tier 1-5
>
> Thanks,
>    Thomas
>
>
>
>> 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.h
>>>>>>> tml
>>>>>> 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.ht
>>> ml
>>> (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
>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20180313/3a5b34b3/attachment.htm>