On Sat, 7 Aug 2021 02:07:24 GMT, Mandy Chung <mch...@openjdk.org> wrote:
>> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`. For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > minor test cleanup Hi Mandy, Quite a big change this has become. Good work! These are just preliminary observations. I'll try to do a more in-depth check in a week since I'm off on vacation in the coming week. I noticed there's a lot of complication due to logic that specializes the cases for small number of parameters via indirection through MHMethodAccessorDelegate just to avoid spreading the arguments of an Object[] array via the MH combinator asSpreader. Does this really bring noticeable performance improvement for the small number of arguments? I'm asking because the arguments in reflection API are already boxed and packed into an Object[] array and my feeling tells me that spreading them via MH combinator should have similar performance as doing that manually in the switch of DirectMethodAccessorImpl.XXX#invokeImpl methods. I may be wrong. If MH combinator is noticeably slower, I think it should be fixed. Another brittle part in my opinion is the AccessorUtils.isIllegalArgument method and the use of it in logic to determine where the caught exception comes from to wrap it in the correct type of exception for the reflection API. The logic of that method is based on assumptions of how a lot of other code is structured. Any change to the structure of that code can brake that logic. I liked the solution used in previous iterations where an exception handler was inserted in the MH chain immediately after the DMH which wrapped any Throwable into InvocationTargetException. So any naked exceptions thrown from MH chain could be contributed to argument processing. Why did you choose another route? Regards, Peter ------------- PR: https://git.openjdk.java.net/jdk/pull/5027
