On Fri, 27 Aug 2021 13:28:08 GMT, Maurizio Cimadamore <mcimadam...@openjdk.org>
wrote:
> Overall, seems like a solid piece of work. I did not review in full the
> intricacies with caller sensitive (as I don't know that area too much), but
> the general rewrite seems solid.
Thanks for the review.
> One thing I had trouble with was the naming of the various method accessors -
> for instance, DirectMethodAccessorImpl vs. NativeMethodAccessorImpl vs.
> DelegatingMethodAccessorImpl vs. AdaptiveMethodAccessor (and there's more).
> It's not always easy to grasp from the method name which one is new and which
> one is old. Maybe sprinkling the `Handle` word on any accessor impl would
> help a bit with that.
I can see how this could be confusing. Sprinkling the `Handle` word may help.
I will try that.
> Similarly, I found the `ClassByteBuilder` name a bit weak, since that is
> meant to only create instance of custom MHInvokers. A more apt name will help
> there too.
What about `InvokerBuilder` (it creates either MHInvoker or VHInvoker)?
> I also had some issues parsing the flow in
> `ReflectionFactory::newMethodAccessor` (and constructor, has similar issue):
>
> ```
> if (useNativeAccessor(method)) {
> return DirectMethodAccessorImpl.nativeAccessor(method,
> callerSensitive);
> }
> return MethodHandleAccessorFactory.newMethodAccessor(method,
> callerSensitive);
> ```
>
> Why can't logic like this be encapsulated in a single factory call? E.g. it
> seems like the code is would like to abstract the differences between the
> various accessor implementations, but it doesn't seem to get all the way
> there (it's also possible I'm missing some constraint here).
We have to avoid to initialize the `MethodHandleAccessorFactory` class until
`java.lang.invoke` is initialized. because
`MethodHandleAccessorFactory::<clinit>` creates lookup object and method
handles. I agree it's cleaner to encapsulate these in one single factory
method. I think moving the the static fields and the initialization to a
holder class may be a way to do it. I will give it a try.
> src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line
> 200:
>
>> 198: return
>> MethodHandleAccessorFactory.newMethodAccessor(method, callerSensitive);
>> 199: } else {
>> 200: if (!useDirectMethodHandle && noInflation
>
> seems to me that the `!useDirectMethodHandle` is useless here (always false) ?
Good catch! Will fix.
-------------
PR: https://git.openjdk.java.net/jdk/pull/5027
