RFR(s): 8185278: TestGreyReclaimedHumongousObjects.java fails guarantee(index != trim_index(_head_index + 1)) failed: should not go past head

Thomas Schatzl thomas.schatzl at oracle.com
Wed Oct 18 10:41:24 UTC 2017


Hi Sangheon,

On Thu, 2017-10-12 at 16:19 -0700, sangheon.kim wrote:
> Hi all,
> 
> Could I have some reviews for fixing G1 MMU concurrency problem?
> 

thanks :)

> * Background
> 
[...]
> * Analysis
> Considering G1MMUTrackerQueue::_head_index and _tail_index couldn't
> be same with _no_entries==QueueLength, the data is corrupted.
> G1MMUTrackerQueue::add_pause() is only called from VMThread but 
> G1MMUTrackerQueue::when_sec() which internally calls 
> remove_expired_entries() can be called from ConcurrentMarkThread 
> concurrently. And when_sec() is guarded by MMUTracker_lock while 
> add_pause() is not guarded.
> 
> * Proposal
> Instead of adding MMUTracker_lock at add_pause(), it would be better
> to use SuspendibleThreadSetJoiner as there are 2 additional benefits.
> 1. If there is running young gc but not yet updated its gc time for
> MMU, its gc time will be reflected at this MMU calculation as STS
> will suspend ConcurrentMarkThread.
> 2. ConcurrentMarkThread will not sleep if there is young gc which
> makes MMU more accurate.
> 
> CR: https://bugs.openjdk.java.net/browse/JDK-8185278
> Webrev: http://cr.openjdk.java.net/~sangheki/8185278/webrev.0/
> Testing: JPRT
> 

Some minor comments:

 - the comment at concurrentMarkThread.cpp:144 seems to be misplaced.
Imho it should be above the SuspendibleThreadSetJoiner call, not as
method comment.

 - in that comment: "If there is running young gc but not yet updated
MMU, its gc time will be reflected at this MMU calculation." -> "If
currently a gc is running, but it has not yet updated the MMU, we will
not forget to consider that pause in the MMU calculation"

 - in that comment, not sure what the third option refers to, maybe the
CPU usage of the ConcurrentMarkThread?

 - concurrentMarkThread.cpp:131: please move the method description (is
it?) to the method definition. Further I would prefer if it read
something like: "Delay marking to meet MMU" only which sums up what it
does. The other part of that comment about that we can schedule pauses
flexibly would probably better at the call site of the method but could
be skipped as well. This comment is a rather weak suggestion, feel free
to ignore it completely.

Thanks,
  Thomas




More information about the hotspot-gc-dev mailing list