On Wed, 1 Sep 2021 01:05:32 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 cleanup and more test case. Hi Peter, thanks for the patch and performance measurement. Yesterday I did a quick hack and observed similar result. I'm excited to see the performance improvement in the `Const` cases without the need of spinning the hidden class and it's a simplification. On the other hand, I'm concerned of the regression of the `Var` case for field access (80%) whereas the regression of the `Var` case for method may be tolerable (9-10%) [1]. We don't have a representative benchmark for core reflection that represents the performance charactistics of real-world applications/frameworks. I can't make a conclusion whether regression of `Var` cases matter in real-world applications/frameworks until customers run their benchmarks. We have to take the conservative side to ensure no significant regression based on micro-benchmarks result. I'm afraid it's not a good state to accept. Claes is on vacation and we should get his opinion. I tried one suggestion from Vladimir Ivanov to ensure `MethodHandle::customize` on top of your patch. But it does not seem to help. In any case, the patch to make `methodAccessor` and `fieldAccessor` not volatile is a good improvement by itself (not to be a volatile field). I would suggest to fix that separately from JEP 416. For JEP 416, two possible options: 1. Keep it as is and no significant performance regression. Continue to work on the performance improvement to remove the need of spinning class. 2. Disable the spinning hidden class and have a flag to enable it so that customers can workaround if having performance issue. However, performance regression on field access is not desirable. So I would prefer option 1 and continue work on the performance improvement as follow-up work. What do you think? [1] https://jmh.morethan.io/?gists=7b2399cea71d12dfd3434701ddeed026,b11842fbd48ebbd9234251fded50d52e ------------- PR: https://git.openjdk.java.net/jdk/pull/5027
