Hi Peter,
The removal of value wrapper is a clever approach to reduce the new
instances created although it feels very unnatural (at least to me). Small
optimization; eagerly calculate the hash in the c'tor,
hash =
149 method.getReturnType().hashCode() ^
150 System.identityHashCode(method.getName()) ^
151 Arrays.hashCode(method.getParameterTypes()
and so the real int hashCode(){return seq+hash;}
Also I am not sure if method.getName().hashCode() would be better or not,
if previously identityHashCode is not invoked on that string and depending
on the configured hashCode (-XX:hashCode) generator System.identityHashCode
might be slow or worse call C random() which doesn't scale. Setting the
hashCode requires a CAS too. CAS is of course one-time off but still
About the order of the returned methods: if you remove and then put from/to
the LHM the order of iteration is going to be greatly altered. Is that ok?
Stanimir
On Wed, Oct 29, 2014 at 7:16 PM, Peter Levart <peter.lev...@gmail.com>
wrote:
> On 10/29/2014 02:08 PM, Joel Borggrén-Franck wrote:
>
>> 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.
>>
>
> Hi Joel,
>
> It has been for a long time like that as you say, but for a long time Java
> did not have default methods. It's unexpected for getMethod() to return a
> method that is not contained in getMethods() result.
>
> Anyway, I have created a bug to track this:
>
> https://bugs.openjdk.java.net/browse/JDK-8062389
>
> For next iteration of the getMethods() O(n^2) fix, I used a slightly
> different approach, which you might like more or not. Instead of using
> linked-lists of Method objects as values of a LinkedHashMap I created a
> special Key object, holding a reference to Method object and an additional
> 'seq' int field, which discriminates among methods with same signature.
> Values of LinkedHashMap are Method objects themselves:
>
> http://cr.openjdk.java.net/~plevart/jdk9-dev/Class.getMethods/webrev.03/
>
> I have encapsulated this functionality into a package-private
> java.lang.MethodTable. The implementation of this API can be easily changed
> to using linked-lists as values of LinkedHashMap if desired. The
> performance characteristics are similar, with hardly measurable advantage
> of this latest approach as can be seen from http://cr.openjdk.java.net/~
> plevart/jdk9-dev/Class.getMethods/GetAllRtMethods.java benchmark:
>
> Original code:
>
> 19658 classes loaded in 2.071013902 seconds.
> 494392 methods obtained in 1.089983418 seconds.
> 494392 methods obtained in 0.952488497 seconds.
> 494392 methods obtained in 0.912878317 seconds.
> 494392 methods obtained in 0.940293784 seconds.
> 494392 methods obtained in 0.987640733 seconds.
> 494392 methods obtained in 0.925393355 seconds.
> 494392 methods obtained in 0.89397002 seconds.
> 494392 methods obtained in 0.915042463 seconds.
> 494392 methods obtained in 0.897669082 seconds.
> 494392 methods obtained in 0.878140502 seconds.
>
> Patched code:
>
> 19658 classes loaded in 2.153024197 seconds.
> 494392 methods obtained in 0.875651469 seconds.
> 494392 methods obtained in 0.791937742 seconds.
> 494392 methods obtained in 0.780995693 seconds.
> 494392 methods obtained in 0.759593461 seconds.
> 494392 methods obtained in 0.766528355 seconds.
> 494392 methods obtained in 0.756567663 seconds.
> 494392 methods obtained in 0.739177848 seconds.
> 494392 methods obtained in 0.729245613 seconds.
> 494392 methods obtained in 0.74081083 seconds.
> 494392 methods obtained in 0.731749505 seconds.
>
>
> Martin's ManyMethodsBenchmark shows this algorithm has O(n) time
> complexity too:
>
> Original:
>
> Base class load time: 131.95 ms
> getDeclaredMethods: 65521 methods, 32.00 ms total time, 0.0005 ms per
> method
> getMethods : 65530 methods, 44.24 ms total time, 0.0007 ms per
> method
> Derived class load time: 32525.23 ms
> getDeclaredMethods: 65521 methods, 30.37 ms total time, 0.0005 ms per
> method
> getMethods : 65530 methods, 7897.03 ms total time, 0.1205 ms per
> method
>
> Patched:
>
> Base class load time: 129.72 ms
> getDeclaredMethods: 65521 methods, 32.76 ms total time, 0.0005 ms per
> method
> getMethods : 65530 methods, 42.68 ms total time, 0.0007 ms per
> method
> Derived class load time: 31620.47 ms
> getDeclaredMethods: 65521 methods, 30.49 ms total time, 0.0005 ms per
> method
> getMethods : 65530 methods, 88.23 ms total time, 0.0013 ms per
> method
>
>
> I have also run Martin's LoadAllClassesAndMethods test (Thanks Martin, I
> changed it slightly so that exceptions are collected and reported at the
> end instead of bailing-out on first exception).
>
> Original LoadAllClassesAndMethods write classlist.txt:
>
> class load: 23052 classes, 1563.75 ms total time, 0.0678 ms per class
> 4 exceptions encountered:
> java.lang.IncompatibleClassChangeError:
> jdk.nashorn.internal.codegen.CompilationPhase$10
> and jdk.nashorn.internal.codegen.CompilationPhase$10$1 disagree on
> InnerClasses attribute
> java.lang.IncompatibleClassChangeError:
> jdk.nashorn.internal.codegen.CompilationPhase$5
> and jdk.nashorn.internal.codegen.CompilationPhase$5$1 disagree on
> InnerClasses attribute
> java.lang.IncompatibleClassChangeError: java.security.ProtectionDomain$3
> and java.security.ProtectionDomain$3$1 disagree on InnerClasses attribute
> java.lang.IncompatibleClassChangeError: java.lang.Compiler and
> java.lang.Compiler$1 disagree on InnerClasses attribute
> getMethods: 23052 classes, 831.87 ms total time, 0.0361 ms per class
>
> Patched LoadAllClassesAndMethods diff classlist.txt:
>
> class load: 23051 classes, 1596.58 ms total time, 0.0693 ms per class
> 5 exceptions encountered:
> java.lang.IncompatibleClassChangeError:
> jdk.nashorn.internal.codegen.CompilationPhase$10
> and jdk.nashorn.internal.codegen.CompilationPhase$10$1 disagree on
> InnerClasses attribute
> java.lang.IncompatibleClassChangeError:
> jdk.nashorn.internal.codegen.CompilationPhase$5
> and jdk.nashorn.internal.codegen.CompilationPhase$5$1 disagree on
> InnerClasses attribute
> java.lang.IncompatibleClassChangeError: java.lang.Class and
> java.lang.Class$MethodArray disagree on InnerClasses attribute
> java.lang.IncompatibleClassChangeError: java.security.ProtectionDomain$3
> and java.security.ProtectionDomain$3$1 disagree on InnerClasses attribute
> java.lang.IncompatibleClassChangeError: java.lang.Compiler and
> java.lang.Compiler$1 disagree on InnerClasses attribute
> getMethods: 23051 classes, 799.99 ms total time, 0.0347 ms per class
> The following classes were expected, but not found:
> [java.lang.Class$MethodArray]
>
>
> The reason for Class$MethodArray to not be loaded by patched code is the
> way I tested patched j.l.Class. I prepended the boot classpath with a
> directory holding patched Class/MethodTable. Martin's
> LoadAllClassesAndMethods tries to load Class$MethodArray anyway, since it's
> in rt.jar, but this class is gone in patched j.l.Class, so
> "IncompatibleClassChangeError: java.lang.Class and
> java.lang.Class$MethodArray disagree on InnerClasses attribute" is expected
> in this case. Otherwise this test shows that original and patched code
> agree on results returned from getMethods() for all system and extension
> JDK classes.
>
> All 86 jtreg tests in java/lang/Class/ and java/lang/reflect/ pass.
>
> I still have to create a test case for inconsistency I discovered in
> previous iteration.
>
> Regards, Peter
>
>
>
>
>> cheers
>> /Joel
>>
>> On 29 okt 2014, at 12:26, Peter Levart <peter.lev...@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.lev...@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
>>>>>
>>>>>
>
