RFR (S/M): 8184348: Merge G1ConcurrentMark::par_mark() and G1ConcurrentMark::grayRoot()

[Thomas Schatzl]

Hi Aleksey,

On Mon, 2017-07-17 at 09:23 +0200, Aleksey Shipilev wrote:
> On 07/14/2017 04:34 PM, Thomas Schatzl wrote:
> > 
> > Thanks. Unfortunately, after re-appyling and fixing other changes
> > based
> > on this one I noticed that I missed one opportunity to refactor in
> > G1CMTask::deal_with_reference(). I would like to add this to this
> > changeset still... sorry.
> > 
> > There is some note about some perf optimization that mentions that
> > it
> > is advantagous to do the nTAMS check before determining the heap
> > region; however I do not think this is an issue.
> > 
> > Quickly comparing runs of a fairly large and reference-intensive
> > workload (BigRAMTester with 20g heap e.g. attached to JDK-8152438),
> > marking cycles with the latest webrev.2 are at least as fast as
> > without
> > any of this RFR's changes.
> > 
> > New webrevs:
> > http://cr.openjdk.java.net/~tschatzl/8184348/webrev.1_to_2 (diff)
> > http://cr.openjdk.java.net/~tschatzl/8184348/webrev.2 (full)
> Looks good.
> 
> I wonder what this was about in the old code:
> 
>  187   if (_g1h->is_in_g1_reserved(objAddr)) {
> 
> New code properly asserts the object is in reserved. Did we ever had
> oops stored
> outside of reserved? That would be surprising!

  the reference can be NULL here. The is_in_g1_reserved() check also
filters those, in a bit of a crude way. So I changed this to an
explicit NULL check, and let it run into the assert (in
ConcurrentMark::mark_in_next_bitmap()) in other cases.

I have not seen any issues in my testing of the changes I extracted
these from. There should obviously no oops referencing anything outside
of the heap.

Thanks for your review.

Thanks,
  Thomas