RFR [M][7/7]: 8197932: Better split work in rebuild remembered sets phase

[Thomas Schatzl]

Hi,

On Tue, 2018-03-27 at 10:50 -0700, sangheon.kim wrote:
> Hi Thomas,
> 
> Finally the last piece! :)
> 

  yes :)

Unfortunately there are already a few more changes out for review,
although much smaller in scope and complexity.

[...]
> > 
> > New webrev:
> > http://cr.openjdk.java.net/~tschatzl/8197932/webrev.2_to_3 (diff)
> > http://cr.openjdk.java.net/~tschatzl/8197932/webrev.3 (full)
> 
> webrev.3 looks good!
> Minor nits below.
> 
> =============================
> src/hotspot/share/gc/g1/g1RemSet.cpp
>   805       void move_to_next() {
>   810       oop next() const {
> (Ignore if you don't agree)
> - I like the idea of using iterator here. But for me, current naming 
> seems misleading. Current move_to_next() seems next(). And something 
> else for current next(). to_oop()?

I would like to keep it as is until we in the team figured out how to
do iterators in hotspot, and then change as needed.

Apart from the naming this style is 1:1 compatible to STL iterators
(i.e. move_to_next() -> "++" operator, next() -> * operator) (which
although I expect us to use them in the future, I have no particular
preference for any style), so I chose that.

> =============================
> src/hotspot/share/gc/g1/g1RemSetTrackingPolicy.cpp
>    37   // All non-young and non-closed archive regions need to be 
> scanned for references;
>    38   // At every gc we gather references to other regions in
> young, 
> and closed archive
>    39   // regions by definition do not have references going
> outside 
> the closed archive.
> - Add some explanation about r->is_free() case as well?

I will fix this before pushing.

> =============================
> src/hotspot/share/gc/g1/g1_globals.hpp
>   261           range(4 * K, 32 * 
> M)                                              \
> - Just question. Why the min is 4K?

4k is like 1024 references max. This seemed to be a small enough
increment to not cause too much safepointing delay for the smallest
machine we are going to run with G1.

I am going to push this change as is - your review did not seem to pose
any significant issue. Otherwise there is enough opportunity to fix
this immediately.

Thanks,
  Thomas