Quantcast

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

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Alexander Harlap

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


 

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Alexander Harlap

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 [hidden email]
Organization: Oracle Corporation
To: [hidden email] [hidden email]


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


 

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: 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 <[hidden email]> 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 <[hidden email]>
> Organization: Oracle Corporation
> To: [hidden email] <[hidden email]>
>
> 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.


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: 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 <[hidden email]> 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 <[hidden email]>
>> Organization: Oracle Corporation
>> To: [hidden email] <[hidden email]>
>>
>> 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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Kim Barrett
> On Apr 27, 2017, at 4:39 PM, Alexander Harlap <[hidden email]> wrote:
> 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.

Looks good.

Loading...