2-nd round RFR (M) 8046246: the constantPoolCacheOopDesc::adjust_method_entries() used in RedefineClasses does not scale

[Daniel D. Daugherty]

On 2/20/15 2:32 PM, serguei.spitsyn at oracle.com wrote:
> The hotspot webrev below addresses the Coleen's comments from the 1-st 
> review round.
>
> Open hotspot webrev:
> http://cr.openjdk.java.net/~sspitsyn/webrevs/2015/hotspot/8046246-JVMTI-redefscale.2/

Thumbs up!

src/share/vm/oops/instanceKlass.hpp
   No comments.

src/share/vm/oops/instanceKlass.cpp
   InstanceKlass::adjust_default_methods() - so you drop the outer level
   of for-loop here by switching from parallel old_methods/new_methods
   arrays to looping on the target array (default methods) and only
   fetching the old_method candidate that's in parallel with the
   current default method _and_ only fetching the new method when you
   need it.

   So you've squashed a nested for-loop and you're only fetching the
   new method when you know you need it. Nicely done.

src/share/vm/oops/cpCache.hpp
   line 482: void adjust_method_entries(InstanceKlass* holder, bool * 
trace_name_printed);
     Nit - this line (and the previous) one have a space between
     'bool' and '*'. The other pointer params do not. Seems to be
     a common format difference in 'bool *' params. :-)

src/share/vm/oops/cpCache.cpp
   No comments.

src/share/vm/oops/klassVtable.hpp
   No comments.

src/share/vm/oops/klassVtable.cpp
   klassVtable::adjust_method_entries() and
   klassItable::adjust_method_entries() have similar
   loop squashing. Again, nicely done.

src/share/vm/prims/jvmtiRedefineClasses.cpp
   No comments.

src/share/vm/classfile/defaultMethods.cpp
   No comments.

src/share/vm/oops/constMethod.hpp
   Cool way of using a little bit of space to squash
   some loops. Surprised Coleen let you have a u2 though :-)

src/share/vm/oops/method.hpp
   No comments.

src/share/vm/oops/method.cpp
   No comments.

Nit - double check copyright updates before you commit.

Dan




> Open jdk (new unit test) webrev:
> http://cr.openjdk.java.net/~sspitsyn/webrevs/2015/jdk/8046246-JVMTI-manymethods.1/ 
>
>
> Thanks,
> Serguei
>
>
> On 2/18/15 9:45 PM, serguei.spitsyn at oracle.com wrote:
>> Please, review the fix for:
>>   https://bugs.openjdk.java.net/browse/JDK-8046246
>>
>>
>> Open hotspot webrevs:
>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2015/hotspot/8046246-JVMTI-redefscale.1/ 
>>
>>
>> Open jdk (new unit test) webrev:
>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2015/jdk/8046246-JVMTI-manymethods.1/ 
>>
>>
>>
>> Summary:
>>
>>    This performance/scalability issue in class redefinition was 
>> reported by HP and the Enterprise Manager team.
>>    The following variants of the adjust_method_entries() functions do 
>> not scale:
>>      ConstantPoolCache::adjust_method_entries()
>>      klassVtable::adjust_method_entries()
>>      klassItable::adjust_method_entries()
>>      InstanceKlass::adjust_default_methods()
>>
>>    The ConstantPoolCache::adjust_method_entries() is the most important.
>>
>>    The approach is to use the holder->method_with_idnum() like this:
>>      Method* new_method = 
>> holder->method_with_idnum(old_method->orig_method_idnum());
>>      if (old_method != new_method) {
>>          <replace old_method with new_method>
>>      }
>>
>>      New algorithm has effectiveness O(M) instead of original O(M^2),
>>      where M is count of methods in the class.
>>      The new test (see webrev above) was used to mesure CPU time 
>> consumed by the
>>      ConstantPoolCache::adjust_method_entries() in both original and 
>> new approach.
>>
>>      The performance numbers are:
>>
>> --------------------------------------------------------------------------------------- 
>>
>>      Methods: ------ 1,000 --------------- 10,000 ----------------- 
>> 20,000
>> --------------------------------------------------------------------------------------- 
>>
>>      Orig:         600,000 nsec (1x)   60,500,000 nsec (~100x) 
>> 243,000,000 nsec (~400x)
>>      New:           16,000 nsec (1x)      178,000 nsec (~10x) 355,000 
>> nsec  (~20x)
>> --------------------------------------------------------------------------------------- 
>>
>>
>>
>>
>> Testing:
>>   In progress: VM SQE RedefineClasses tests, JTREG 
>> java/lang/instrument, com/sun/jdi tests
>>
>>
>> Thanks,
>> Serguei
>>
>