RFR 9 and 8u: JDK-8029674: (reflect) getMethods returns default methods that are not members of the class

[Joel Borggrén-Franck]

Hi Peter,

Thanks for looking at this. I like the idea, but I think you need to use LangReflectAccess, which almost isn’t in use today. I would like to think some more about how to best do it so I think this should be a potential follow up.

cheers
/Joel

On 10 jun 2014, at 08:14, Peter Levart <peter.levart at gmail.com> wrote:

> Hi Joel,
> 
> I don't know if this is warranted, since this is on slow path (the resulting methods are cached) and the number of overloaded methods is typically not large, but the following MethodArray method:
> 
> 2803         private boolean matchesNameAndDescriptor(Method m1, Method m2) {
> 
> could use a JavaLangAccess bridge to access the Method's parameter types without cloning the array each time.  Especially since it's called many times inside nested loop (see removeLessSpecifics()) where the number of invocations grows quadratically with the MethodArray's length. Method.getParameterTypes() is only invoked for pairs of overloaded methods, so this might not be a real issue.
> 
> Number of invocations could be halved though with this optimization of removeLessSpecific():
> 
>         void removeLessSpecifics() {
>             if (!hasDefaults())
>                 return;
> 
>             for (int i = 1; i < length; i++) {
>                 Method m = get(i);
>                 if  (m == null)
>                     continue;
> 
>                 for (int j  = 0; j < i; j++) {
>                     Method n = get(j);
>                     if (n == null || !(m.isDefault() || n.isDefault()))
>                         continue;
> 
>                     if (!matchesNameAndDescriptor(m, n))
>                         continue;
> 
>                     if (hasMoreSpecificClass(m, n))
>                         remove(j);
>                     else if (hasMoreSpecificClass(n, m))
>                         remove(i);
>                 }
>             }
>         }
> 
> 
> Regards, Peter
> 
> 
> On 05/07/2014 07:43 PM, Joel Borggrén-Franck wrote:
>> Hi,
>> 
>> Since Java 8 the methods Class.getMethod() and Class.getMethods() can in some circumstances return methods that are not members of the class. This is due to a spec change in 8 not due to a code change. For this to happen you need to have an interface hierarchy that looks something like:
>> 
>> interface I { void m(); } 
>> interface J extends I { void m(); } 
>> 
>> and either one of:
>> 
>> interface K extends ***I,*** J {} 
>> class C implements ***I,*** J {}
>> 
>> In Java 7 K.class.getMethods() would return [ I.m, J.m ]. The 7 spec is a bit vague but I have been convinced by experts that this is actually correct in 7. However this was changed in the spec for 8, now only J.m() is a member of K or C so the result should only contain [ J.m() ].
>> 
>> See the bug: 
>> https://bugs.openjdk.java.net/browse/JDK-8029674
>>  for a more thorough discussion of this.
>> 
>> The fix proposed for 8u (and 9 at first) is a partial fix that ensures that:
>> 
>> - a concrete class does not have any abstract methods
>> - nothing is changed unless someone introduces default methods into the interface hierarchy
>> 
>> This is to minimise the behavioural compatibility concerns in a minor release. For 9 we might implement the full solution.  (See the 9 bug: 
>> https://bugs.openjdk.java.net/browse/JDK-8029459
>> ).
>> 
>> Basically in the fix proposed for 8u, default methods prune less specific methods from the result if they have the same discriptor. There are some complications due to preserving the old lookup order and the fact that we don’t want different results depending on the order of pairwise comparisons. Since this work and MethodArray in general is comparing descriptors bridges should not interact with this patch.
>> 
>> Bug: 
>> https://bugs.openjdk.java.net/browse/JDK-8029674
>> 
>> Patch: 
>> http://cr.openjdk.java.net/~jfranck/8029674/webrev.02/
>> 
>> 
>> *** Testing ***
>> 
>> I made a test with a bit over 100 cases where 40 of the tests test behaviour that shouldn’t change (IE they pass both with and without the patch). The test is a bit vast, but I have commented some of the more interesting results.
>> 
>> *** Performance ***
>> 
>> The time complexity of this is O(defaultMethods * methods). getMethods() is already somewhat quadratic in complexity due to removing interface methods that have a concrete implementation. Also the result is cached so this is a one time increase per Class.
>> 
>> I have done some experiments on a patched jdk without the cache and the performance looks decent:
>> 
>> - Loading every class in rt.jar and calling getMethods() on every Class takes 8s on my laptop both before and after the patch. Perhaps not that surprising since there aren’t that many default methods in the jdk and this is probably dominated by IO.
>> 
>> - Calling getMethods on a Class with 16K methods, 10% which are default methods, seem to take roughly 50% longer on my laptop (2,4 instead of 1,7 seconds). I’m working on converting this benchmark to JMH, but at least I don’t think I have made the most basic benchmark errors.
>> 
>> If this turns out to be a performance problem I have an idea for an index that will make this O(methods sharing name * methods) for an addition of a few allocations, but I don’t expect this to be necessary.
>> 
>> Thanks in advance
>> 
>> cheers
>> /Joel
>> 
>