RFR: implementation for JEP 334: JVM Constants API
Hi all, Please review the proposed implementation for JEP 334 [1]. The webrev can be found at [2] and the javadoc at [3]. Thanks, Vicente [1] https://bugs.openjdk.java.net/browse/JDK-8203252 [2] http://cr.openjdk.java.net/~vromero/constant.api/webrev.07/constants.api.pat... [3] http://cr.openjdk.java.net/~vromero/constant.api/javadoc.07/overview-summary... PS. We are offering a MacBook Wheel to the authors of the first 5 comments :)
I think using FieldTypeDescriptor is misleading. since that is used for param types and return types. I propose SimpleTypeDescriptor or VariableTypeDescriptor. On Wed, May 23, 2018 at 11:11 PM, Vicente Romero <vicente.romero@oracle.com> wrote:
Hi all,
Please review the proposed implementation for JEP 334 [1]. The webrev can be found at [2] and the javadoc at [3].
Thanks, Vicente
[1] https://bugs.openjdk.java.net/browse/JDK-8203252 [2] http://cr.openjdk.java.net/~vromero/constant.api/webrev.07/ constants.api.patch [3] http://cr.openjdk.java.net/~vromero/constant.api/javadoc.07/ overview-summary.html
PS. We are offering a MacBook Wheel to the authors of the first 5 comments :)
-- Best Regards, Ali Ebrahimi
This is the terminology used by the JVMS (JVMS 4.3.2). Since these things model classfile entities, we adopted the JVMS terminology. On 5/24/2018 1:32 AM, Ali Ebrahimi wrote:
I think using FieldTypeDescriptor is misleading. since that is used for param types and return types. I propose SimpleTypeDescriptor or VariableTypeDescriptor.
On Wed, May 23, 2018 at 11:11 PM, Vicente Romero <vicente.romero@oracle.com> wrote:
Hi all,
Please review the proposed implementation for JEP 334 [1]. The webrev can be found at [2] and the javadoc at [3].
Thanks, Vicente
[1] https://bugs.openjdk.java.net/browse/JDK-8203252 [2] http://cr.openjdk.java.net/~vromero/constant.api/webrev.07/ constants.api.patch [3] http://cr.openjdk.java.net/~vromero/constant.api/javadoc.07/ overview-summary.html
PS. We are offering a MacBook Wheel to the authors of the first 5 comments :)
Hi all, I've not taken a look to the code, so this is just my comments based on the javadoc. This version is far better than the previous one, given that all my other comments are "you should change that too ..., i don't like ..., etc", i want to first say that i'm very please by this new version. MethodHandleDesc.Kind: The JVMS defines names for these constants, why using different names here ? ClassDesc.isClassOrInterface() seems very specific, does it includes annotation or enums, how will it works when value types will be included ? it seems to be defined as !isPrimitive() and !isArray(), in that case, i think it can be removed. EnumDesc and VarHandleDesc, those should be static inner classes of respectively Enum and VarHandle, they are not part of the main API but nice addons. DynamicCallSiteDesc is not a Desc (a ConstantDesc), so the name is very confusing. ConstantDescs is a bag of constants, so if you want to use one, all of them need to be initialized, here we should own our own dog food and initialize these constants only when needed, so instead of being static final, they should be exposed as methods and implemented as a ldc on a ConstandDynamic. DynamicConstantDesc, it's not clear to me why this is not an interface like ClassDesc or MethodHandleDesc For ConstantClassDesc, ConstantMethodTypeDesc, ConstantMethodHandleDesc, do we really need to make these classes public, in theory they should not have a proper name because they have the same API as there interface counter parts and it will be easier to retrofit them as value type if there are not part of the public APIs. By doing that, i believe that FieldDescriptor and TypeDescriptor will not need to be generics anymore. It seems that java.lang.invoke.Intrinsics has disappear (at least from the javadoc). Constable.describeConstable() is weirdly named, does asConstantDesc better ? Some implementation of Constable do not have the return type of describeConstable being tidy up (i.e. the return type uses the Optional<? ...> instead of a more specific one). I really dislike the fact that there is a method describeConstable in java.lang.Enum. Given the number of people that are interested by describeConstable(), i think this interface should be removed and static methods should be used instead. And fundamentally, i do not like the fact that you can go from a live object to its ConstantDesc representation, people will abuse of that like currently we use a java.lang.Class instead of a ClassDesc. This API, for me, goes in the wrong direction and pollute too many classes. For javadoc for java.lang shows too many classes, not only the one that are impacted by the JEP 334. regards, Rémi ----- Mail original -----
De: "Vicente Romero" <vicente.romero@oracle.com> À: "amber-dev" <amber-dev@openjdk.java.net>, "core-libs-dev" <core-libs-dev@openjdk.java.net> Envoyé: Mercredi 23 Mai 2018 20:41:11 Objet: RFR: implementation for JEP 334: JVM Constants API
Hi all,
Please review the proposed implementation for JEP 334 [1]. The webrev can be found at [2] and the javadoc at [3].
Thanks, Vicente
[1] https://bugs.openjdk.java.net/browse/JDK-8203252 [2] http://cr.openjdk.java.net/~vromero/constant.api/webrev.07/constants.api.pat... [3] http://cr.openjdk.java.net/~vromero/constant.api/javadoc.07/overview-summary...
PS. We are offering a MacBook Wheel to the authors of the first 5 comments :)
Thanks for the review and the great comments!
This version is far better than the previous one,
... or the previous seven :)
MethodHandleDesc.Kind: The JVMS defines names for these constants, why using different names here ?
Mostly for readability of client code. Happy to consider alternatives.
ClassDesc.isClassOrInterface() seems very specific, does it includes annotation or enums, how will it works when value types will be included ?
Hah, this was added based on a previous review, where someone said "you have isArray() and isPrimitive(), but there are methods that only work on non-array, non-primitive (e.g., packageName()), so you should have a corresponding method for this, and then the isXxx methods partition the space." After some bikeshed painting, So isClassOrInterface(), which means "not array, not primitive", seemed the best choice. (The VM sees enums as classes and annotations as interfaces, and this is essentially a VM interop API.) For value types, value types will be classes ("codes like a class!") so they will be included by this method. We could have call this isClassWhoseDescriptorStartsWithL() but that seemed a bit less friendly, though that's basically the intent.
it seems to be defined as !isPrimitive() and !isArray(), in that case, i think it can be removed.
Tried that, see above :)
EnumDesc and VarHandleDesc, those should be static inner classes of respectively Enum and VarHandle, they are not part of the main API but nice addons.
Good thought! Will consider this (need to work through some of the other specializations we intend to add, to make sure this scales properly.)
DynamicCallSiteDesc is not a Desc (a ConstantDesc), so the name is very confusing.
It's not a ConstantDesc, but it _is_ a nominal descriptor. Just not a nominal descriptor for a (loadable) constant. The confusing bit is that its the only one of its kind, for now; later, I think this will become more common. (If we had descriptors for the non-loadable constants in the CP (e.g., NameAndType), they'd be in this boat too.)
DynamicConstantDesc, it's not clear to me why this is not an interface like ClassDesc or MethodHandleDesc Fair question; we went back and forth on this a few times. It could be shredded into a pair like ClassDesc/ConstantClassDesc. However, in reality, we expect that all implementations of DCD would want to extend the base implementation anyway, since it provides useful functionality for which there's no point in reinventing. Which means really DCD/ConstantDCD/AbstractDCD. Which is starting to seem a little silly? But I agree that things are a bit all over the map here. (This is in part due to trying to cover the seam between primitive and reference class mirrors with a single abstraction, because the alternative is far worse.)
For ConstantClassDesc, ConstantMethodTypeDesc, ConstantMethodHandleDesc, do we really need to make these classes public, in theory they should not have a proper name because they have the same API as there interface counter parts and it will be easier to retrofit them as value type if there are not part of the public APIs. By doing that, i believe that FieldDescriptor and TypeDescriptor will not need to be generics anymore.
The reason these were made public is that we expect bytecode APIs to case over them. If you're writing out an LDC based on a ConstantDesc, you can case over: String, Integer (and friends), Constant{Class,MT,MH}Desc, DynamicConstantDesc -- which are the concrete types that correspond exactly to the loadable CP types defined in JVMS -- and you're done. If we made ConstantMHDesc private, the API gets more complicated, because now there are a bunch of methods that only work on direct method handles.
It seems that java.lang.invoke.Intrinsics has disappear (at least from the javadoc).
We split JEP 334 off from 303; Intrinsics remains in 303, and will come later. This is the foundation.
Constable.describeConstable() is weirdly named, does asConstantDesc better ?
Yeah, we had quite a loooooong discussion on this...suffice it to say we considered every possible option, and then some. The basic challenge is that ConstantDesc implements Constable, and code like constantDesc.asConstantDesc() would totally look like a no-op -- but it's not! It's asking the constantDesc to _describe itself_ as a constantDesc. So calling it constantDesc.describeConstable() is more clearly asking a Constable to describe itself.
Some implementation of Constable do not have the return type of describeConstable being tidy up (i.e. the return type uses the Optional<? ...> instead of a more specific one).
I'll take a look at these. Some are trickier than they look.
I really dislike the fact that there is a method describeConstable in java.lang.Enum. Given the number of people that are interested by describeConstable(), i think this interface should be removed and static methods should be used instead. And fundamentally, i do not like the fact that you can go from a live object to its ConstantDesc representation, people will abuse of that like currently we use a java.lang.Class instead of a ClassDesc. This API, for me, goes in the wrong direction and pollute too many classes.
Yeah, this is a sharp edge. But there's also a reason for it; it enables the compiler to much more aggressively constant-fold things into condy. The problem is not the method, or the mechanism -- the mechanism is awesome. The problem is that this is a very low-level method bleeding into the Javadoc of some high-level things, which is sure to confuse users. (We have the same problem with the two new methods on String -- describeConstable and resolveConstantDesc. Both are kind of silly on String, since they are basically identities. In fact, the reason these methods are named so oddly is because they are going to show up in the API of some very general classes, like String.)
EnumDesc and VarHandleDesc, those should be static inner classes of respectively Enum and VarHandle, they are not part of the main API but nice addons.
After some thought, I am inclined to agree. Here's the rationale. There are some built in constant types; they have Desc counterparts in JLIC. Then there are types that are Constable, that need a companion subtype of DynamicConstantDesc. These classes should bring their own. What to call them? Well, we could leave the names the same, which is wordy: Enum.EnumDesc, but plays nicely with static imports. We could call them something like Descriptor, but then if static imported, they'd like conflict. Or, cleverly, we could call them ConstantDesc, which if people static-imported them, would shoot their own feet, and therefore learn not to do that :) I like the latter for its corrective behavior but I think the first is probably best.
Hi Vincente, the following files do not have the copyright header: src/java.base/share/classes/java/lang/invoke/FieldTypeDescriptor.java src/java.base/share/classes/java/lang/invoke/MethodTypeDescriptor.java src/java.base/share/classes/java/lang/invoke/TypeDescriptor.java In the following files the year in the copyright should be updated: test/jdk/java/lang/invoke/constant/ClassRefTest.java test/jdk/java/lang/invoke/constant/CondyRefTest.java test/jdk/java/lang/invoke/constant/MethodHandleRefTest.java test/jdk/java/lang/invoke/constant/MethodTypeRefTest.java test/jdk/java/lang/invoke/constant/SymbolicRefTest.java test/jdk/java/lang/invoke/constant/TypeDescriptorTest.java And I also think that Constable.describeConstable() is weirdly named. What about: #describe() #getDescriptor() #getConstantDescriptor() #toConstantDescriptor() Best regards, Andrej Golovnin On Wed, May 23, 2018 at 8:41 PM, Vicente Romero <vicente.romero@oracle.com> wrote:
Hi all,
Please review the proposed implementation for JEP 334 [1]. The webrev can be found at [2] and the javadoc at [3].
Thanks, Vicente
[1] https://bugs.openjdk.java.net/browse/JDK-8203252 [2] http://cr.openjdk.java.net/~vromero/constant.api/webrev.07/constants.api.pat... [3] http://cr.openjdk.java.net/~vromero/constant.api/javadoc.07/overview-summary...
PS. We are offering a MacBook Wheel to the authors of the first 5 comments :)
Thanks for the comments so far, I have uploaded another iteration of the implementation + javadoc Vicente [1] http://cr.openjdk.java.net/~vromero/constant.api/webrev.08 [2] http://cr.openjdk.java.net/~vromero/constant.api/javadoc.08 On 05/23/2018 02:41 PM, Vicente Romero wrote:
Hi all,
Please review the proposed implementation for JEP 334 [1]. The webrev can be found at [2] and the javadoc at [3].
Thanks, Vicente
[1] https://bugs.openjdk.java.net/browse/JDK-8203252 [2] http://cr.openjdk.java.net/~vromero/constant.api/webrev.07/constants.api.pat... [3] http://cr.openjdk.java.net/~vromero/constant.api/javadoc.07/overview-summary...
PS. We are offering a MacBook Wheel to the authors of the first 5 comments :)
Hi all, I have uploaded another iteration of the implementation of the constants API + the javadoc. Thanks for the comments so far, Vicente [1] http://cr.openjdk.java.net/~vromero/constant.api/webrev.09 [2] http://cr.openjdk.java.net/~vromero/constant.api/javadoc.09 On 05/24/2018 09:09 PM, Vicente Romero wrote:
Thanks for the comments so far, I have uploaded another iteration of the implementation + javadoc
Vicente
[1] http://cr.openjdk.java.net/~vromero/constant.api/webrev.08 [2] http://cr.openjdk.java.net/~vromero/constant.api/javadoc.08
On 05/23/2018 02:41 PM, Vicente Romero wrote:
Hi all,
Please review the proposed implementation for JEP 334 [1]. The webrev can be found at [2] and the javadoc at [3].
Thanks, Vicente
[1] https://bugs.openjdk.java.net/browse/JDK-8203252 [2] http://cr.openjdk.java.net/~vromero/constant.api/webrev.07/constants.api.pat... [3] http://cr.openjdk.java.net/~vromero/constant.api/javadoc.07/overview-summary...
PS. We are offering a MacBook Wheel to the authors of the first 5 comments :)
Hi all, There is a new iteration of the implementation at: [1] http://cr.openjdk.java.net/~vromero/constant.api/webrev.10 [2] http://cr.openjdk.java.net/~vromero/constant.api/javadoc.10 main changes in this iteration: we have added additional tests to improve code coverage. Thanks, Vicente On 05/31/2018 05:09 PM, Vicente Romero wrote:
Hi all,
I have uploaded another iteration of the implementation of the constants API + the javadoc.
Thanks for the comments so far, Vicente
[1] http://cr.openjdk.java.net/~vromero/constant.api/webrev.09 [2] http://cr.openjdk.java.net/~vromero/constant.api/javadoc.09
On 05/24/2018 09:09 PM, Vicente Romero wrote:
Thanks for the comments so far, I have uploaded another iteration of the implementation + javadoc
Vicente
[1] http://cr.openjdk.java.net/~vromero/constant.api/webrev.08 [2] http://cr.openjdk.java.net/~vromero/constant.api/javadoc.08
On 05/23/2018 02:41 PM, Vicente Romero wrote:
Hi all,
Please review the proposed implementation for JEP 334 [1]. The webrev can be found at [2] and the javadoc at [3].
Thanks, Vicente
[1] https://bugs.openjdk.java.net/browse/JDK-8203252 [2] http://cr.openjdk.java.net/~vromero/constant.api/webrev.07/constants.api.pat... [3] http://cr.openjdk.java.net/~vromero/constant.api/javadoc.07/overview-summary...
PS. We are offering a MacBook Wheel to the authors of the first 5 comments :)
participants (5)
-
Ali Ebrahimi
-
Andrej Golovnin
-
Brian Goetz
-
Remi Forax
-
Vicente Romero