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

[Daniel D. Daugherty]

I'm good with all of your proposed changes.

Dan


On 9/7/16 10:42 AM, Coleen Phillimore wrote:
>
>
> On 9/7/16 11:36 AM, Daniel D. Daugherty wrote:
>> On 9/6/16 10:24 AM, 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
>>
>> src/share/vm/oops/instanceKlass.hpp
>>     No comments.
>>
>> src/share/vm/oops/instanceKlass.cpp
>>     L3370: // Globally, there is at least one previous version of a 
>> class to walk
>>     L3371: // during class unloading, which is saved because there 
>> are old methods in the class
>>     L3372: // are still running.   Otherwise the previous version 
>> list is cleaned up.
>>         Perhaps a little tweaking of the comment:
>>
>>            // Globally, there is at least one previous version of the 
>> class to walk
>>            // during class unloading, which was saved because there 
>> were old methods
>>            // in the class that are still running. Otherwise the 
>> previous version
>>            // list is cleaned up.
>
> George had the same comment, so I fixed the grammar there.
>>
>>     L3375: // Returns whether there are previous versions of a class 
>> for class
>>     L3376: // unloading only.  This resets the value to false. 
>> purge_previous_versions
>>     L3377: // will set it to true if there are any left, i.e. if 
>> there's any work to do for
>>     L3378: // next time.  This is to avoid the expensive code cache 
>> walk in CLDG::do_unloading.
>>         Perhaps a little tweaking of the comment:
>>
>>            // Returns true if there are previous versions of a class 
>> for class
>>            // unloading only. Also resets the flag to false. 
>> purge_previous_versions
>>            // will set the flag to true if there are any left, i.e., 
>> if there's any
>>            // work to do for next time. This is to avoid the 
>> expensive code cache
>>            // walk in CLDG::do_unloading().
>>
> Duly tweaked.  I also renamed the function 
> has_previous_versions_and_reset().
>
>>     L3379: bool InstanceKlass::has_previous_versions() {
>>     L3383:   _has_previous_versions = false;
>>         Named like a query function, but it has a side effect.
>>         Perhaps: has_previous_versions_and_reset()?
>>
>>     L3391:   assert (has_been_redefined(), "Should only be called for 
>> main class");
>>         Nit: please delete the space before '('.
>>
>
> Fixed.
>
>>     L3427: loader_data->add_to_deallocate_list(pv_node);
>>     L3428:         InstanceKlass* next = pv_node->previous_versions();
>>     L3429:         pv_node->link_previous_versions(NULL); // point 
>> next to NULL
>>     L3430:         last->link_previous_versions(next);
>>         The pv_node to be deleted is added to the deallocate list
>>         before it is decoupled from chain. Perhaps it should be
>>         added after it is decoupled to permit parallel processing
>>         of the deallocate list, i.e., move L3427 after L3430. I
>>         don't think it is processed in parallel now, but down the
>>         road...
>
> Okay, this seems like a good suggestion.  It's not processed in 
> parallel now but yeah, you never know.
>
> I did this carefully and added a comment.
>
>         log_trace(redefine, class, iklass, purge)
>           ("previous version " INTPTR_FORMAT " is dead.", p2i(pv_node));
>         // For debugging purposes.
>         pv_node->set_is_scratch_class();
>         // Unlink from previous version list.
>         assert(pv_node->class_loader_data() == loader_data, "wrong 
> loader_data");
>         InstanceKlass* next = pv_node->previous_versions();
>         pv_node->link_previous_versions(NULL);   // point next to NULL
>         last->link_previous_versions(next);
>         // Add to the deallocate list after unlinking.
>         loader_data->add_to_deallocate_list(pv_node);
>         pv_node = next;
>         deleted_count++;
>         version++;
>
>>
>>     L3592:   // Add previous version if any methods are still running.
>>     L3593:   _has_previous_versions = true;
>>         In the other place you set the flag to true, you mention
>>         that this is for class unloading. Do you want to say that
>>         here also?
>
> ok, added:
>
>   // Set has_previous_version flag for processing during class unloading.
>
>>
>>
>> test/runtime/RedefineTests/RedefinePreviousVersions.java
>>     L26:  * @bug 8164692
>>         Please change the bug ID to 8165246.
>
> thanks, missed that.
>>
>>     L27:  * @summary Redefine previous_versions count goes negative
>>         Need to update the test summary.
>
>  * @summary Test has_previous_versions flag and processing during 
> class unloading.
>
>>
>>     L88:         // previous_version_list and the count should stay 
>> zero and not go negative
>>         Need to update the comment since there is no longer
>>         a counter.
>
>         // previous_version_list and the flag _has_previous_versions 
> should stay false
>
> I reused the test so missed the old comments.  Thanks for pointing 
> them out.
>>
>> OK so the whole reason for InstanceKlass::has_previous_versions() is
>> an optimization so that we don't have to do an expensive code cache
>> walk for previous versions during class unloading. The old code
>> implemented the "state of having a previous version" as a counter.
>> The new code implements the "state of having a previous version"
>> as a flag that is only set when we add a previous version (yea!)
>> and cleared when it is queried. Since there is no more counter,
>> we won't run into the bug we're trying to fix.
>>
>> The only thing that bugs me is this:
>>
>>   bool v1 = has_previous_versions();
>>   bool v2 = has_previous_versions();
>>
>> I would expect (v1 == v2) which isn't what you get.
>> Perhaps renaming from has_previous_versions() to
>> has_previous_versions_and_reset() will make it clear
>> that there is a side effect...
>>
>
> Yes, this is a bit dangerous to assume that two calls to 
> has_previuos_verisons() would have the same result.  I renamed the 
> function as suggested.
>
> Thanks for the thorough review!
>
> Coleen
>
>> Dan
>>
>>
>>> bug link https://bugs.openjdk.java.net/browse/JDK-8165246
>>>
>>> Thanks,
>>> Coleen
>>>
>>>
>>
>