RFR (M/L): 8247928: Refactor G1ConcurrentMarkThread for mark abort (JDK-8240556)

[Kim Barrett]

> On Sep 3, 2020, at 6:07 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> On 03.09.20 09:24, Kim Barrett wrote:
>> It seems backward that the phase functions return true to indicate aborted,
>> overflow, or whatever, and return false for successful completion. And
>> there's no mention of what the return values are in the description in the
>> header file.
> 
> I think the code is easier to read without the need to always negate the result. Ie. in the following example
> 
> if (!phase1) return;
> if (!phase2) return;
> 
> the "!" detracts imho from reading the steps.
> 
> Another option I can see could be a macro hiding all this, e.g.
> 
> EXEC(phase1);
> EXEC(phase2);
> 
> but I kind of dislike that even more.
> 
> Added documentation.

I agree a macro is not the way to go here. I'm less bothered by the need for
lots of preceeding "!" than you are, but I certainly see your point. So I'm
okay with the code as is, now that there's a comment to set expectations.

> http://cr.openjdk.java.net/~tschatzl/8247928/webrev.1_to_2 (diff)
> http://cr.openjdk.java.net/~tschatzl/8247928/webrev.2 (full)

I like the overall approach a lot.  Thanks for doing this cleanup.

Although I have a number of comments, there's nothing major here.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1ConcurrentMarkThread.cpp 
 190   uint iter = 1;
 191   while (true) {
...
 209     log_info(gc, marking)("Concurrent Mark Restart for Mark Stack Overflow (iteration #%u)",
 210                           iter++);

I think that's the only use of iter, and that it's not used after the loop.
How about instead

  for (uint iter = 1; true; ++iter) {

and remove the increment in the logging statement.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1ConcurrentMarkThread.cpp 
 191   while (true) {
...
 211   }
 212 
 213   Ticks mark_end = Ticks::now();
 214   log_info(gc, marking)("Concurrent Mark (%.3fs, %.3fs) %.3fms",
 215                         mark_start.seconds(), mark_end.seconds(),
 216                         (mark_end - mark_start).seconds() * 1000.0);

This is a change to the logging. Some consequences I like, others I'm not so
sure about.  You talked about some of this in the RFR description, but I
want to go through it carefully here.

- This now includes the remark pause in the time, where previously that
seemed to be intentionally excluded. I don't know the rationale for that
exclusion, but it was pretty clearly intentional since it intentionally (per
comment) contorted the code somewhat. So it's hard to decide whether this is
okay. The remark time could be excluded though by capturing the mark_end
time just before calling subphase_remark().

- It only does this logging once, rather than on each iteration.  That seems
good; as you pointed out in the RFR, multiples are pretty likely handled
wrong by some uses.

- This logging no longer happens if marking is aborted by remark.  That's
now consistent with other abort cases, so probably an improvement.

------------------------------------------------------------------------------ 
src/hotspot/share/gc/g1/g1ConcurrentMarkThread.cpp 
 137   while (!should_terminate()) {
 138     if (wait_for_next_cycle()) {
 139       break;
 140     }

This could instead just be

  while (!wait_for_next_cycle()) { ... 

But I already thought the sense of the wait_for_next_cycle result was
backward, and this makes an even stronger case for that. I think better
would be to reverse the sense of the result and use

  while (wait_for_next_cycle()) { ... 

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1ConcurrentMarkThread.hpp 
  65   bool phase_concurrent_cycle_start();

Unused and undefined.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1ConcurrentMarkThread.hpp 
  63   // ConcurrentGCBreakpoints must not be placed before the the root
  64   // region scan phase too for this reason.

This comment seems misplaced here. I think it belongs in the implementation
of full_concurrent_cycle_do, in conjunction with the comment about not
returning before the scan root regions phase.

I think the rest of the comment about STS &etc also seems misplaced here and
should be similarly moved (more or less where it used to be).

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1ConcurrentMarkThread.hpp 
  66   bool phase_clear_cld_claimed_marks();

The result of this function is unused, because of the restriction against
returning before the scan root regions phase. I think I'd prefer it return
void, to make it clear there's nothing to be done with it. Even though that
makes its signature different from all the phase functions.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1ConcurrentMarkThread.hpp 
  81   // Perform a full concurrent cycle.

This comment seems kind of vacuous. The function also in some sense doesn't
even do that, since it doesn't include the start/end parts.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1ConcurrentMarkThread.hpp 
  70   bool mark_loop_needs_restart() const;

This one seems out of order or otherwise misplaced here.  Seems like it
should be after subphase_remark.  Or maybe it shouldn't be in the phase
function section at all.

------------------------------------------------------------------------------