RFR: 8146409: TestPromotionFailedEventWithParallelScavenge.java failed with assert(_time_stamps != __null) failed: Sanity

[Thomas Schatzl]

Hi,

On Tue, 2016-01-05 at 14:23 +0100, David Lindholm wrote:
> Hi,
> 
> Please review the following patch that fixes 8146409. Since
> _time_stamps 
> is now lazily allocated we need to check that it is != null in 
> GCTaskThread::print_task_time_stamps() instead of asserting.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8146409
> Webrev: http://cr.openjdk.java.net/~david/JDK-8146409/webrev.00/
> 

  looks good.

While looking at this code, I noticed that there is a potential race of
the assert(log_is_enabled(), ...) within print_task_time_stamps().

It looks as if logging can be turned on or off at any time, so while in
GCTaskManager::print_task_time_stamps() the code

if (!log_is_enabled(Debug, gc, task, time)) {
  return;
}

may succeed but the assert in GCTaskThread::print_task_time_stamps()
fail.

There seems to be another (pre-existing) issue with the time stamping,
in gcTaskThread.cpp:164 there is this comment "Update the index after
we have set up the entry correctly since
GCTaskThread::print_task_time_stamps() may read this value
concurrently".

I think it is not sufficient to just write that requirement in a
comment, but also add the necessary compiler and memory barriers.

Not sure how to handle these, maybe new CRs for both issues?

Thanks,
  Thomas