6983726: Reimplement MethodHandleProxies.asInterfaceInstance#13197
6983726: Reimplement MethodHandleProxies.asInterfaceInstance#13197liach wants to merge 54 commits into
Conversation
|
👋 Welcome back liach! A progress list of the required criteria for merging this PR into |
Webrevs
|
|
I believe you can have better performance if you pass the method handle as the class data of the hidden class and you load it with a constant dynamic EDIT: you may have to use invokedynamic instead of a ldc constant dynamic + invokeExact() because the interface can have several methods to implement due to bridge methods (inheritance + generics) |
With one class per method handle + classdata, you do have better performance for invocations, but the penalties on creation is insurmountable: JMH results comparison between OracleJDK 20 and my initial patch, implemented based on this idea
Since this API needs to call |
|
Let suppose you have an interface like: The implementation needs to override both accept(Object) and accept(String). |
But this doesn't implement the same ClassValue cache? And spinning byte code can indeed be slow (I've seen this in other profiles). I think it should be possible to spin the class bytes once, stick the result in a Also, it would be nice if you could include the benchmarks you used in the patch as well. |
|
Also, some history.
|
Great point, I was dumbfounded there.
It already does, and there are multiple test cases covering that: mine and another in MethodHandlesGeneralTest
They are from the JDK itself: https://github.com/openjdk/jdk/blob/master/test/micro/org/openjdk/bench/java/lang/invoke/MethodHandleProxiesSuppl.java https://github.com/openjdk/jdk/blob/master/test/micro/org/openjdk/bench/java/lang/invoke/MethodHandleProxiesAsIFInstance.java Due to make issues (it passed
About access:
Sounds good; it's not hard to make a benchmark that compares asInterfaceInstance and metafactory side-by-side either. The non-strong feature was intended indeed when one class is defined for each class + methodhandle combination. I agree |
I attempted to spin one class for each mh+interface combination again. The creation time is halved compared to the no-template version, but the creation time is still very long (factor of 1000) compared to the passing to instance approach. I think we need to determine our approach based on the call patterns of this API:
In addition, we might write a benchmark for fresh creation performance as well (as Proxy does class caching, and LambdaMetafactory also caches class in CallSite). I don't think that asInterfaceInstance class spinning is much slower than metafactory. |
|
On a side note, one implementation class for one interface and passing handles via final fields approach (currently in this PR) is the closest to the current Proxy approach, that it almost always have better performance than the existing Proxy implementations. But this approach needs to be subject to the fresh creation performance checks as well. |
|
I think the current MethodHandleProxies implementation is indeed on par with LambdaMetafactory: The creation performance is slightly slower than that of Lmf (maybe because of heads-up asType conversions, which is required by the API specification), but the execution performance is already on par with it. |
JornVernee
left a comment
JornVernee
left a comment
There was a problem hiding this comment.
I think the approach taken by this patch is good.
I agree with your analysis about access checking. The access checks are preformed when creating the target method handle, and if the target is a @CallerSensitive method, the caller will be the lookup class. So, it seems like it's not possible to e.g. turn a MethodHandle pointing at MethodHandles::lookup into an interface instance to get access to a Lookup created in the target interface's module.
I also think the approach of creating a new class per method handle is good. This is currently the only way to guarantee peak performance as far as I'm aware. If only a single class is desired, this can be achieved by using a lambda expression that captures the target method handle. Though, that also requires knowing the target type statically.
I think this change needs a CSR as well to document the change in behavior. If an application currently calls asInterfaceInstance many times for the same interface, the amount of classes generated will increase (potentially a lot).
| proxy = lookup.findConstructor(lookup.lookupClass(), methodType(void.class)) | ||
| .asType(methodType(Object.class)).invokeExact(); |
There was a problem hiding this comment.
This can use invoke instead of an explicit asType and invokeExact. It is more or less the same.
| proxy = lookup.findConstructor(lookup.lookupClass(), methodType(void.class)) | |
| .asType(methodType(Object.class)).invokeExact(); | |
| proxy = lookup.findConstructor(lookup.lookupClass(), methodType(void.class)).invoke(); |
There was a problem hiding this comment.
Fyi the original code was an emulation of how LambdaMetafactory initializes no-arg lambda instances.
There was a problem hiding this comment.
@JornVernee Is there any performance difference with invokeExact vs invoke? I have the impression there is.
There was a problem hiding this comment.
It only matters when we get to C2 compilation for a constant method handle instance being called, which is not the case here. invoke is just a nice shorthand for doing an inexact call.
I had another look at the implementation, and it seems like C2 would not be able to inline through invoke calls, unless the type of the call site matches the type of the MethodHandle exactly. Using invokeExact would make sure that is the case. But, again, since we're just doing a one-off invocation of a non-constant method handle here, it doesn't matter in this case, so let's just use invoke for its convenience.
| proxyLoader = cl != null ? cl : ClassLoader.getSystemClassLoader(); | ||
|
|
||
| // Interface-specific setup | ||
| var info = INTERFACE_INFOS.get(intfc); // throws IllegalArgumentException |
There was a problem hiding this comment.
I find the use of var confusing here, since the type doesn't appear locally and is not well known either.
| var info = INTERFACE_INFOS.get(intfc); // throws IllegalArgumentException | |
| InterfaceInfo info = INTERFACE_INFOS.get(intfc); // throws IllegalArgumentException |
|
|
||
| private record LocalMethodInfo(MethodTypeDesc desc, List<ClassDesc> thrown) {} | ||
|
|
||
| private record InterfaceInfo(@Stable MethodType[] types, Lookup lookup, @Stable byte[] template) {} |
There was a problem hiding this comment.
Use of @Stable here is not needed. We don't constant fold through InterfaceInfo instances.
| private record InterfaceInfo(@Stable MethodType[] types, Lookup lookup, @Stable byte[] template) {} | |
| private record InterfaceInfo(MethodType[] types, Lookup lookup, byte[] template) {} |
There was a problem hiding this comment.
Hmm, I thought records are constant-folded.
jdk/src/hotspot/share/ci/ciField.cpp
Line 240 in 44f33ad
I will try with and without these annotations and report the creation benchmark results to see if they are worth it.
There was a problem hiding this comment.
The InterfaceInfo instance would have to be constant as well in order for loads of the fields & array elements to be constant folded (the same applies to records in general). However, the instances come from a call from ClassValue::get so they are not constant. (See also: https://bugs.openjdk.org/browse/JDK-8238260)
| ih); | ||
| } | ||
|
|
||
| var template = createTemplate(desc(intfc), methods.get(0).getName(), infos); |
There was a problem hiding this comment.
I think using an explicit type would be preferable here as well
| var template = createTemplate(desc(intfc), methods.get(0).getName(), infos); | |
| byte[] template = createTemplate(desc(intfc), methods.get(0).getName(), infos); |
|
|
||
| // Spin an implementation class for an interface. A new class should be defined for each handle. | ||
| // constructor parameter: Array[target, mh1, mh2, ...] | ||
| private static byte[] createTemplate(ClassDesc ifaceDesc, String name, List<LocalMethodInfo> methods) { |
There was a problem hiding this comment.
'name' is quite ambiguous here, I suggest name -> methodName
| bcb.constantInstruction(condy); | ||
| int slot = 1; | ||
| for (var t : mi.desc.parameterList()) { | ||
| var kind = TypeKind.from(t); | ||
| bcb.loadInstruction(kind, slot); | ||
| slot += kind.slotSize(); | ||
| } |
There was a problem hiding this comment.
You could also use CodeBuilder::parameterSlot instead of computing the slot manually.
| .return_()); | ||
|
|
||
| // actual implementations | ||
| int i = 1; |
There was a problem hiding this comment.
Can you give i a more descriptive name here? Also, can you add a comment that says why it starts at 1 (e.g. // +1 skip original target mh).
| // We both check for correctness and that it doesn't throw | ||
| @Test | ||
| public void testObjectMethods() throws Throwable { | ||
| var mh = MethodHandles.publicLookup() | ||
| .findVirtual(Integer.class, "compareTo", methodType(int.class, Integer.class)); | ||
| @SuppressWarnings("unchecked") | ||
| Comparator<Integer> p1 = (Comparator<Integer>) asInterfaceInstance(Comparator.class, mh); | ||
| @SuppressWarnings("unchecked") | ||
| Comparator<Integer> p2 = (Comparator<Integer>) asInterfaceInstance(Comparator.class, mh); | ||
|
|
||
| assertEquals(System.identityHashCode(p1), p1.hashCode()); | ||
| assertEquals(System.identityHashCode(p2), p2.hashCode()); | ||
|
|
||
| assertEquals(p1, p1); | ||
| assertEquals(p1 == p2, p1.equals(p2)); | ||
| assertEquals(p2 == p1, p2.equals(p1)); | ||
|
|
||
| assertEquals(Objects.toIdentityString(p1), p1.toString()); | ||
| assertEquals(Objects.toIdentityString(p2), p2.toString()); | ||
| } |
There was a problem hiding this comment.
tbh I don't really see the point of this test since we don't override hashCode/equals/toString?
| } | ||
|
|
||
| @Benchmark | ||
| public Doable lambdaCreate() throws Throwable { |
There was a problem hiding this comment.
I know this is pre-existing, but I don't think having both call and create benchmarks in the same class is that useful. Since the time that is taken by each is very different.
I suggest splitting these benchmarks into 2 files instead: one for the call benchmarks, and the other for create, and create + call benchmarks (and then change the output time units for the latter).
I think the call benchmark could be fleshed out a bit more as well. It would be interesting to see these cases:
- direct call to
doWork(this would be the baseline) - call through non-constant method handle
- call through non-constant interface instance created with lambda (existing
lambdaCall) - call through non-constant interface instance created with MHP::asInterfaceInstance (existing
testCall) - call through constant (
static final) method handle - call through constant (
static final) interface instance created with lambda - call through constant (
static final) interface instance created with MHP::asInterfaceInstance
|
Thank you so much @JornVernee: The WeakReference should point to the impl class. The Lookup is a cheap wrapper, so I changed it to be created each time instead. The test comes back green on my device with your suggestions. I think this patch is stable now; feel free to review again, especially that I've rearranged the tests. |
|
I missed to comment on |
I won't object to keep the impl class. Just to be clear, the test should pass even if it keeps Lookup as the referent, right? |
| */ | ||
| @Test | ||
| public void testNoAccess() { | ||
| Client untrusted = asInterfaceInstance(Client.class, MethodHandles.zero(void.class)); |
There was a problem hiding this comment.
| Client untrusted = asInterfaceInstance(Client.class, MethodHandles.zero(void.class)); | |
| Client obj = asInterfaceInstance(Client.class, MethodHandles.zero(void.class)); |
| public void testNoAccess() { | ||
| Client untrusted = asInterfaceInstance(Client.class, MethodHandles.zero(void.class)); | ||
| var instanceClass = untrusted.getClass(); | ||
| var leakLookup = Client.leakLookup(); |
There was a problem hiding this comment.
This is not really malicious code. It's checking the interface has no access to the proxy class. Probably better to rename them to be less alarming.
| var leakLookup = Client.leakLookup(); | |
| var lookup = Client.lookup(); |
| var instanceClass = untrusted.getClass(); | ||
| var leakLookup = Client.leakLookup(); | ||
| assertEquals(MethodHandles.Lookup.ORIGINAL, leakLookup.lookupModes() & MethodHandles.Lookup.ORIGINAL, | ||
| "Leaked lookup original flag"); |
There was a problem hiding this comment.
| "Leaked lookup original flag"); | |
| "expect lookup has original flag"); |
| WeakReference<Class<?>> cl; | ||
|
|
||
| var c1 = asInterfaceInstance(ifaceClass, mh); | ||
| cl = new WeakReference<>(c1.getClass()); |
There was a problem hiding this comment.
Nit: define a new variable not to mix with c1.
| cl = new WeakReference<>(c1.getClass()); | |
| var wr = new WeakReference<>(c1.getClass()); |
| System.gc(); | ||
| var c2 = asInterfaceInstance(ifaceClass, mh); | ||
| assertTrue(cl.refersTo(c2.getClass()), "MHP should reuse implementation class when available"); |
There was a problem hiding this comment.
We can simplify the test a little bit. Just to check if the class of c1 and c2 is the same first.
| System.gc(); | |
| var c2 = asInterfaceInstance(ifaceClass, mh); | |
| assertTrue(cl.refersTo(c2.getClass()), "MHP should reuse implementation class when available"); | |
| var c2 = asInterfaceInstance(ifaceClass, mh); | |
| assertTrue(c1.getClass() == c2.getClass(), "MHP should reuse implementation class when available"); |
There was a problem hiding this comment.
I will keep the gc, which ensures a scenario like that incorrectly weakref'd Lookup doesn't happpen again.
| * @build ProxiesImplementationTest Client | ||
| * @run junit ProxiesImplementationTest | ||
| */ | ||
| public class ProxiesImplementationTest { |
There was a problem hiding this comment.
how about WrapperHiddenClassTest? It's explicit that this verifies the wrapper hidden class.
| assertTrue(cl.refersTo(c2.getClass()), "MHP should reuse implementation class when available"); | ||
| Reference.reachabilityFence(c1); | ||
|
|
||
| // allow GC in interpreter |
There was a problem hiding this comment.
This not only affects GC in interpreter. maybe something like this:
| // allow GC in interpreter | |
| // clear strong reference to the wrapper instances |
| import org.openjdk.jmh.annotations.State; | ||
| import org.openjdk.jmh.annotations.Warmup; | ||
|
|
||
| import java.lang.invoke.LambdaMetafactory; |
|
|
||
| /** | ||
| * Benchmark evaluates the call performance of MethodHandleProxies.asInterfaceInstance | ||
| * return value, compared to |
There was a problem hiding this comment.
On a side note, these benchmarks were added for the class-per-MH implementation; I probably need to reevaluate if these 2 benchmarks are needed, or if the original MethodHandleProxiesAsIFInstance.java suffices.
| if (cl == null) { | ||
| // If the referent is cleared, create a new value and update cached weak reference. | ||
| cl = newProxy(intfc); | ||
| r.set(cl); | ||
| } | ||
| return new Lookup(cl); |
There was a problem hiding this comment.
| if (cl == null) { | |
| // If the referent is cleared, create a new value and update cached weak reference. | |
| cl = newProxy(intfc); | |
| r.set(cl); | |
| } | |
| return new Lookup(cl); | |
| if (cl != null) | |
| return new Lookup(cl); | |
| synchronized (r) { | |
| cl = r.get(); | |
| if (cl == null) { | |
| // If the referent is cleared, create a new value and update cached weak reference. | |
| cl = newProxy(intfc); | |
| r.set(cl); | |
| } | |
| return new Lookup(cl); | |
| } |
No. The test will not because there's no reference to the The alternative to keeping Classes in weak reference is to keep a lookup in the hidden class and cache that particular lookup, which IMO only complicates both the support code (have to distinguish from |
|
I see now. Ok. Then |
mlchung
left a comment
mlchung
left a comment
There was a problem hiding this comment.
Looks good. As the hidden interface check is added, the spec needs update to say "The interface must be public and not sealed and not hidden." as the CSR says. (sorry I might have confused you).
The CSR will need update as well as it no longer uses class data.
| assertTrue(implModule.getName().startsWith("jdk.MHProxy"), | ||
| () -> "incorrect dynamic module name: " + implModule.getName()); | ||
|
|
||
| assertSame(implClass.getClassLoader(), implModule.getClassLoader(), |
There was a problem hiding this comment.
I think this should check against ifaceModule.getClassLoader() instead, right? Since the dynamic module is defined in the interface' class loader.
There was a problem hiding this comment.
Good catch! Yes, this should check against the interface's loader.
There was a problem hiding this comment.
I've added a new check for ifaceClass.getClassLoader(), should be functionally equivalent. This check is originally part of ProxyForMethodHandle test, so I decided to keep it as well as it tests impl module's behavior.
|
@liach This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 40 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@JornVernee, @mlchung) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
openjdk
Bot
added
ready
|
Joe Darcy asks us to consider whether a release note should be added. I think yes, since there's some behavioral differences like hidden vs non-hidden classes. However, I think I might be able to upgrade asInterfaceInstance to accept protected abstract classes with a single public/protected abstract method(s) and a no-arg public/protected constructor, like |
|
I think applications unlikely depend on the old implementation of dynamic proxies. A release note may serve as a good documentation in case any application observes the performance difference because the hidden classes are GC'ed.
This is a separate issue and should not combine with this. |
|
I have made a release note documenting the potential performance impact: https://bugs.openjdk.org/browse/JDK-8312331 |
|
Looks like Mandy has cleaned up the release note. Thank you, integrating. /integrate |
|
yes, I made some modification to the release note in particular the performance regression seems alarming. If this becomes a real issue, we could change it to soft reference. This is great work! /sponsor |
|
Going to push as commit 5d57b5c.
Your commit was automatically rebased without conflicts. |
openjdk
Bot
removed
ready
8 participants
As John Rose has pointed out in this issue, the current j.l.r.Proxy based implementation of MethodHandleProxies.asInterface has a few issues:
This patch addresses all 3 problems:
Emptyto avoid method clashesBenchmark for revision 17:
Additionally, an obsolete
ProxyForMethodHandletest was removed, for it's no longer applicable.State of this patch:
Progress
Issues
Reviewers
Contributors
<mchung@openjdk.org>Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/13197/head:pull/13197$ git checkout pull/13197Update a local copy of the PR:
$ git checkout pull/13197$ git pull https://git.openjdk.org/jdk.git pull/13197/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 13197View PR using the GUI difftool:
$ git pr show -t 13197Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/13197.diff
Webrev
Link to Webrev Comment
Footnotes
single abstract method ↩