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

[serguei.spit...@oracle.com]

Dan,

On 2/24/15 6:59 PM, Daniel D. Daugherty wrote:
On 2/20/15 2:32 PM, serguei.spit...@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!

Thanks a lot!


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.

Thanks


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

I agree, it is better to fix it as the line is touched anyway.


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.


I'm also thinking how to optimize this call:

3438     for (InstanceKlass* pv_node = ik->previous_versions();
3439          pv_node != NULL;
3440          pv_node = pv_node->previous_versions()) {
3441       cp_cache = pv_node->constants()->cache();
3442       if (cp_cache != NULL) {
3443         cp_cache->adjust_method_entries(_matching_old_methods,
3444                                         _matching_new_methods,
3445                                         _matching_methods_length,
3446                                         &trace_name_printed);
3447       }
3448     }


But I left it for possible future improvement that is covered by new bug:
  https://bugs.openjdk.java.net/browse/JDK-8073705
 



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

Yes, I'm grateful to Coleen that she suggested it! :)


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

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

Nit - double check copyright updates before you commit.

Sure, that was my plan.


Thanks!
Serguei


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