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

[Thomas Schatzl]

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.

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

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

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

>   61 // logical and.
> This seems incomplete sentence?

This refers to the logical and ("&") operation and seems correct.

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

Thanks,
  Thomas