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.lev...@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
>>
>
