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
