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

[Thomas Schatzl]

Hi again,

On Fri, 2017-07-14 at 15:18 +0200, Aleksey Shipilev wrote:
> On 07/14/2017 02:20 PM, Thomas Schatzl wrote:
> > 
> > Not completely sure what you are referring to, but I split some
> > very
> > long asserts across lines.
> Yes, I meant that, sorry for not being clear. Any webrev that
> requires me to scroll horizontally on 2560-pixel wide screen triggers
> me!

I noticed that too :)

> > > 
> > > *) So, mark_reference_grey used to be called from
> > > G1CMSATBBufferClosure on
> > > objects below TAMS, but now it would get called on objects past
> > > TAMS
> > > too?
> > CMTask::make_reference_grey() now calls
> > G1ConcurrentMark::mark_in_next_bitmap(), not
> > ConcurrentMark::par_mark()
> > which does not exist any more:
> > G1ConcurrentMark::mark_in_next_bitmap()
> > in the first check filters out marking attempts above nTAMS
> > (g1ConcurrentMark.inline.hpp:47 now), returning false, which makes
> > make_reference_grey() exit immediately in that case. This seems to
> > achieve the same effect.
> Ah, I missed that part! I agree this part is fine then.
> 
> > 
> > If you are worried whether there is a performance difference
> > because maybe now we do more work in some cases, all paths
> > previously leading to the former G1ConcurrentMark::par_mark() did
> > the nTAMS check in one way or another already (of course in
> > inconsistent fashion) so there should be no change here.
> No, I am not worried. SATB-heavy workloads have problems way beyond
> bitmap marking :)
> 
> > 
> > New webrevs:
> > http://cr.openjdk.java.net/~tschatzl/8184348/webrev.1/ (full)
> Looks good to me.

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)

Thanks,
  Thomas