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

[sangheon.kim]

Hi Thomas,

On 03/14/2018 12:55 AM, Thomas Schatzl wrote:
> Hi,
>
> On Tue, 2018-03-13 at 14:39 -0700, sangheon.kim wrote:
>> 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,
>>>>
>>>>
> [...]
>>> 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.ht
>>> ml
>>> (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()?
> This is a pre-existing bug (or feature, depending on your POV) - for
> humongous objects the regions are considered to be always full (by
> definition), while obviously if there is only one live object in them,
> they do not always fill all regions the humongous object is in.
>
> Also, the marking attributes all of the humongous object size to the
> starts region. There is need to distribute that.
>
> So the assert does not hold for other type of regions at the moment.
>
> This will be fixed with JDK-8178105 as far as my plan goes.
Okay, thanks.

>
>> ===============================
>> 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 ...
> Done.
OK.

>
>> ===============================
>> 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?
> This is a pre-existing issue. I filed JDK-8199598.
Thanks for filing a new one.

>
>> ===============================
>> 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?
> Yes.
OK

>
>>    61 // logical and.
>> This seems incomplete sentence?
> This refers to the logical and ("&") operation and seems correct.
My bad, sorry.

>
> New webrevs:
> http://cr.openjdk.java.net/~tschatzl/8197850/webrev.2_to_3/ (diff)
> http://cr.openjdk.java.net/~tschatzl/8197850/webrev.3/ (full)
Looks good to me.

Thanks,
Sangheon


>
> Thanks,
>    Thomas
>