Loading classes with many methods is very expensive

[Stanimir Simeonoff]

On Mon, Oct 27, 2014 at 1:23 PM, Aleksey Shipilev <
aleksey.shipilev at oracle.com> wrote:

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

But N would be the amount of exactly the same overridden methods from
superclasses and declared in interfaces. Usually there would be zero or
1-2. So the in-place linked list conserves memory, the extra indirection of
ArrayList for n=1 would also be slower. IMO, it's a well designed approach.
The only case where it'd actually matter is hundreds of superclasses each
overriding all of the their methods.

Stanimir


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