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

[Alexander Harlap]

On 4/26/2017 5:15 PM, Kim Barrett wrote:
>> 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.
I like this idea - to eliminate should_expand tracking. It really 
straightens mark stack expansion mechanism.
>
Revised change is here: 
http://cr.openjdk.java.net/~aharlap/8065402/webrev.01/
I retested change.

Alex