Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v17]
On Wed, 13 Apr 2022 21:21:25 GMT, liach wrote: >> src/java.base/share/classes/java/lang/module/ModuleDescriptor.java line 167: >> >>> 165: * but is optional in the dynamic phase, during execution. >>> 166: */ >>> 167: STATIC(AccessFlag.STATIC.mask()), >> >> This is actually `AccessFlag.STATIC_PHASE` (`0x0040`), and not >> `AccessFlag.STATIC` (`0x0008`): >> Suggestion: >> >> STATIC(AccessFlag.STATIC_PHASE.mask()), > >> In the current hodgepodge AccessFlag, we have STATIC and STATIC_PHASE, and >> the incorrect ModuleDescriptor.accessFlags().contains(AccessFlag.STATIC) >> call is much more subtle, especially to new users of this class. Arguably, >> this misuse would be way worse than that in the distinct enum case. > > Oops, didn't know this already happened. Good spot right there. Corrected to STATIC_PHASE in subsequent push; thanks. - PR: https://git.openjdk.org/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v17]
On Fri, 29 Apr 2022 20:34:21 GMT, Mandy Chung wrote: > I took a closer look at the proposed APIs. I think it's in a good state to > target for 19. I skimmed on the existing JDK usage of `getModifiers` other > than a trivial test e.g. is public, final, etc and the proposed API works > well (btw no plan to convert them and just use those cases for validation). > > The value of `ACC_SUPER` and `ACC_STRICT` could possibly be reused for new > access flags. It may be useful to document when the flag becomes obsolete. > > Nit: the enum constants are listed in the order of the mask value, which I > like. Some enum constants reference the `Modifer` constants but I think it'd > help to see the mask value here consistently for all entries. One go-to place > in the source if I want to find the value of different flags. I've pushed a changeset to add the mask values to the constants' javadoc. Following a don't-repeat-yourself approach, I would have preferred to do this with a @-value tage using the java.lang.reflect.Modifier value (where available), but the @-value tags doesn't seem to offer any formatting so I added the values separately. - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v17]
On Sat, 5 Mar 2022 19:54:44 GMT, Joe Darcy wrote:
>> This is an early review of changes to better model JVM access flags, that is
>> "modifiers" like public, protected, etc. but explicitly at a VM level.
>>
>> Language level modifiers and JVM level access flags are closely related, but
>> distinct. There are concepts that overlap in the two domains (public,
>> private, etc.), others that only have a language-level modifier (sealed),
>> and still others that only have an access flag (synthetic).
>>
>> The existing java.lang.reflect.Modifier class is inadequate to model these
>> subtleties. For example, the bit positions used by access flags on different
>> kinds of elements overlap (such as "volatile" for fields and "bridge" for
>> methods. Just having a raw integer does not provide sufficient context to
>> decode the corresponding language-level string. Methods like
>> Modifier.methodModifiers() were introduced to cope with this situation.
>>
>> With additional modifiers and flags on the horizon with projects like
>> Valhalla, addressing the existent modeling deficiency now ahead of time is
>> reasonable before further strain is introduced.
>>
>> This PR in its current form is meant to give the overall shape of the API.
>> It is missing implementations to map from, say, method modifiers to access
>> flags, taking into account overlaps in bit positions.
>>
>> The CSR https://bugs.openjdk.java.net/browse/JDK-8281660 will be filled in
>> once the API is further along.
>
> Joe Darcy has updated the pull request with a new target base due to a merge
> or a rebase. The incremental webrev excludes the unrelated changes brought in
> by the merge/rebase. The pull request contains 26 additional commits since
> the last revision:
>
> - Respond to review feedback.
> - Merge branch 'master' into JDK-8266670
> - Make workding changes suggested in review feedback.
> - Merge branch 'master' into JDK-8266670
> - Typo fix; add implSpec to Executable.
> - Appease jcheck.
> - Fix some bugs found by inspection, docs cleanup.
> - Merge branch 'master' into JDK-8266670
> - Initial support for accessFlags methods
> - Add mask to access flag functionality.
> - ... and 16 more:
> https://git.openjdk.java.net/jdk/compare/7ac698ba...14980605
I took a closer look at the proposed APIs. I think it's in a good state to
target for 19. I skimmed on the existing JDK usage of `getModifiers` other
than a trivial test e.g. is public, final, etc and the proposed API works well
(btw no plan to convert them and just use those cases for validation).
The value of `ACC_SUPER` and `ACC_STRICT` could possibly be reused for new
access flags. It may be useful to document when the flag becomes obsolete.
Nit: the enum constants are listed in the order of the mask value, which I
like. Some enum constants reference the `Modifer` constants but I think it'd
help to see the mask value here consistently for all entries. One go-to place
in the source if I want to find the value of different flags.
src/java.base/share/classes/java/lang/Class.java line 1328:
> 1326: * @since 19
> 1327: */
> 1328: public Set accessFlags() {
The access flags of a hidden class has no difference than that of a normal
class. A hidden class is created with normal `ClassFile` except that it's
created via `Lookup::defineHiddenClass` API.
I think the `Class::accessFlags` method should specify the access flags for
primitive type, void, and array classes as `Class::getModifiers` specified.
src/java.base/share/classes/java/lang/module/ModuleDescriptor.java line 216:
> 214:
> 215: /**
> 216: * {@return an unmodifiable set of the module {@linkplain
> AccessFlag
Suggestion:
* {@return a possibly-empty unmodifiable set of the module {@linkplain
AccessFlag
as specified in the `@return` of the `modifiers()` method. Same comment
applies to the `accessFlags` method in `ModuleDescriptor.Opens` and other
classes.
src/java.base/share/classes/java/lang/reflect/AccessFlag.java line 44:
> 42: * not have an access flag, such as {@code sealed} (JVMS
> 43: * {@jvms 4.7.31}) and some access flags have no corresponding
> 44: * modifier, such as {@linkplain SYNTHETIC synthetic}.
Suggestion:
* modifier, such as {@linkplain #SYNTHETIC synthetic}.
src/java.base/share/classes/java/lang/reflect/AccessFlag.java line 186:
> 184: /**
> 185: * The access flag {@code ACC_ABSTRACT}, corresponding to the
> 186: * source modifier {@code link Modifier#ABSTRACT abstract}.
Suggestion:
* source modifier {@link Modifier#ABSTRACT abstract}.
src/java.base/share/classes/java/lang/reflect/AccessFlag.java line 306:
> 304: * Note that since these locations represent class file structures
> 305: * rather than language structures many language structures, such
> 306: * as constructors and interfaces, are not present.
missing `@since 19`
-
PR: https://git.ope
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v17]
On Wed, 13 Apr 2022 21:12:40 GMT, ExE Boss wrote: >> Joe Darcy has updated the pull request with a new target base due to a merge >> or a rebase. The incremental webrev excludes the unrelated changes brought >> in by the merge/rebase. The pull request contains 26 additional commits >> since the last revision: >> >> - Respond to review feedback. >> - Merge branch 'master' into JDK-8266670 >> - Make workding changes suggested in review feedback. >> - Merge branch 'master' into JDK-8266670 >> - Typo fix; add implSpec to Executable. >> - Appease jcheck. >> - Fix some bugs found by inspection, docs cleanup. >> - Merge branch 'master' into JDK-8266670 >> - Initial support for accessFlags methods >> - Add mask to access flag functionality. >> - ... and 16 more: >> https://git.openjdk.java.net/jdk/compare/afd02683...14980605 > > src/java.base/share/classes/java/lang/module/ModuleDescriptor.java line 167: > >> 165: * but is optional in the dynamic phase, during execution. >> 166: */ >> 167: STATIC(AccessFlag.STATIC.mask()), > > This is actually `AccessFlag.STATIC_PHASE` (`0x0040`), and not > `AccessFlag.STATIC` (`0x0008`): > Suggestion: > > STATIC(AccessFlag.STATIC_PHASE.mask()), > In the current hodgepodge AccessFlag, we have STATIC and STATIC_PHASE, and > the incorrect ModuleDescriptor.accessFlags().contains(AccessFlag.STATIC) call > is much more subtle, especially to new users of this class. Arguably, this > misuse would be way worse than that in the distinct enum case. Oops, didn't know this already happened. Good spot right there. - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v17]
On Sat, 5 Mar 2022 19:54:44 GMT, Joe Darcy wrote: >> This is an early review of changes to better model JVM access flags, that is >> "modifiers" like public, protected, etc. but explicitly at a VM level. >> >> Language level modifiers and JVM level access flags are closely related, but >> distinct. There are concepts that overlap in the two domains (public, >> private, etc.), others that only have a language-level modifier (sealed), >> and still others that only have an access flag (synthetic). >> >> The existing java.lang.reflect.Modifier class is inadequate to model these >> subtleties. For example, the bit positions used by access flags on different >> kinds of elements overlap (such as "volatile" for fields and "bridge" for >> methods. Just having a raw integer does not provide sufficient context to >> decode the corresponding language-level string. Methods like >> Modifier.methodModifiers() were introduced to cope with this situation. >> >> With additional modifiers and flags on the horizon with projects like >> Valhalla, addressing the existent modeling deficiency now ahead of time is >> reasonable before further strain is introduced. >> >> This PR in its current form is meant to give the overall shape of the API. >> It is missing implementations to map from, say, method modifiers to access >> flags, taking into account overlaps in bit positions. >> >> The CSR https://bugs.openjdk.java.net/browse/JDK-8281660 will be filled in >> once the API is further along. > > Joe Darcy has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains 26 additional commits since > the last revision: > > - Respond to review feedback. > - Merge branch 'master' into JDK-8266670 > - Make workding changes suggested in review feedback. > - Merge branch 'master' into JDK-8266670 > - Typo fix; add implSpec to Executable. > - Appease jcheck. > - Fix some bugs found by inspection, docs cleanup. > - Merge branch 'master' into JDK-8266670 > - Initial support for accessFlags methods > - Add mask to access flag functionality. > - ... and 16 more: > https://git.openjdk.java.net/jdk/compare/364bd126...14980605 src/java.base/share/classes/java/lang/module/ModuleDescriptor.java line 167: > 165: * but is optional in the dynamic phase, during execution. > 166: */ > 167: STATIC(AccessFlag.STATIC.mask()), This is actually `AccessFlag.STATIC_PHASE` (`0x0040`), and not `AccessFlag.STATIC` (`0x0008`): Suggestion: STATIC(AccessFlag.STATIC_PHASE.mask()), - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v17]
On Sat, 5 Mar 2022 19:54:44 GMT, Joe Darcy wrote: >> This is an early review of changes to better model JVM access flags, that is >> "modifiers" like public, protected, etc. but explicitly at a VM level. >> >> Language level modifiers and JVM level access flags are closely related, but >> distinct. There are concepts that overlap in the two domains (public, >> private, etc.), others that only have a language-level modifier (sealed), >> and still others that only have an access flag (synthetic). >> >> The existing java.lang.reflect.Modifier class is inadequate to model these >> subtleties. For example, the bit positions used by access flags on different >> kinds of elements overlap (such as "volatile" for fields and "bridge" for >> methods. Just having a raw integer does not provide sufficient context to >> decode the corresponding language-level string. Methods like >> Modifier.methodModifiers() were introduced to cope with this situation. >> >> With additional modifiers and flags on the horizon with projects like >> Valhalla, addressing the existent modeling deficiency now ahead of time is >> reasonable before further strain is introduced. >> >> This PR in its current form is meant to give the overall shape of the API. >> It is missing implementations to map from, say, method modifiers to access >> flags, taking into account overlaps in bit positions. >> >> The CSR https://bugs.openjdk.java.net/browse/JDK-8281660 will be filled in >> once the API is further along. > > Joe Darcy has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains 26 additional commits since > the last revision: > > - Respond to review feedback. > - Merge branch 'master' into JDK-8266670 > - Make workding changes suggested in review feedback. > - Merge branch 'master' into JDK-8266670 > - Typo fix; add implSpec to Executable. > - Appease jcheck. > - Fix some bugs found by inspection, docs cleanup. > - Merge branch 'master' into JDK-8266670 > - Initial support for accessFlags methods > - Add mask to access flag functionality. > - ... and 16 more: > https://git.openjdk.java.net/jdk/compare/0053820b...14980605 Here, I argue for the case of splitting different types of access flags into distinct enums, such as `MemberAccessFlag`, `ModuleRequiresAccessFlag`, `ModuleExportsAccessFlag` implementing a sealed interface `AccessFlag` that keeps all its existing methods, since most of those access flags will never be returned in the same collection. The `accessFlags()` method should return a `Set`, which is 1. safe as the set is read-only; 2. can be overridden with `Set` (For forward compability, we can make `MemberAccessFlag` an interface with static final fields, implemented by some package-private enum in case we want to split it into more specific types down the road). But you may ask, what about `SYNTHETIC`, `MANDATED`, `FINAL`? Aren't they shared by almost all of the locations? What if users test with incorrect enum instance, such as looking for the `MemberAccessFlag.SYNTHETIC` in `moduleDescriptor.accessFlags()`? This problem can be prevented: 1. by making `ModuleDescriptor.accessFlags()` return `Set`, thus the IDE can easily detect misuse when they insert the access flag of a different type; 2. In the current hodgepodge `AccessFlag`, we have `STATIC` and `STATIC_PHASE`, and the incorrect `ModuleDescriptor.accessFlags().contains(AccessFlag.STATIC)` call is much more subtle, especially to new users of this class. Arguably, this misuse would be way worse than that in the distinct enum case. - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v17]
On Sat, 5 Mar 2022 19:54:44 GMT, Joe Darcy wrote: >> This is an early review of changes to better model JVM access flags, that is >> "modifiers" like public, protected, etc. but explicitly at a VM level. >> >> Language level modifiers and JVM level access flags are closely related, but >> distinct. There are concepts that overlap in the two domains (public, >> private, etc.), others that only have a language-level modifier (sealed), >> and still others that only have an access flag (synthetic). >> >> The existing java.lang.reflect.Modifier class is inadequate to model these >> subtleties. For example, the bit positions used by access flags on different >> kinds of elements overlap (such as "volatile" for fields and "bridge" for >> methods. Just having a raw integer does not provide sufficient context to >> decode the corresponding language-level string. Methods like >> Modifier.methodModifiers() were introduced to cope with this situation. >> >> With additional modifiers and flags on the horizon with projects like >> Valhalla, addressing the existent modeling deficiency now ahead of time is >> reasonable before further strain is introduced. >> >> This PR in its current form is meant to give the overall shape of the API. >> It is missing implementations to map from, say, method modifiers to access >> flags, taking into account overlaps in bit positions. >> >> The CSR https://bugs.openjdk.java.net/browse/JDK-8281660 will be filled in >> once the API is further along. > > Joe Darcy has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains 26 additional commits since > the last revision: > > - Respond to review feedback. > - Merge branch 'master' into JDK-8266670 > - Make workding changes suggested in review feedback. > - Merge branch 'master' into JDK-8266670 > - Typo fix; add implSpec to Executable. > - Appease jcheck. > - Fix some bugs found by inspection, docs cleanup. > - Merge branch 'master' into JDK-8266670 > - Initial support for accessFlags methods > - Add mask to access flag functionality. > - ... and 16 more: > https://git.openjdk.java.net/jdk/compare/f55e4cdc...14980605 Keep-alive. - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v17]
On Sat, 5 Mar 2022 19:54:44 GMT, Joe Darcy wrote: >> This is an early review of changes to better model JVM access flags, that is >> "modifiers" like public, protected, etc. but explicitly at a VM level. >> >> Language level modifiers and JVM level access flags are closely related, but >> distinct. There are concepts that overlap in the two domains (public, >> private, etc.), others that only have a language-level modifier (sealed), >> and still others that only have an access flag (synthetic). >> >> The existing java.lang.reflect.Modifier class is inadequate to model these >> subtleties. For example, the bit positions used by access flags on different >> kinds of elements overlap (such as "volatile" for fields and "bridge" for >> methods. Just having a raw integer does not provide sufficient context to >> decode the corresponding language-level string. Methods like >> Modifier.methodModifiers() were introduced to cope with this situation. >> >> With additional modifiers and flags on the horizon with projects like >> Valhalla, addressing the existent modeling deficiency now ahead of time is >> reasonable before further strain is introduced. >> >> This PR in its current form is meant to give the overall shape of the API. >> It is missing implementations to map from, say, method modifiers to access >> flags, taking into account overlaps in bit positions. >> >> The CSR https://bugs.openjdk.java.net/browse/JDK-8281660 will be filled in >> once the API is further along. > > Joe Darcy has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains 26 additional commits since > the last revision: > > - Respond to review feedback. > - Merge branch 'master' into JDK-8266670 > - Make workding changes suggested in review feedback. > - Merge branch 'master' into JDK-8266670 > - Typo fix; add implSpec to Executable. > - Appease jcheck. > - Fix some bugs found by inspection, docs cleanup. > - Merge branch 'master' into JDK-8266670 > - Initial support for accessFlags methods > - Add mask to access flag functionality. > - ... and 16 more: > https://git.openjdk.java.net/jdk/compare/f992eace...14980605 > > > The mapping from Location to AccessFlag(s) could be implemented event > > > without using a Map. You just have to be careful not to use EnumSet for > > > that (i.e. before AccessFlag enum constants are fully initialized) - an > > > ArrayList is better for this case anyway. Also, conversion from > > > ModuleDescriptor modifier(s) to AccessFlag(s) could be done without first > > > creating a bitmask and then re-constructing a set of AccessFlag(s) from > > > it. Reflective object modifiers can only be translated via bitmask, but > > > various ModuleDescriptor Modifier(s) may have a direct mapping to > > > corresponding AccessFlag(s). For example: > > > [plevart@5a3cd8f](https://github.com/plevart/jdk/commit/5a3cd8f6851a0deae7e3e5028bba9a51d7863929) > > > > > > Yes, I made the same observation for the module-related modifiers when > > coding this up; I'll look to add some API support to avoid the need to > > detour through bitmasks. > > To get the public API worked out, I wanted to avoid complications in > > inter-dependent class initialization. Thanks for suggesting an alternative; > > I'll consider that when I resume work on this PR (need to start writing > > some tests...) Thanks. > > On a second thought, maybe the common bitmask representation is the way to > go. One one hand it must be computed from the set of module-related Modifier > set(s), but then again such bitmask is very easy to cache economically (a > single int field, no synchronization). When you have a bitmask at hand and a > list of all possible elements (the universe), a very efficient implementation > of a Set is possible (similar to EnumSet). Such AccessFlagSet instances do > not need to be cached since they are just thin views over existing > "permanent" structures and will, as such, typically be optimized away by JIT. > Note the particularly efficient .contains() implementation which just checks > two bitmasks (the flag mask and the location mask). > > [plevart@de9f261](https://github.com/plevart/jdk/commit/de9f2614da56775be4ae07c6781cbec9fbd39930) FWIW, I did link this issue to the RFE [JDK-8145048](https://bugs.openjdk.java.net/browse/JDK-8145048) consider enhancements to EnumMap and EnumSet, which includes a request an immutable EnumSet. - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v17]
On Sat, 5 Mar 2022 19:54:44 GMT, Joe Darcy wrote: >> This is an early review of changes to better model JVM access flags, that is >> "modifiers" like public, protected, etc. but explicitly at a VM level. >> >> Language level modifiers and JVM level access flags are closely related, but >> distinct. There are concepts that overlap in the two domains (public, >> private, etc.), others that only have a language-level modifier (sealed), >> and still others that only have an access flag (synthetic). >> >> The existing java.lang.reflect.Modifier class is inadequate to model these >> subtleties. For example, the bit positions used by access flags on different >> kinds of elements overlap (such as "volatile" for fields and "bridge" for >> methods. Just having a raw integer does not provide sufficient context to >> decode the corresponding language-level string. Methods like >> Modifier.methodModifiers() were introduced to cope with this situation. >> >> With additional modifiers and flags on the horizon with projects like >> Valhalla, addressing the existent modeling deficiency now ahead of time is >> reasonable before further strain is introduced. >> >> This PR in its current form is meant to give the overall shape of the API. >> It is missing implementations to map from, say, method modifiers to access >> flags, taking into account overlaps in bit positions. >> >> The CSR https://bugs.openjdk.java.net/browse/JDK-8281660 will be filled in >> once the API is further along. > > Joe Darcy has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains 26 additional commits since > the last revision: > > - Respond to review feedback. > - Merge branch 'master' into JDK-8266670 > - Make workding changes suggested in review feedback. > - Merge branch 'master' into JDK-8266670 > - Typo fix; add implSpec to Executable. > - Appease jcheck. > - Fix some bugs found by inspection, docs cleanup. > - Merge branch 'master' into JDK-8266670 > - Initial support for accessFlags methods > - Add mask to access flag functionality. > - ... and 16 more: > https://git.openjdk.java.net/jdk/compare/b7d02b1c...14980605 > Such AccessFlagSet ... > > [plevart@de9f261](https://github.com/plevart/jdk/commit/de9f2614da56775be4ae07c6781cbec9fbd39930) ...also needs proper hashCode/equals/toString implementations of course: https://github.com/plevart/jdk/commit/1ad5e1f925029908ecf8b21d793c25aec34a80cb - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v17]
On Tue, 8 Mar 2022 00:19:58 GMT, Joe Darcy wrote: > > The mapping from Location to AccessFlag(s) could be implemented event > > without using a Map. You just have to be careful not to use EnumSet for > > that (i.e. before AccessFlag enum constants are fully initialized) - an > > ArrayList is better for this case anyway. Also, conversion from > > ModuleDescriptor modifier(s) to AccessFlag(s) could be done without first > > creating a bitmask and then re-constructing a set of AccessFlag(s) from it. > > Reflective object modifiers can only be translated via bitmask, but various > > ModuleDescriptor Modifier(s) may have a direct mapping to corresponding > > AccessFlag(s). For example: > > [plevart@5a3cd8f](https://github.com/plevart/jdk/commit/5a3cd8f6851a0deae7e3e5028bba9a51d7863929) > > Yes, I made the same observation for the module-related modifiers when coding > this up; I'll look to add some API support to avoid the need to detour > through bitmasks. > > To get the public API worked out, I wanted to avoid complications in > inter-dependent class initialization. Thanks for suggesting an alternative; > I'll consider that when I resume work on this PR (need to start writing some > tests...) Thanks. On a second thought, maybe the common bitmask representation is the way to go. One one hand it must be computed from the set of module-related Modifier set(s), but then again such bitmask is very easy to cache economically (a single int field, no synchronization). When you have a bitmask at hand and a list of all possible elements (the universe), a very efficient implementation of a Set is possible (similar to EnumSet). Such AccessFlagSet instances do not need to be cached since they are just thin views over existing "permanent" structures and will, as such, typically be optimized away by JIT. Note the particularly efficient .contains() implementation which just checks two bitmasks (the flag mask and the location mask). https://github.com/plevart/jdk/commit/de9f2614da56775be4ae07c6781cbec9fbd39930 - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v17]
On Sat, 5 Mar 2022 19:54:44 GMT, Joe Darcy wrote: >> This is an early review of changes to better model JVM access flags, that is >> "modifiers" like public, protected, etc. but explicitly at a VM level. >> >> Language level modifiers and JVM level access flags are closely related, but >> distinct. There are concepts that overlap in the two domains (public, >> private, etc.), others that only have a language-level modifier (sealed), >> and still others that only have an access flag (synthetic). >> >> The existing java.lang.reflect.Modifier class is inadequate to model these >> subtleties. For example, the bit positions used by access flags on different >> kinds of elements overlap (such as "volatile" for fields and "bridge" for >> methods. Just having a raw integer does not provide sufficient context to >> decode the corresponding language-level string. Methods like >> Modifier.methodModifiers() were introduced to cope with this situation. >> >> With additional modifiers and flags on the horizon with projects like >> Valhalla, addressing the existent modeling deficiency now ahead of time is >> reasonable before further strain is introduced. >> >> This PR in its current form is meant to give the overall shape of the API. >> It is missing implementations to map from, say, method modifiers to access >> flags, taking into account overlaps in bit positions. >> >> The CSR https://bugs.openjdk.java.net/browse/JDK-8281660 will be filled in >> once the API is further along. > > Joe Darcy has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains 26 additional commits since > the last revision: > > - Respond to review feedback. > - Merge branch 'master' into JDK-8266670 > - Make workding changes suggested in review feedback. > - Merge branch 'master' into JDK-8266670 > - Typo fix; add implSpec to Executable. > - Appease jcheck. > - Fix some bugs found by inspection, docs cleanup. > - Merge branch 'master' into JDK-8266670 > - Initial support for accessFlags methods > - Add mask to access flag functionality. > - ... and 16 more: > https://git.openjdk.java.net/jdk/compare/012ab186...14980605 > The mapping from Location to AccessFlag(s) could be implemented event without > using a Map. You just have to be careful not to use EnumSet for that (i.e. > before AccessFlag enum constants are fully initialized) - an ArrayList is > better for this case anyway. Also, conversion from ModuleDescriptor > modifier(s) to AccessFlag(s) could be done without first creating a bitmask > and then re-constructing a set of AccessFlag(s) from it. Reflective object > modifiers can only be translated via bitmask, but various ModuleDescriptor > Modifier(s) may have a direct mapping to corresponding AccessFlag(s). For > example: > > [plevart@5a3cd8f](https://github.com/plevart/jdk/commit/5a3cd8f6851a0deae7e3e5028bba9a51d7863929) Yes, I made the same observation for the module-related modifiers when coding this up; I'll look to add some API support to avoid the need to detour through bitmasks. To get the public API worked out, I wanted to avoid complications in inter-dependent class initialization. Thanks for suggesting an alternative; I'll consider that when I resume work on this PR (need to start writing some tests...) Thanks. - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v17]
On Mon, 7 Mar 2022 18:20:23 GMT, Roger Riggs wrote: >> Joe Darcy has updated the pull request with a new target base due to a merge >> or a rebase. The incremental webrev excludes the unrelated changes brought >> in by the merge/rebase. The pull request contains 26 additional commits >> since the last revision: >> >> - Respond to review feedback. >> - Merge branch 'master' into JDK-8266670 >> - Make workding changes suggested in review feedback. >> - Merge branch 'master' into JDK-8266670 >> - Typo fix; add implSpec to Executable. >> - Appease jcheck. >> - Fix some bugs found by inspection, docs cleanup. >> - Merge branch 'master' into JDK-8266670 >> - Initial support for accessFlags methods >> - Add mask to access flag functionality. >> - ... and 16 more: >> https://git.openjdk.java.net/jdk/compare/36b93dbf...14980605 > > src/java.base/share/classes/java/lang/Class.java line 1334: > >> 1332: // allows PRIVATE, PROTECTED, and STATIC, which are not >> 1333: // allowed on Location.CLASS. >> 1334: return AccessFlag.maskToAccessFlags(getModifiers(), > > Computing and creating the Set every time seems like a high overhead > operation (compared to getModifiers()). > Caching either here (in the Member) or in AccessFlag.maskToAccessFlags would > be desirable. > Caching is idempotent so it should not need synchronization. For this phase of the work, I was trying to avoid premature optimization of building caches, etc. Such refinement should certainly be considered later on. - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v17]
On Sat, 5 Mar 2022 19:54:44 GMT, Joe Darcy wrote: >> This is an early review of changes to better model JVM access flags, that is >> "modifiers" like public, protected, etc. but explicitly at a VM level. >> >> Language level modifiers and JVM level access flags are closely related, but >> distinct. There are concepts that overlap in the two domains (public, >> private, etc.), others that only have a language-level modifier (sealed), >> and still others that only have an access flag (synthetic). >> >> The existing java.lang.reflect.Modifier class is inadequate to model these >> subtleties. For example, the bit positions used by access flags on different >> kinds of elements overlap (such as "volatile" for fields and "bridge" for >> methods. Just having a raw integer does not provide sufficient context to >> decode the corresponding language-level string. Methods like >> Modifier.methodModifiers() were introduced to cope with this situation. >> >> With additional modifiers and flags on the horizon with projects like >> Valhalla, addressing the existent modeling deficiency now ahead of time is >> reasonable before further strain is introduced. >> >> This PR in its current form is meant to give the overall shape of the API. >> It is missing implementations to map from, say, method modifiers to access >> flags, taking into account overlaps in bit positions. >> >> The CSR https://bugs.openjdk.java.net/browse/JDK-8281660 will be filled in >> once the API is further along. > > Joe Darcy has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains 26 additional commits since > the last revision: > > - Respond to review feedback. > - Merge branch 'master' into JDK-8266670 > - Make workding changes suggested in review feedback. > - Merge branch 'master' into JDK-8266670 > - Typo fix; add implSpec to Executable. > - Appease jcheck. > - Fix some bugs found by inspection, docs cleanup. > - Merge branch 'master' into JDK-8266670 > - Initial support for accessFlags methods > - Add mask to access flag functionality. > - ... and 16 more: > https://git.openjdk.java.net/jdk/compare/aa899b84...14980605 src/java.base/share/classes/java/lang/Class.java line 1334: > 1332: // allows PRIVATE, PROTECTED, and STATIC, which are not > 1333: // allowed on Location.CLASS. > 1334: return AccessFlag.maskToAccessFlags(getModifiers(), Computing and creating the Set every time seems like a high overhead operation (compared to getModifiers()). Caching either here (in the Member) or in AccessFlag.maskToAccessFlags would be desirable. Caching is idempotent so it should not need synchronization. - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v17]
On Sat, 5 Mar 2022 19:54:44 GMT, Joe Darcy wrote: >> This is an early review of changes to better model JVM access flags, that is >> "modifiers" like public, protected, etc. but explicitly at a VM level. >> >> Language level modifiers and JVM level access flags are closely related, but >> distinct. There are concepts that overlap in the two domains (public, >> private, etc.), others that only have a language-level modifier (sealed), >> and still others that only have an access flag (synthetic). >> >> The existing java.lang.reflect.Modifier class is inadequate to model these >> subtleties. For example, the bit positions used by access flags on different >> kinds of elements overlap (such as "volatile" for fields and "bridge" for >> methods. Just having a raw integer does not provide sufficient context to >> decode the corresponding language-level string. Methods like >> Modifier.methodModifiers() were introduced to cope with this situation. >> >> With additional modifiers and flags on the horizon with projects like >> Valhalla, addressing the existent modeling deficiency now ahead of time is >> reasonable before further strain is introduced. >> >> This PR in its current form is meant to give the overall shape of the API. >> It is missing implementations to map from, say, method modifiers to access >> flags, taking into account overlaps in bit positions. >> >> The CSR https://bugs.openjdk.java.net/browse/JDK-8281660 will be filled in >> once the API is further along. > > Joe Darcy has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains 26 additional commits since > the last revision: > > - Respond to review feedback. > - Merge branch 'master' into JDK-8266670 > - Make workding changes suggested in review feedback. > - Merge branch 'master' into JDK-8266670 > - Typo fix; add implSpec to Executable. > - Appease jcheck. > - Fix some bugs found by inspection, docs cleanup. > - Merge branch 'master' into JDK-8266670 > - Initial support for accessFlags methods > - Add mask to access flag functionality. > - ... and 16 more: > https://git.openjdk.java.net/jdk/compare/6eb09aa6...14980605 The mapping from Location to AccessFlag(s) could be implemented event without using a Map. You just have to be careful not to use EnumSet for that (i.e. before AccessFlag enum constants are fully initialized) - an ArrayList is better for this case anyway. Also, conversion from ModuleDescriptor modifier(s) to AccessFlag(s) could be done without first creating a bitmask and then re-constructing a set of AccessFlag(s) from it. Reflective object modifiers can only be translated via bitmask, but various ModuleDescriptor Modifier(s) may have a direct mapping to corresponding AccessFlag(s). For example: https://github.com/plevart/jdk/commit/5a3cd8f6851a0deae7e3e5028bba9a51d7863929 - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v17]
On Sat, 5 Mar 2022 19:54:44 GMT, Joe Darcy wrote: >> This is an early review of changes to better model JVM access flags, that is >> "modifiers" like public, protected, etc. but explicitly at a VM level. >> >> Language level modifiers and JVM level access flags are closely related, but >> distinct. There are concepts that overlap in the two domains (public, >> private, etc.), others that only have a language-level modifier (sealed), >> and still others that only have an access flag (synthetic). >> >> The existing java.lang.reflect.Modifier class is inadequate to model these >> subtleties. For example, the bit positions used by access flags on different >> kinds of elements overlap (such as "volatile" for fields and "bridge" for >> methods. Just having a raw integer does not provide sufficient context to >> decode the corresponding language-level string. Methods like >> Modifier.methodModifiers() were introduced to cope with this situation. >> >> With additional modifiers and flags on the horizon with projects like >> Valhalla, addressing the existent modeling deficiency now ahead of time is >> reasonable before further strain is introduced. >> >> This PR in its current form is meant to give the overall shape of the API. >> It is missing implementations to map from, say, method modifiers to access >> flags, taking into account overlaps in bit positions. >> >> The CSR https://bugs.openjdk.java.net/browse/JDK-8281660 will be filled in >> once the API is further along. > > Joe Darcy has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains 26 additional commits since > the last revision: > > - Respond to review feedback. > - Merge branch 'master' into JDK-8266670 > - Make workding changes suggested in review feedback. > - Merge branch 'master' into JDK-8266670 > - Typo fix; add implSpec to Executable. > - Appease jcheck. > - Fix some bugs found by inspection, docs cleanup. > - Merge branch 'master' into JDK-8266670 > - Initial support for accessFlags methods > - Add mask to access flag functionality. > - ... and 16 more: > https://git.openjdk.java.net/jdk/compare/b40644f8...14980605 Changes requested by plevart (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v17]
> This is an early review of changes to better model JVM access flags, that is > "modifiers" like public, protected, etc. but explicitly at a VM level. > > Language level modifiers and JVM level access flags are closely related, but > distinct. There are concepts that overlap in the two domains (public, > private, etc.), others that only have a language-level modifier (sealed), and > still others that only have an access flag (synthetic). > > The existing java.lang.reflect.Modifier class is inadequate to model these > subtleties. For example, the bit positions used by access flags on different > kinds of elements overlap (such as "volatile" for fields and "bridge" for > methods. Just having a raw integer does not provide sufficient context to > decode the corresponding language-level string. Methods like > Modifier.methodModifiers() were introduced to cope with this situation. > > With additional modifiers and flags on the horizon with projects like > Valhalla, addressing the existent modeling deficiency now ahead of time is > reasonable before further strain is introduced. > > This PR in its current form is meant to give the overall shape of the API. It > is missing implementations to map from, say, method modifiers to access > flags, taking into account overlaps in bit positions. > > The CSR https://bugs.openjdk.java.net/browse/JDK-8281660 will be filled in > once the API is further along. Joe Darcy has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 26 additional commits since the last revision: - Respond to review feedback. - Merge branch 'master' into JDK-8266670 - Make workding changes suggested in review feedback. - Merge branch 'master' into JDK-8266670 - Typo fix; add implSpec to Executable. - Appease jcheck. - Fix some bugs found by inspection, docs cleanup. - Merge branch 'master' into JDK-8266670 - Initial support for accessFlags methods - Add mask to access flag functionality. - ... and 16 more: https://git.openjdk.java.net/jdk/compare/7c47831d...14980605 - Changes: - all: https://git.openjdk.java.net/jdk/pull/7445/files - new: https://git.openjdk.java.net/jdk/pull/7445/files/e63fb13e..14980605 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7445&range=16 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7445&range=15-16 Stats: 11263 lines in 322 files changed: 7362 ins; 2509 del; 1392 mod Patch: https://git.openjdk.java.net/jdk/pull/7445.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7445/head:pull/7445 PR: https://git.openjdk.java.net/jdk/pull/7445
