Loading classes with many methods is very expensive

[Joel Borggrén-Franck]

Hi Peter,

I’m not entirely convinced this is a bug.

The lookup order for getMethod has for a long time been walk up superclasses and return what you find there first without even looking at interfaces. It might be desirable to change that but I’m not sure.

cheers
/Joel

On 29 okt 2014, at 12:26, Peter Levart <peter.levart at gmail.com> wrote:

> Hi Joel,
> 
> I found an inconsistency between getMethod() and getMethods() results that is present in current JDK8/9 code and in my latest webrev.02. The following program:
> 
> import java.util.stream.Collectors;
> import java.util.stream.Stream;
> 
> public class GetMethodTest {
> 
>    static void test(Class<?> clazz) throws Exception {
> 
>        System.out.println(clazz.getName() + ".class.getMethods():  " +
>                           Stream
>                               .of(clazz.getMethods())
>                               .filter(m -> m.getDeclaringClass() != Object.class)
>                               .collect(Collectors.toList()));
> 
>        System.out.println(clazz.getName() + ".class.getMethod(\"m\"): " +
>                           clazz.getMethod("m"));
> 
>        System.out.println();
>    }
> 
>    public static void main(String[] args) throws Exception {
>        test(I.class);
>        test(J.class);
>        test(A.class);
>        test(B.class);
>    }
> }
> 
> interface I {
>    void m();
> }
> 
> interface J extends I {
>    default void m() {}
> }
> 
> abstract class A implements I {}
> 
> abstract class B extends A implements J {}
> 
> 
> prints:
> 
> I.class.getMethods():  [public abstract void I.m()]
> I.class.getMethod("m"): public abstract void I.m()
> 
> J.class.getMethods():  [public default void J.m()]
> J.class.getMethod("m"): public default void J.m()
> 
> A.class.getMethods():  [public abstract void I.m()]
> A.class.getMethod("m"): public abstract void I.m()
> 
> B.class.getMethods():  [public default void J.m()]
> B.class.getMethod("m"): public abstract void I.m()
> 
> B.class.getMethods() reports default method J.m() (which I think is correct), but B.class.getMethod("m") reports the abstract I.m() inherited from A, because here the getMethod0() algorithm stops searching for and consolidating any methods in (super)interfaces. Do you agree that this is a bug?
> 
> 
> Regards, Peter
> 
> On 10/27/2014 02:45 PM, Joel Borggrén-Franck wrote:
>> Hi Peter,
>> 
>> As always, thanks for doing this! It has been on my todolist for a while but never quite bubbling up to the top.
>> 
>> I don’t have time to look att his right now, but I expect to have some free time next week, but i have two short comments
>> 
>> First, I have been thinking about moving MethodArray to its’s own top-level class, isn’t it about time?
>> 
>> Second I would expect testing for the missing cases you uncovered (good catch!).
>> 
>> I’ll try to get back to you asap.
>> 
>> cheers
>> /Joel
>> 
>> 
>> On 26 okt 2014, at 23:53, Peter Levart <peter.levart at gmail.com> wrote:
>> 
>>> On 10/26/2014 09:25 PM, Peter Levart wrote:
>>>> 19657 classes loaded in 1.987373401 seconds.
>>>> 494141 methods obtained in 1.02493941 seconds.
>>>> 
>>>> vs.
>>>> 
>>>> 19657 classes loaded in 2.084409717 seconds.
>>>> 494124 methods obtained in 0.915928578 seconds.
>>> Hi,
>>> 
>>> 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/
>>> 
>>> Comparing original vs. patched code still shows speed-up:
>>> 
>>> Original:
>>> 
>>> 19657 classes loaded in 1.980493029 seconds.
>>> 494141 methods obtained in 0.976318927 seconds.
>>> 494141 methods obtained in 0.886504437 seconds.
>>> 494141 methods obtained in 0.911153722 seconds.
>>> 494141 methods obtained in 0.880550509 seconds.
>>> 494141 methods obtained in 0.875526704 seconds.
>>> 494141 methods obtained in 0.877258894 seconds.
>>> 494141 methods obtained in 0.871794344 seconds.
>>> 494141 methods obtained in 0.884159644 seconds.
>>> 494141 methods obtained in 0.892648522 seconds.
>>> 494141 methods obtained in 0.884581841 seconds.
>>> 
>>> Patched:
>>> 
>>> 19657 classes loaded in 2.055697675 seconds.
>>> 494141 methods obtained in 0.853922188 seconds.
>>> 494141 methods obtained in 0.776203794 seconds.
>>> 494141 methods obtained in 0.858774803 seconds.
>>> 494141 methods obtained in 0.778178867 seconds.
>>> 494141 methods obtained in 0.760043997 seconds.
>>> 494141 methods obtained in 0.756352444 seconds.
>>> 494141 methods obtained in 0.740826372 seconds.
>>> 494141 methods obtained in 0.744264782 seconds.
>>> 494141 methods obtained in 0.73805894 seconds.
>>> 494141 methods obtained in 0.746852752 seconds.
>>> 
>>> 
>>> 55 java/lang/reflect jtreg tests still pass. As they did before, which means that we don't have a coverage for such cases. I'll see where I can add such a case (EnumSet for example, which inherits from Set interface and AbstractColection class via two different paths, so Set.size()/iterator() and AbstractCollection.size()/iterator() are both returned from getMethods())...
>>> 
>>> 
>>> Regards, Peter
>>> 
>