RFR: JDK-8148992: VM can hang on exit if root region scanning is initiated but not executed

[Bengt Rutisson]

Hi Thomas,

On 2016-02-10 11:10, Thomas Schatzl wrote:
> Hi Bengt,
>
> On Wed, 2016-02-10 at 10:02 +0100, Bengt Rutisson wrote:
>> Hi Jesper (and everyone),
>>
>> On 2016-02-08 15:50, Jesper Wilhelmsson wrote:
>>> Looks good!
>> Thanks for looking at this!
>>
>> I found a bug in the last webrev. The
>> G1ConcurrentMark::scanRootRegions()  has the side effect that it
>> calls ClassLoaderDataGraph::clear_claimed_marks(). It is not ok to
>> skip this call just because there are no root regions to scan. So, I
>> can't guard the call to scanRootRegions() with "if
>> (_cm->root_regions()->scan_in_progress())" as I did in webrev.02.
>>
>> I find this side effect a bit odd. So, I moved
>> ClassLoaderDataGraph::clear_claimed_marks() out to its own phase.
>> This will also make it show up clearer in our logs if it starts
>> taking a long time.
> I think this is a good idea.
>
>> Instead of checking scan_in_progress() both before the call to
>> scanRootRegions() and inside of it I moved the assert(!has_aborted())
>> in to scanRootRegions().
>>
>> Here's an updated webrev:
>> http://cr.openjdk.java.net/~brutisso/8148992/webrev.03/
>>
>> And here's a diff compared to the last version:
>> http://cr.openjdk.java.net/~brutisso/8148992/webrev.02-03.diff/
> Looks good.

Thanks!

>
> One nit: the change in heapVerifier.cpp seems to be something
> completely unrelated. Do you think it would be a good idea to move it
> to some extra CR?
>
>   411     _g1h->print_extended_on(log.error_stream());
>
> Would not need a re-review for this.

Right! That was part of my debugging for the bug with 
clear_claimed_marks() issue. I have a separate patch with more such 
changes that I will send out for review soon. I will remove this change 
before I push. Thanks for catching it!

Bengt

>
> Thanks,
>    Thomas
>
>