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

[Coleen Phillimore]

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.
>
> 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
>   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 
>