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

[Thomas Schatzl]

Hi Aleksey,

  thanks for looking into this.

On Fri, 2017-07-14 at 13:25 +0200, Aleksey Shipilev wrote:
> On 07/14/2017 01:19 PM, Thomas Schatzl wrote:
> > 
> > Webrev:
> > http://cr.openjdk.java.net/~tschatzl/8184348/webrev/
> *) I'd probably split the assert with newlines. Makes webrevs tidier!

Not completely sure what you are referring to, but I split some very
long asserts across lines.

As for the asserts themselves, I tend to group them together in blocks
separate from actual code with newlines. But there are (often) no
newlines between subsequent asserts.

> *) This is not needed, because par_mark already has the optimistic
> check, down
> below in Bitmap::par_set_bit?
> 
>   54   // Dirty read to avoid CAS.
>   55   if (_nextMarkBitMap->is_marked(obj_addr)) {
>   56     return false;
>   57   }

Thanks for catching this, I simply copied this check from the former
grayRoot() method... :)

> *) 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.

See the comment in g1ConcurrentMark.inline.hpp:51 too, which refers to
that issue.

(The documentation of G1ConcurrentMark::mark_in_next_bitmap() also
mentions that: "Mark the given object on the next bitmap if it is below
nTAMS")

Indeed, I tripped over this when trying to refactor this, and I did do
runs of some gc stress applications with verification on (actually that
issue is also caught during check-in by jprt tests). :)

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.

There may be some further optimizations to be done here (like for
marking during initial mark pause, as e.g. survivor region nTAMS ==
bottom so we will never put a mark for them), but that, unless like the
one with the duplicated marking check which is dead simple, I would
prefer to do in an extra CR. But please feel free to mention them, I
may pick them up immediately afterwards :)

> Doesn't G1 verify there are no bits set in bitmap past TAMS
> (G1HeapVerifier::verify_no_bits_over_tams)?

It does, but as mentioned above, these mark attempts past nTAMS should
be filtered out as expected.

New webrevs:
http://cr.openjdk.java.net/~tschatzl/8184348/webrev.1/ (full)
http://cr.openjdk.java.net/~tschatzl/8184348/webrev.0_to_1/ (diff)

Thanks a lot,
  Thomas