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