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

[David Lindholm]

Thomas,

Thanks for the review! Yes, I believe the best thing to do is to write 
new CRs for the two other things you found.


Thanks,
David

On 2016-01-07 12:11, Thomas Schatzl wrote:
> 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
>