RFR 8165246: [REDO] InstanceKlass::_previous_version_count goes negative

[Coleen Phillimore]

Thank you for the code review.
Coleen

On 9/7/16 3:41 PM, serguei.spitsyn at oracle.com wrote:
> On 9/7/16 11:57, Coleen Phillimore wrote:
>>
>> Hi Serguei,  Thank you for reviewing also.
>>
>> On 9/7/16 2:52 PM, serguei.spitsyn at oracle.com wrote:
>>> Hi Coleen,
>>>
>>> I saw the review comments from George and Dan and so, do not touch 
>>> this ground.
>>> The fix looks good in general but I have a couple of comments.
>>>
>>> The _has_previous_versions being a static field looks a bit 
>>> dangerous to me,
>>> but it is probably Ok as the function has_previous_versions() is 
>>> called only
>>> in one place in the do_unloading which then will recalculate the 
>>> filed value.
>>>
>>
>> The renaming makes me feel better about it.
>
> Right.
> In terms of safety it is better with this fix then it is used to be.
> Nice approach anyway. :)
>
>
>>>
>>> src/share/vm/oops/instanceKlass.cpp
>>> 3389 void InstanceKlass::purge_previous_version_list() {
>>> 3390 assert(SafepointSynchronize::is_at_safepoint(), "only called at 
>>> safepoint");
>>> 3391 assert (has_been_redefined(), "Should only be called for main 
>>> class");
>>> 3392
>>> 3393 if (previous_versions() != NULL) { . . . 3476   }
>>>   Nit: I'd suggest to replace the 3393+3476 with:
>>> if (previous_versions() == NULL) { return; }
>> Okay.  I can change it to have an early exit and re-indent.   It 
>> might be easier to understand that way. thanks, Coleen 
>
> Thanks!
> Serguei
>
>>>   It will simplify the code by reducing the nesting level and 
>>> indentation. Thanks, Serguei On 9/6/16 09:24, Coleen Phillimore wrote:
>>>> Summary: make _has_previous_version a boolean that is set to true 
>>>> when previous version of a class is added or during class unloading 
>>>> call to purge_previous_versions The failed fix was because 
>>>> InstanceKlass's previous_versions klasses were walked more than 
>>>> once during class unloading, because the scratch classes are also 
>>>> on the CLD::_klasses list.  This change does three things.  1. Only 
>>>> walk the class if has_been_redefined (which is not set for the 
>>>> scratch class), 2. set next link to NULL for the scratch class when 
>>>> removed from the previous version list, and 3. change the counter 
>>>> to a flag because the counter may never go to zero if a CLD has 
>>>> redefined classes with previous versions and is unloaded. For this 
>>>> latter case we don't need to walk previous_versions. I tested this 
>>>> with changes 1 and 2 in Kitchensink, to verify the diagnosis, and 
>>>> then added 3 and retested. Also tested with the fix for 
>>>> https://bugs.openjdk.java.net/browse/JDK-8156137 which was checked 
>>>> into hs_-omp repository. Also ran through RBT 4 tiers nightly 
>>>> tests. open webrev at 
>>>> http://cr.openjdk.java.net/~coleenp/8165246.01/webrev bug link 
>>>> https://bugs.openjdk.java.net/browse/JDK-8165246 Thanks, Coleen 
>>>
>