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

Wed Sep 7 18:16:50 UTC 2016

On 9/7/16 12:46 PM, Daniel D. Daugherty wrote:
> I'm good with all of your proposed changes.

Thank you Dan for the detailed review.  I'll retest the change with some 
redefine tests, just to make sure I didn't break anything.
Coleen

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