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

[Coleen Phillimore]

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