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

[serguei.spitsyn at oracle.com]

Dan,

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

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20150224/4daa1806/attachment-0001.html>