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

[Bengt Rutisson]

Hi all,

I'm withdrawing this review request. I closed the bug as will not fix.

Bengt


On 2014-06-11 16:22, Bengt Rutisson wrote:
>
> 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/20140612/69d0d71c/attachment.htm>