RFR (S/M): 8178105: Switch mark bitmaps during Remark

[Thomas Schatzl]

Hi Sangheon, Stefan,

  thanks for your reviews.

Thomas

On Wed, 2018-04-11 at 21:47 -0700, sangheon.kim wrote:
> Hi Thomas,
> 
> On 04/04/2018 05:42 AM, Thomas Schatzl wrote:
> > Hi all,
> > 
> >    while looking at some parallelizing work in this area I noticed
> > that one assert in the rebuild remsets code that has been changed
> > in this change already needs an update too.
> > It uses a wrong value for the informational text, which makes it
> > really confusing.
> > 
> > Since this change touches this code already, I would like to amend
> > this change with this small fix:
> > 
> > Webrev:
> > http://cr.openjdk.java.net/~tschatzl/8178105/webrev.2_to_3/ (diff)
> > http://cr.openjdk.java.net/~tschatzl/8178105/webrev.3/ (full)
> 
> Webrev.3 looks good.
> 
> Thanks,
> Sangheon
> 
> 
> > 
> > Sorry for the trouble.
> > 
> > Thanks,
> >    Thomas
> > 
> > On Tue, 2018-04-03 at 17:00 +0200, Stefan Johansson wrote:
> > > On 2018-04-03 16:55, Thomas Schatzl wrote:
> > > > Hi,
> > > > 
> > > >     thanks for your patience...
> > > > 
> > > > On Tue, 2018-04-03 at 14:49 +0200, Stefan Johansson wrote:
> > > > > On 2018-04-03 14:25, Thomas Schatzl wrote:
> > > > > > Hi Stefan,
> > > > > > 
> > > > > > On Tue, 2018-04-03 at 12:58 +0200, Stefan Johansson wrote:
> > > > > > > Hi Thomas,
> > > > > > > 
> > > > > > > On 2018-03-29 16:35, Thomas Schatzl wrote:
> > > > > > > > Hi all,
> > > > > > > > [...]
> > > > > > > > 
> > > > > > > > CR:
> > > > > > > > https://bugs.openjdk.java.net/browse/JDK-8178105
> > > > > > > > Webrev:
> > > > > > > > http://cr.openjdk.java.net/~tschatzl/8178105/
> > > > > > > 
> > > > > > >     Looks good, but a few comments:
> > > > > > > 
> > > > 
> > > > [...]
> > > > > > I added a different kind of check that verifies what you
> > > > > > probably
> > > > > > thought of, and that is that the number of words to add to
> > > > > > the
> > > > > > current region must always be larger than zero. If it is
> > > > > > zero,
> > > > > > there is something really wrong.
> > > > > 
> > > > > Exactly, that's what I wanted. But I don't see that check
> > > > > now,
> > > > > just
> > > > > having a assert(marked_words != 0, "") on row 1032 would be
> > > > > enough.
> > > > 
> > > > My fault. I did not upload the latest version of the change :(
> > > > It
> > > > is up
> > > > now, the only difference to previous is line
> > > > 
> > > > 1036       assert(words_to_add > 0, "Out of space to distribute
> > > > before
> > > > end of humongous object in region %u (starts %u)", i,
> > > > region_idx);
> > > > 
> > > > But there is a new webrev anyway... in the 1_to_2 webrev it is
> > > > line
> > > > 1033
> > > > 
> > > > > > > 1047       log_trace(gc)("Added " SIZE_FORMAT " words to
> > > > > > > region
> > > > > > > %u
> > > > > > > (%s)", marked_words, region_idx, hr->get_type_str());
> > > > > > > I think the logging should use at least one more tag,
> > > > > > > maybe
> > > > > > > "region" or "marking", but you probably know what makes
> > > > > > > most
> > > > > > > sense in this area. I also think it would be nice if we
> > > > > > > got
> > > > > > > this
> > > > > > > same log-line for all regions. Now we get this for
> > > > > > > "normal"
> > > > > > > regions, but we get "Distributing..." for humongous start
> > > > > > > regions
> > > > > > > and "Not Added..." for humongous continuous. So I would
> > > > > > > suggest
> > > > > > > adding the same log-line  to the distribute-function
> > > > > > > after
> > > > > > > line
> > > > > > > 1034.
> > > > > > 
> > > > > > Done.
> > > > > 
> > > > > Sorry for being picky but the current solution won't print
> > > > > anything
> > > > > for continuous humongous. Adding the log to
> > > > > distribute_marked_bytes()
> > > > > would solve this and we would still get info on region type
> > > > > so I
> > > > > see
> > > > > no need for:
> > > > 
> > > > Okay. I think I got your intention right this time :)
> > > > 
> > > > > 1052         log_trace(gc, marking)("Adding " SIZE_FORMAT "
> > > > > words
> > > > > to
> > > > > humongous start region %u (%s), word size %d (%f)",
> > > > > 1053                                marked_words, region_idx,
> > > > > hr->get_type_str(),
> > > > > 1054                                oop(hr->bottom())-
> > > > > >size(),
> > > > > (double)oop(hr->bottom())->size() / HeapRegion::GrainWords);
> > > > 
> > > > [...]
> > > > > > All fixed.
> > > > > > 
> > > > > > Webrevs:
> > > > > > http://cr.openjdk.java.net/~tschatzl/8178105/webrev.0_to_1/
> > > > > > (diff)
> > > > > > http://cr.openjdk.java.net/~tschatzl/8178105/webrev.1/
> > > > > > (full)
> > > > > 
> > > > > Apart from the small things above this looks great.
> > > > 
> > > > http://cr.openjdk.java.net/~tschatzl/8178105/webrev.1_to_2/
> > > > (diff)
> > > > http://cr.openjdk.java.net/~tschatzl/8178105/webrev.2/ (full)
> > > 
> > > Looks good!
> > > 
> > > Thanks,
> > > StefanJ
> > > 
> > > > Ran gc/g1 tests successfully with that change.
> > > > 
> > > > Thanks,
> > > >     Thomas
> > > > 
> 
>