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