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

[Daniel D. Daugherty]

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.

     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().

     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 '('.

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

     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?


test/runtime/RedefineTests/RedefinePreviousVersions.java
     L26:  * @bug 8164692
         Please change the bug ID to 8165246.

     L27:  * @summary Redefine previous_versions count goes negative
         Need to update the test summary.

     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.

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

Dan


> bug link https://bugs.openjdk.java.net/browse/JDK-8165246
>
> Thanks,
> Coleen
>
>