Loading classes with many methods is very expensive

[Aleksey Shipilev]

On 10/27/2014 01:53 AM, Peter Levart wrote:
> As you might have noticed, the number of methods obtained from patched
> code differed from original code. I have investigated this and found
> that original code treats abstract class methods the same as abstract
> interface methods as far as multiple inheritance is concerned (it keeps
> them together in the returned array). So I fixed this and here's new
> webrev which behaves the same as original code:
> 
> http://cr.openjdk.java.net/~plevart/jdk9-dev/Class.getMethods/webrev.02/

This seems to be a candidate fix for
https://bugs.openjdk.java.net/browse/JDK-8061950, right? If so, please
assign yourself there. It might be advisable to start the separate RFR
thread for a fix?

My comments for the first reading of the patch:

 * Why MethodList maintains the linked list? Doesn't O(n)
MethodList.add() lead to quadratic complexity again? Should we instead
use the ArrayList<Method> for storing method synonyms?

 * Please convert some of the indexed loops into for-each loops for
better readability? E.g:

      for (int i = 0; i < intfcsMethods.length; i++) {
         for (Method m : intfcsMethods[i]) {

  ...to:

      for (Method[] ms : intfcsMethods) {
         for (Method m : ms) {

 * I would think the code would be more readable if MethodList.m was
having a more readable name, e.g. MethodList.base or MethodList.image or
MethodList.primary?

 * Formatting and logic in MethodList.consolidateWith gives me chills. I
think at very least introducing variables for m.getDeclaringClass() and
ml.m.getDeclaringClass() improve readability. Also, moving the comments
at the end of the line should improve readability and better highlight
the boolean expression you are spelling out. Below is the
straight-forward rewrite:

>     MethodList consolidateWith(MethodList ml) {
>         Method mineM = m;
>         Method otherM = ml.m;
>         Class<?> mine = mineM.getDeclaringClass();
>         Class<?> other = otherM.getDeclaringClass();
> 
>         if (other == mine                                             // other is same method as mine
>             || !Modifier.isAbstract(mineM.getModifiers())             // OR, mine is non-abstract method...
>                 && (other.isAssignableFrom(mine)                      // ...which overrides other
>                    || other.isInterface() && !mine.isInterface())) {  // ...class method which wins over interface
>             // just return
>             return this;
>         }
>         if (!Modifier.isAbstract(otherM.getModifiers())               // other is non-abstract method...
>              && (mine.isAssignableFrom(other)                         // ...which overrides mine
>                  || mine.isInterface() && !other.isInterface())) {    // ...class method which wins over interface
>             // knock m out and continue
>             return (next == null) ? ml : next.consolidateWith(ml);
>         }
> 
>         // else leave m in and continue
>         next = (next == null) ? ml : next.consolidateWith(ml);
>         return this;
>     }


  * Should we use just methods.put() here?
      2851             MethodList ml0 = methods.putIfAbsent(ml, ml);

  * I wonder if the code would be simpler if we push the Map<MethodList,
MethodList> maintenance to some internal utility class? That would
probably simplify the loops in privateGetPublicMethods?

Thanks,
-Aleksey.