RFR (S): JDK-8046518: G1: Double calls to register_concurrent_cycle_end()

[Bengt Rutisson]

Hi Stefan,

Thanks for looking at this!

On 6/11/14 1:10 PM, Stefan Karlsson wrote:
>
> On 2014-06-11 12:33, Bengt Rutisson wrote:
>>
>> Hi all,
>>
>> Can I have a review for this change?
>>
>> http://cr.openjdk.java.net/~brutisso/8046518/webrev.00/
>>
>> https://bugs.openjdk.java.net/browse/JDK-8046518
>>
>> Background:
>> When we abort a concurrent cycle due to a Full GC in G1 we call 
>> ConcurrentMark::abort(). That will set _has_aborted flag and then 
>> call register_concurrent_cycle_end().
>>
>> The concurrent marking thread will see the _has_aborted flag in its 
>> ConcurrentMarkThread::run() method, abort the execution and then call 
>> register_concurrent_cycle_end().
>>
>> Currently this works since the code inside 
>> register_concurrent_cycle_end() is guarded by 
>> _concurrent_cycle_started which it then resets. So, the double calls 
>> will not necessarily result in too much extra work being done. But 
>> one of the things that register_concurrent_cycle_end() does is to 
>> call report_gc_end() on the concurrent GC tracer. That prevents 
>> further use of it for this GC. This means that inside the 
>> ConcurrentMarkThread::run() method we can not rely on the tracer.
>>
>> Removing the call to register_concurrent_cycle_end() in 
>> ConcurrentMark::abort() and relying on the call in 
>> ConcurrentMarkThread::run() seems to be a reasonable approach.
>
> The double call was deliberately put there to make sure that we end 
> the tracing of the concurrent GC before starting to trace teh Full GC. 

I figured there was a reason. I just couldn't remember. We would get 
overlapping GC events without this extra call. Thanks for pointing that out!

> Why do you need to change this? I guess it has to do with your other 
> GCId changes?

Right. It is for the GCId change. The problem is that calling 
register_concurrent_cycle_end() will reset the GCId to be -1. When we 
get to the logging, which is done in ConcurrentMarkThread::run(), I want 
to add the GCId to this log entry:

       if (cm()->has_aborted()) {
         if (G1Log::fine()) {
gclog_or_tty->gclog_stamp(g1h->gc_tracer_cm()->gc_id());
           gclog_or_tty->print_cr("[GC concurrent-mark-abort]");
         }
       }

But with the current code the GCId is always -1 here.

I guess one workaround I can do is to in abort() store the last aborted 
GC id and use that for logging. It just seems a bit fragile that we 
reset the concurrent gc tracer while we still have the concurrent mark 
running.

Bengt


>
> thanks,
> StefanK
>
>>
>> Thanks,
>> Bengt
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20140611/94325f68/attachment.htm>