Request for review JDK-8165674 - G1CMMarkStack::out_of_memory possibly redundant
thomas.schatzl at oracle.com
Tue Mar 7 12:00:06 UTC 2017
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
> 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)
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
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.
More information about the hotspot-gc-dev