Request for review JDK-8065402, , G1 does not expand marking stack when mark stack overflow happens during concurrent marking

[Kim Barrett]

> On Mar 24, 2017, at 9:59 AM, Alexander Harlap <alexander.harlap at oracle.com> wrote:
> 
> Sorry forgot link to the actual change:
> 
> http://cr.openjdk.java.net/~aharlap/8065402/webrev.00/
> Alex
> 
> -------- Forwarded Message --------
> Subject:	Request for review JDK-8065402, , G1 does not expand marking stack when mark stack overflow happens during concurrent marking
> Date:	Thu, 23 Mar 2017 16:47:43 -0400
> From:	Alexander Harlap <alexander.harlap at oracle.com>
> Organization:	Oracle Corporation
> To:	hotspot-gc-dev at openjdk.java.net <hotspot-gc-dev at openjdk.java.net>
> 
> Please review change for JDK-8065402 - G1 does not expand marking stack when mark stack overflow happens during concurrent marking.
> 
> I added call to the G1CMMarkStack::expand() from G1ConcurrentMark::mark_from_roots().
> 
> I added comment and attachment to the JDK-8065402 with an explanation of testing.
> 
> Thank you,
> 
> Alex

There's some pretty convoluted code surrounding this.  I needed
several hours of serious study to convince myself the proposed change
would do anything at all, let alone solve the intended problem.
(Maybe I'm just slow, but the number of state flags and their
interactions seem quite, um, dense, yeah that's a good word for it.)

I think the proposed change would be improved by also

(a) changing set_should_expand() to not take an argument, instead
always setting the underlying member to true, and

(b) changing reset_marking_state, which is the only caller of
set_should_expand(), from

  _global_mark_stack.set_should_expand(has_overflown());

to

  if (has_overflown()) {
    _global_mark_stack.set_should_expand();
  }

Or perhas even better, eliminate the whole should_expand tracking and
instead change reset_marking_state to

  if (has_overflown()) {
    _global_mark_stack.expand();
  }

If should_expand is eliminated then we no longer need either the new
or the pre-exisinting "if should_expand then expand" code chunks.

The benefit of eliminating should_expand and eagerly expanding is that
an overflow during concurrent marking is responded to quickly, rather
than possibly overflowing and restarting multiple times before finally
completing and only then expanding.  Of course, eager expansion could
end up using more memory, but I'm inclined to agree with one of
Thomas's comments in the CR that this probably isn't very important
here.  And getting rid of one of those state flags would be nice.