Quantcast

Request for review JDK-8165674 - G1CMMarkStack::out_of_memory possibly redundant

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

Request for review JDK-8165674 - G1CMMarkStack::out_of_memory possibly redundant

Alexander Harlap

Please review change for JDK-8165674 - G1CMMarkStack::out_of_memory possibly redundant.

Change is located at http://cr.openjdk.java.net/~aharlap/8165674/webrev.01/

JDK-8165674 states that the G1CMMarkStack::_out_of_memory member may be redundant,
just mirroring the value of G1ConcurrentMark::_has_overflown.

Attached is proof of redundancy (based on some data flow analysis)

Alex


ProofOfRedundancy.pdf (44K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Request for review JDK-8165674 - G1CMMarkStack::out_of_memory possibly redundant

Thomas Schatzl
Hi Alex,

On Wed, 2017-03-01 at 16:03 -0500, Alexander Harlap wrote:
> Please review change for JDK-8165674 - G1CMMarkStack::out_of_memory
> possibly redundant.
> Change is located at http://cr.openjdk.java.net/~aharlap/8165674/webr
> ev.01/
> JDK-8165674 states that the G1CMMarkStack::_out_of_memory member may
> be redundant,
> just mirroring the value of G1ConcurrentMark::_has_overflown.
> Attached is proof of redundancy (based on some data flow analysis)
> Alex

  I looked through the change and I think in general the analysis and
the motivation for the change are good.

There seems to be some pre-existing issue with the possibility of some
threads going into the overflow protocol and other threads clearing the
overflow flag in the meantime, and when the decision whether to go into
the sync barriers is taken, the some threads may not enter it until
they notice an overflow again.

I am not sure yet whether this can cause marking to be effectively hung
(but tend to "yes") , as if that happens, some threads may be waiting
in the sync barrier while others already finished their work (ie. there
is no guarantee that the latter will get an overflow again afaics).

In any case this is a pre-existing issue, and does not seem to make the
situation worse than it already is.

Overall I would however prefer if the overflow state would be
summarized in the _has_overflown flag in G1ConcurrentMark, not in
G1CMMarkStack::_out_of_memory. I.e. remove the _out_of_memory variable
keeping G1ConcurrentMark::_has_overflown.

While this sounds like some nitpick, the flag is ultimately only used
by G1ConcurrentMark, but now somehow stored in G1CMMarkStack.
G1CMMarkStack never uses it actually - it always reports its current
mark stack state when calling par_push_chunk() anyway without the need
for this flag. This seems wrong.

Maybe somebody else has a different opinion.

Thanks,
  Thomas

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

Re: Request for review JDK-8165674 - G1CMMarkStack::out_of_memory possibly redundant

Alexander Harlap
Happy to get  review.

Changed to use

G1ConcurrentMark::_has_overflown


Revised change is here:

http://cr.openjdk.java.net/~aharlap/8165674/webrev.02/


On 3/7/2017 7:00 AM, Thomas Schatzl wrote:

> Hi Alex,
>
> On Wed, 2017-03-01 at 16:03 -0500, Alexander Harlap wrote:
>> Please review change for JDK-8165674 - G1CMMarkStack::out_of_memory
>> possibly redundant.
>> Change is located at http://cr.openjdk.java.net/~aharlap/8165674/webr
>> ev.01/
>> JDK-8165674 states that the G1CMMarkStack::_out_of_memory member may
>> be redundant,
>> just mirroring the value of G1ConcurrentMark::_has_overflown.
>> Attached is proof of redundancy (based on some data flow analysis)
>> Alex
>    I looked through the change and I think in general the analysis and
> the motivation for the change are good.
>
> There seems to be some pre-existing issue with the possibility of some
> threads going into the overflow protocol and other threads clearing the
> overflow flag in the meantime, and when the decision whether to go into
> the sync barriers is taken, the some threads may not enter it until
> they notice an overflow again.
>
> I am not sure yet whether this can cause marking to be effectively hung
> (but tend to "yes") , as if that happens, some threads may be waiting
> in the sync barrier while others already finished their work (ie. there
> is no guarantee that the latter will get an overflow again afaics).
>
> In any case this is a pre-existing issue, and does not seem to make the
> situation worse than it already is.
>
> Overall I would however prefer if the overflow state would be
> summarized in the _has_overflown flag in G1ConcurrentMark, not in
> G1CMMarkStack::_out_of_memory. I.e. remove the _out_of_memory variable
> keeping G1ConcurrentMark::_has_overflown.
>
> While this sounds like some nitpick, the flag is ultimately only used
> by G1ConcurrentMark, but now somehow stored in G1CMMarkStack.
> G1CMMarkStack never uses it actually - it always reports its current
> mark stack state when calling par_push_chunk() anyway without the need
> for this flag. This seems wrong.
Agree.
It is always better to call things by their own name.
> Maybe somebody else has a different opinion.
>
> Thanks,
>    Thomas
>

Thank you,
Alex
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Request for review JDK-8165674 - G1CMMarkStack::out_of_memory possibly redundant

Kim Barrett
> On Mar 7, 2017, at 2:26 PM, Alexander Harlap <[hidden email]> wrote:
>
> Happy to get  review.
>
> Changed to use
>
> G1ConcurrentMark::_has_overflown

I like this better.  I'd intended to make comments similar promises but he got there first.

> Revised change is here:
>
> http://cr.openjdk.java.net/~aharlap/8165674/webrev.02/

One very minor and nit:

src/share/vm/gc/g1/g1ConcurrentMark.cpp
1746             "Mark stack should be empty (unless it is out of memory)");

The assert message should refer to “overflow" rather than "out of memory”

Looks good otherwise.
I don't need a new webrev for a change there.

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

Re: Request for review JDK-8165674 - G1CMMarkStack::out_of_memory possibly redundant

Alexander Harlap
Hi Kim,

How about:

"Mark stack should be empty (unless it has overflown)"

Alex


On 3/7/2017 3:05 PM, Kim Barrett wrote:

>> On Mar 7, 2017, at 2:26 PM, Alexander Harlap <[hidden email]> wrote:
>>
>> Happy to get  review.
>>
>> Changed to use
>>
>> G1ConcurrentMark::_has_overflown
> I like this better.  I'd intended to make comments similar promises but he got there first.
>
>> Revised change is here:
>>
>> http://cr.openjdk.java.net/~aharlap/8165674/webrev.02/
> One very minor and nit:
>
> src/share/vm/gc/g1/g1ConcurrentMark.cpp
> 1746             "Mark stack should be empty (unless it is out of memory)");
>
> The assert message should refer to “overflow" rather than "out of memory”
>
> Looks good otherwise.
> I don't need a new webrev for a change there.
>

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

Re: Request for review JDK-8165674 - G1CMMarkStack::out_of_memory possibly redundant

Kim Barrett
> On Mar 7, 2017, at 3:46 PM, Alexander Harlap <[hidden email]> wrote:
>
> Hi Kim,
>
> How about:
>
> "Mark stack should be empty (unless it has overflown)”

Sure, that looks good.

>
> Alex
>
>
> On 3/7/2017 3:05 PM, Kim Barrett wrote:
>>> On Mar 7, 2017, at 2:26 PM, Alexander Harlap <[hidden email]> wrote:
>>>
>>> Happy to get  review.
>>>
>>> Changed to use
>>>
>>> G1ConcurrentMark::_has_overflown
>> I like this better.  I'd intended to make comments similar promises but he got there first.
>>
>>> Revised change is here:
>>>
>>> http://cr.openjdk.java.net/~aharlap/8165674/webrev.02/
>> One very minor and nit:
>>
>> src/share/vm/gc/g1/g1ConcurrentMark.cpp
>> 1746             "Mark stack should be empty (unless it is out of memory)");
>>
>> The assert message should refer to “overflow" rather than "out of memory”
>>
>> Looks good otherwise.
>> I don't need a new webrev for a change there.


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

Re: Request for review JDK-8165674 - G1CMMarkStack::out_of_memory possibly redundant

Thomas Schatzl
Hi Alex,

On Tue, 2017-03-07 at 18:04 -0500, Kim Barrett wrote:

> >
> > On Mar 7, 2017, at 3:46 PM, Alexander Harlap <alexander.harlap@orac
> > le.com> wrote:
> >
> > Hi Kim,
> >
> > How about:
> >
> > "Mark stack should be empty (unless it has overflown)”
> Sure, that looks good.

  webrev.02 looks good with the suggested change to the log message.

Thanks,
  Thomas


Loading...