On Tue, 28 Mar 2023 14:00:41 GMT, Jorn Vernee <jver...@openjdk.org> wrote:
> I think it should be possible to spin the class bytes once, stick the result
> in a ClassValue cache, but then use the bytes to define multiple classes with
> different class data (MethodHandles).
Great point, I was dumbfounded there.
> Let suppose you have an interface like:
>
> ```
> interface StringConsumer extends Consumer<String> {}
> ```
>
> The implementation needs to override both accept(Object) and accept(String).
It already does, and there are multiple test cases covering that:
[mine](https://github.com/openjdk/jdk/pull/13197/files#diff-9753e5dae7b27070a1fe0d6c12eb65db605ee479d0e93d214ce710a8bbfc4922R176)
and another in
[MethodHandlesGeneralTest](https://github.com/openjdk/jdk/blob/927e674c12aa7965c63059b8f650d8f60156cefc/test/jdk/java/lang/invoke/MethodHandlesGeneralTest.java#L1785)
> Also, it would be nice if you could include the benchmarks you used in the
> patch as well.
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 `/` for module exports as `` on my windows
somehow), I ran the test by copying them to a separate Gradle project with
Gradle JMH plugin.
> I've worked on something similar before:
> https://bugs.openjdk.org/browse/JDK-8257605 The idea then was to add a new
> API in MethodHandles but since then I've been thinking that enhancing the
> performance of MethodHandleProxies might be better. The only issue I saw with
> that is that the current API doesn't accept a MethodHandles.Lookup object
> which would be needed to define an interface instance if the interface is in
> a non-exported package. Your solution just creates the lookup directly.
> Typically we have to be careful with injecting classes into other
> packages/modules since it can lead to access escalation, but maybe in this
> case it is fine. I don't think there's a way that this could be used to
> escalate access, though it seems strange that anyone could now define a class
> in a non-exported package/some other module.
About access:
1. The method handle itself is already access-checked upon creation.
2. The interface access may be problematic: A non-exported interface Class
object can be obtained via Reflection inspection on exported types, such as
java packages and jdk.internal packages.
- In that case, it might not be of best interest to create an interface, but
I don't think the current asInterfaceInstance API rejects such creations either.
3. The class definition under the interface and its classloader IMO is safe, as
the class will not refer to any type the interface itself does not refer to;
the annotation is not involved in class loading either.
> We've also recently discussed LambdaMetafactory
> https://github.com/openjdk/jdk/pull/12493 which will produce classes strongly
> tied to the class loader. If we can bring MethodHandleProxies up to the same
> performance level as the classes produced by LambdaMetaFactory, it could
> serve as an alternative and people could move away from using the
> LambdaMetafactory runtime API (which is not really meant to be used
> directly). WRT that, I think the fact that the implementation proposed by
> this patch defines classes that are not strongly tied to the defining loader
> is a good thing.
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 `asInterfaceInstance` would be extremely useful to users,
like a plugin loader converting json-based static method references into SAM
implementations, which definitely should not become defined as a part of the
plugin loader; or an event bus firing events.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/13197#issuecomment-1487012367
