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

serguei.spitsyn at oracle.com serguei.spitsyn at oracle.com
Wed Sep 7 19:41:33 UTC 2016


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



More information about the hotspot-runtime-dev mailing list