Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v25]
On Tue, 14 Jun 2022 02:32:54 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 incrementally with one additional > commit since the last revision: > > Respond to review feedback. src/java.base/share/classes/java/lang/reflect/AccessFlag.java line 64: > 62: * arity (varargs)} method. > 63: * > 64: * The access flag constants are ordered by non-decreasing mask This paragraph seems more like an @implNote, describing the ordering of the source. - PR: https://git.openjdk.org/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v25]
On Tue, 14 Jun 2022 02:32:54 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 incrementally with one additional
> commit since the last revision:
>
> Respond to review feedback.
src/java.base/share/classes/java/lang/reflect/AccessFlag.java line 104:
> 102: */
> 103: PROTECTED(Modifier.PROTECTED, true,
> 104: Set.of(Location.FIELD, Location.METHOD,
> Location.INNER_CLASS)),
In both space and startup time, creating separate sets for the same set of
Locations is inefficient.
src/java.base/share/classes/java/lang/reflect/AccessFlag.java line 213:
> 211: * {@value "0x%04x" Modifier#STRICT}.
> 212: */
> 213: STRICT(Modifier.STRICT, true, Set.of(Location.METHOD)),
ACC_STRICT is defined for class files and appears in the Class.getModifiers()
before class file version 46.
Also it is included in Modifer.classModifiers(); Modifer.CLASS_MODIFIERS.
it might be worth a note saying it is class file version specific.
-
PR: https://git.openjdk.org/jdk/pull/7445
Re: RFR: 8288140: Avoid redundant Hashtable.get call in Signal.handle [v2]
On Fri, 10 Jun 2022 11:31:06 GMT, Andrey Turbanov wrote:
>> https://github.com/openjdk/jdk/blob/bc28baeba9360991e9b7575e1fbe178d873ccfc1/src/java.base/share/classes/jdk/internal/misc/Signal.java#L177-L178
>>
>> Instead of separate Hashtable.get/remove calls we can just use value
>> returned by `remove`,
>> It results in cleaner and a bit faster code.
>
> Andrey Turbanov has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8288140: Avoid redundant Hashtable.get call in Signal.handle
> apply dmlloyd's suggestion
Marked as reviewed by rriggs (Reviewer).
src/java.base/share/classes/jdk/internal/misc/Signal.java line 181:
> 179: } else {
> 180: oldHandler = handlers.remove(sig);
> 181: }
A ternary assignment might be an alternative here:
Signal.Handler oldHandler = (newH == 2) ? handlers.put(sig, handler) :
handlers.remove(sig);
-
PR: https://git.openjdk.org/jdk/pull/9100
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v20]
On Wed, 25 May 2022 00:35:24 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 32 additional commits since
> the last revision:
>
> - Target JDK 20 rather than 19.
> - Merge branch 'master' into JDK-8266670
> - Add mask values to constants' javadoc.
> - Implement review feedback from mlchung.
> - Fix type in @throws tag.
> - Merge branch 'master' into JDK-8266670
> - Respond to review feedback.
> - Merge branch 'master' into JDK-8266670
> - Make workding changes suggested in review feedback.
> - Merge branch 'master' into JDK-8266670
> - ... and 22 more:
> https://git.openjdk.java.net/jdk/compare/1b91dbd2...05cf2d8b
Testing a mask for one AccessFlag looks like:
if ((modifiers & AccessFlag.STRICTFP.mask()) != 0) { ... }
A method on AccessFlag to test if its mask value is present in a 32-bit value
would/could make the code easier to read and write.
if (AccessFlag.STRICTFP.isPresent(modifiers)) { ...}
The method name should be chosen to make the test clear and readable.
-
PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: 8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner [v12]
On Tue, 7 Jun 2022 21:19:02 GMT, Brent Christian wrote: > > The commented out printf/println's should be removed before committing. > > Do you mean the pre-existing `println`s in LdapSearchEnumeration.java? Usually, I would mean any that were added for this issue. The changes in indentation (as presented by Github) mislead me. Keep them if they were there before. - PR: https://git.openjdk.java.net/jdk/pull/8311
Re: RFR: 8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner [v12]
On Mon, 6 Jun 2022 21:59:56 GMT, Brent Christian wrote: >> Please review this change to replace the finalizer in >> `AbstractLdapNamingEnumeration` with a Cleaner. >> >> The pieces of state required for cleanup (`LdapCtx homeCtx`, `LdapResult >> res`, and `LdapClient enumClnt`) are moved to a static inner class . From >> there, the change is fairly mechanical. >> >> Details of note: >> 1. Some operations need to change the state values (the update() method is >> probably the most interesting). >> 2. Subclasses need to access `homeCtx`; I added a `homeCtx()` method to read >> `homeCtx` from the superclass's `state`. >> >> The test case is based on a copy of >> `com/sun/jndi/ldap/blits/AddTests/AddNewEntry.java`. A more minimal test >> case might be possible, but this was done for expediency. >> >> The test only confirms that the new Cleaner use does not keep the object >> reachable. It only tests `LdapSearchEnumeration` (not >> `LdapNamingEnumeration` or `LdapBindingEnumeration`, though all are >> subclasses of `AbstractLdapNamingEnumeration`). >> >> Thanks. > > Brent Christian has updated the pull request incrementally with one > additional commit since the last revision: > > Restore EnumCtx to being an inner class, to keep all the state together in > the code trivial comments. The commented out printf/println's should be removed before committing. src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java line 152: > 150: // Ensures that context won't get closed from underneath us > 151: this.enumCtx.homeCtx.incEnumCount(); > 152: this.cleanable = LDAP_CLEANER.register(this, enumCtx); Use `enumCtx.getHomeCts()` above. Use `this.enumCtx` in call to `register`. For consistency. - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8311
Re: CVF: new Core Libraries Group member: Naoto Sato
Vote: Yes On 6/6/22 8:52 PM, Stuart Marks wrote: I hereby nominate Naoto Sato [1] to membership in the Core Libraries Group [2].
Re: Integrated: 8287837: ProblemList java/lang/ref/OOMEInReferenceHandler.java in -Xcomp
On Sun, 5 Jun 2022 14:00:09 GMT, Daniel D. Daugherty wrote: > A trivial fix to ProblemList java/lang/ref/OOMEInReferenceHandler.java in > -Xcomp. Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/9033
Re: RFR: JDK-8287671: Adjust ForceGC to invoke System::gc fewer times for negative case
On Fri, 3 Jun 2022 18:05:52 GMT, Mandy Chung wrote: > This reapplies JDK-8287384 and adjust the number of GCs for negative case, > i.e. the condition is never met that is used to verify a reference is not > GC'ed. Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/9021
Re: RFR: 8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner [v8]
On Fri, 27 May 2022 22:09:22 GMT, Brent Christian wrote: >> Please review this change to replace the finalizer in >> `AbstractLdapNamingEnumeration` with a Cleaner. >> >> The pieces of state required for cleanup (`LdapCtx homeCtx`, `LdapResult >> res`, and `LdapClient enumClnt`) are moved to a static inner class . From >> there, the change is fairly mechanical. >> >> Details of note: >> 1. Some operations need to change the state values (the update() method is >> probably the most interesting). >> 2. Subclasses need to access `homeCtx`; I added a `homeCtx()` method to read >> `homeCtx` from the superclass's `state`. >> >> The test case is based on a copy of >> `com/sun/jndi/ldap/blits/AddTests/AddNewEntry.java`. A more minimal test >> case might be possible, but this was done for expediency. >> >> The test only confirms that the new Cleaner use does not keep the object >> reachable. It only tests `LdapSearchEnumeration` (not >> `LdapNamingEnumeration` or `LdapBindingEnumeration`, though all are >> subclasses of `AbstractLdapNamingEnumeration`). >> >> Thanks. > > Brent Christian has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 14 commits: > > - Merge branch 'master' into remove-finalizers > - Threading-related fixes > - NOW it builds > - Fix the merge fix > - Fix merge > - Merge > - Rename inner class to EnumCtx ; use homeCtx() in > AbstractLdapNamingEnumeration for consistencty ; new instance vars can be > final > - fix whitespace > - Merge branch 'master' into remove-finalizers > - Test changes to test new cleaner code > - ... and 4 more: > https://git.openjdk.java.net/jdk/compare/ed8e8ac2...4af66bff Hans, thank for the detailed example. I had not fully considered the flow of control in the throwing case. - PR: https://git.openjdk.java.net/jdk/pull/8311
Re: RFR: 8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner [v8]
On Fri, 27 May 2022 22:09:22 GMT, Brent Christian wrote:
>> Please review this change to replace the finalizer in
>> `AbstractLdapNamingEnumeration` with a Cleaner.
>>
>> The pieces of state required for cleanup (`LdapCtx homeCtx`, `LdapResult
>> res`, and `LdapClient enumClnt`) are moved to a static inner class . From
>> there, the change is fairly mechanical.
>>
>> Details of note:
>> 1. Some operations need to change the state values (the update() method is
>> probably the most interesting).
>> 2. Subclasses need to access `homeCtx`; I added a `homeCtx()` method to read
>> `homeCtx` from the superclass's `state`.
>>
>> The test case is based on a copy of
>> `com/sun/jndi/ldap/blits/AddTests/AddNewEntry.java`. A more minimal test
>> case might be possible, but this was done for expediency.
>>
>> The test only confirms that the new Cleaner use does not keep the object
>> reachable. It only tests `LdapSearchEnumeration` (not
>> `LdapNamingEnumeration` or `LdapBindingEnumeration`, though all are
>> subclasses of `AbstractLdapNamingEnumeration`).
>>
>> Thanks.
>
> Brent Christian has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 14 commits:
>
> - Merge branch 'master' into remove-finalizers
> - Threading-related fixes
> - NOW it builds
> - Fix the merge fix
> - Fix merge
> - Merge
> - Rename inner class to EnumCtx ; use homeCtx() in
> AbstractLdapNamingEnumeration for consistencty ; new instance vars can be
> final
> - fix whitespace
> - Merge branch 'master' into remove-finalizers
> - Test changes to test new cleaner code
> - ... and 4 more:
> https://git.openjdk.java.net/jdk/compare/ed8e8ac2...4af66bff
src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
line 216:
> 214: } finally {
> 215: // Ensure Cleaner does not run until after this method
> completes
> 216: Reference.reachabilityFence(this);
I don't think there is any benefit to the `try{} finally {fence}`.
The reachabilityFence has no executable code. Its only purpose is to keep the
reference in scope alive.
src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
line 409:
> 407: } finally {
> 408: // Ensure Cleaner does not run until after this method
> completes
> 409: Reference.reachabilityFence(enumCtx);
Ditto, the try/finally is unnecessary.
src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
line 440:
> 438: listArg = ne.listArg;
> 439: } finally {
> 440: // Ensure Cleaner does not run until after this method
> completes
Ditto try/finally is unnecessary.
-
PR: https://git.openjdk.java.net/jdk/pull/8311
Re: RFR: 8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner [v8]
On Fri, 27 May 2022 22:09:22 GMT, Brent Christian wrote:
>> Please review this change to replace the finalizer in
>> `AbstractLdapNamingEnumeration` with a Cleaner.
>>
>> The pieces of state required for cleanup (`LdapCtx homeCtx`, `LdapResult
>> res`, and `LdapClient enumClnt`) are moved to a static inner class . From
>> there, the change is fairly mechanical.
>>
>> Details of note:
>> 1. Some operations need to change the state values (the update() method is
>> probably the most interesting).
>> 2. Subclasses need to access `homeCtx`; I added a `homeCtx()` method to read
>> `homeCtx` from the superclass's `state`.
>>
>> The test case is based on a copy of
>> `com/sun/jndi/ldap/blits/AddTests/AddNewEntry.java`. A more minimal test
>> case might be possible, but this was done for expediency.
>>
>> The test only confirms that the new Cleaner use does not keep the object
>> reachable. It only tests `LdapSearchEnumeration` (not
>> `LdapNamingEnumeration` or `LdapBindingEnumeration`, though all are
>> subclasses of `AbstractLdapNamingEnumeration`).
>>
>> Thanks.
>
> Brent Christian has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 14 commits:
>
> - Merge branch 'master' into remove-finalizers
> - Threading-related fixes
> - NOW it builds
> - Fix the merge fix
> - Fix merge
> - Merge
> - Rename inner class to EnumCtx ; use homeCtx() in
> AbstractLdapNamingEnumeration for consistencty ; new instance vars can be
> final
> - fix whitespace
> - Merge branch 'master' into remove-finalizers
> - Test changes to test new cleaner code
> - ... and 4 more:
> https://git.openjdk.java.net/jdk/compare/ed8e8ac2...4af66bff
src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
line 176:
> 174: LdapResult newRes =
> homeCtx().getSearchReply(enumCtx.enumClnt, enumCtx.res);
> 175: enumCtx.setRes(newRes);
> 176: if (enumCtx.res == null) {
This looks odd, setting the value using synchronized, but reading it without.
-
PR: https://git.openjdk.java.net/jdk/pull/8311
Re: RFR: 8282662: Use List.of() factory method to reduce memory consumption [v9]
On Wed, 1 Jun 2022 07:50:58 GMT, Сергей Цыпанов wrote: >> `List.of()` along with `Set.of()` create unmodifiable `List/Set` but with >> smaller footprint comparing to `Arrays.asList()` / `new HashSet()` when >> called with vararg of size 0, 1, 2. >> >> In general replacement of `Arrays.asList()` with `List.of()` is dubious as >> the latter is null-hostile, however in some cases we are sure that arguments >> are non-null. Into this PR I've included the following cases (in addition to >> those where the argument is proved to be non-null at compile-time): >> - `MethodHandles.longestParameterList()` never returns null >> - parameter types are never null >> - interfaces used for proxy construction and returned from >> `Class.getInterfaces()` are never null >> - exceptions types of method signature are never null > > Сергей Цыпанов has updated the pull request incrementally with one additional > commit since the last revision: > > 8282662: Revert wrong copyright year change Marked as reviewed by rriggs (Reviewer). Thanks for the improvements. - PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8282662: Use List.of() factory method to reduce memory consumption [v8]
On Tue, 31 May 2022 19:30:07 GMT, Сергей Цыпанов wrote: >> `List.of()` along with `Set.of()` create unmodifiable `List/Set` but with >> smaller footprint comparing to `Arrays.asList()` / `new HashSet()` when >> called with vararg of size 0, 1, 2. >> >> In general replacement of `Arrays.asList()` with `List.of()` is dubious as >> the latter is null-hostile, however in some cases we are sure that arguments >> are non-null. Into this PR I've included the following cases (in addition to >> those where the argument is proved to be non-null at compile-time): >> - `MethodHandles.longestParameterList()` never returns null >> - parameter types are never null >> - interfaces used for proxy construction and returned from >> `Class.getInterfaces()` are never null >> - exceptions types of method signature are never null > > Сергей Цыпанов has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains six commits: > > - 8282662: Revert wrong copyright year change > - 8282662: Revert ProxyGenerator > - 8282662: Revert ProxyGenerator > - 8282662: Revert dubious changes in MethodType > - 8282662: Revert dubious changes > - 8282662: Use List/Set.of() factory methods to save memory Merging is preferable, it doesn't disturb the history. Usually an explicit merge is not necessary; Skara will indicate when an automatic merge is needed due to conflicts. If you want to run your own tests then use a merge, not rebase. Thanks - PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8282662: Use List.of() factory method to reduce memory consumption [v8]
On Tue, 31 May 2022 19:30:07 GMT, Сергей Цыпанов wrote: >> `List.of()` along with `Set.of()` create unmodifiable `List/Set` but with >> smaller footprint comparing to `Arrays.asList()` / `new HashSet()` when >> called with vararg of size 0, 1, 2. >> >> In general replacement of `Arrays.asList()` with `List.of()` is dubious as >> the latter is null-hostile, however in some cases we are sure that arguments >> are non-null. Into this PR I've included the following cases (in addition to >> those where the argument is proved to be non-null at compile-time): >> - `MethodHandles.longestParameterList()` never returns null >> - parameter types are never null >> - interfaces used for proxy construction and returned from >> `Class.getInterfaces()` are never null >> - exceptions types of method signature are never null > > Сергей Цыпанов has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains six commits: > > - 8282662: Revert wrong copyright year change > - 8282662: Revert ProxyGenerator > - 8282662: Revert ProxyGenerator > - 8282662: Revert dubious changes in MethodType > - 8282662: Revert dubious changes > - 8282662: Use List/Set.of() factory methods to save memory Why the force push? They are discouraged, making it harder to review. - PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8287384: Speed up jdk.test.lib.util.ForceGC
On Tue, 31 May 2022 17:41:08 GMT, Mandy Chung wrote: > Hmm... one benefit of Cleaner is the ease of use to avoid the need of > managing the reference queue. If the performance of the Cleaner API is a > concern, perhaps we should look into reducing its overhead? The code using a Cleaner here creates a CountDownLatch to wait on; that along with the Cleaner seems very similar to the complexity of the suggested alternative directly using a ReferenceQueue. But I'm fine with it either way Each Cleaner creates a new OS thread, used for a single instance of ForceGC. The caching of the Cleaner in a Static mostly reduces the per ForceGC overhead. When Loom is a feature, a Virtual Thread would be sufficient. - PR: https://git.openjdk.java.net/jdk/pull/8907
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v20]
On Fri, 27 May 2022 20:21:12 GMT, Mandy Chung wrote: > With the `AccessFlag` API, what is the role of the `Modifier` API going > forward? [Value Objects JEP](https://openjdk.java.net/jeps/8277163) defines > the new `identity` and `value` modifiers. [PR > #698](https://github.com/openjdk/valhalla/pull/698) proposes to add > `Modifier.IDENTITY` and `Modifier.VALUE` constants as the `identity` and > `value` modifiers are encoded in a class file using the `ACC_IDENTITY` and > `ACC_VALUE` flags. However, with the new improved `AccessFlag` API, the new > flags will be defined in the `AccessFlag` API. I think we should avoid adding > the new flags in `Modifier` and leave it for the existing usage. Use > `AccessFlag` for new features. Looking over the existing uses of Modifier, as relates to Class.getModifiers(), there are simple uses testing for an individual access flag and others that expect a full set of modifier bits that apply to the class. There are a few places that need all the bits to compose a new set of modifier bits and a few others that test for combinations of bits. `Class.getModifiers()` is intrinsified, testing against various bit patterns using the static methods of `Modifier` is very performant. The `Modifer.toString()` method cannot distinguish the flags overloaded by the Class vs Member cases. Modifier.toString() is used by both Class and Member `toString` methods. Methods to convert a Set to a mask and to produce the 'toString' of the set would be useful. To replace all uses of Modifier the existing use cases will need some attention. - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: 8287544: Replace uses of StringBuffer with StringBuilder in java.naming
On Sun, 29 May 2022 21:57:46 GMT, Andrey Turbanov wrote: > StringBuffer is a legacy synchronized class. StringBuilder is a direct > replacement to StringBuffer which generally have better performance. > There are some code that still uses StringBuffer in java.naming which could > be migrated to `StringBuilder`. Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8942
Re: RFR: 8282662: Use List.of() factory method to reduce memory consumption [v7]
On Tue, 31 May 2022 07:40:56 GMT, Сергей Цыпанов wrote: >> `List.of()` along with `Set.of()` create unmodifiable `List/Set` but with >> smaller footprint comparing to `Arrays.asList()` / `new HashSet()` when >> called with vararg of size 0, 1, 2. >> >> In general replacement of `Arrays.asList()` with `List.of()` is dubious as >> the latter is null-hostile, however in some cases we are sure that arguments >> are non-null. Into this PR I've included the following cases (in addition to >> those where the argument is proved to be non-null at compile-time): >> - `MethodHandles.longestParameterList()` never returns null >> - parameter types are never null >> - interfaces used for proxy construction and returned from >> `Class.getInterfaces()` are never null >> - exceptions types of method signature are never null > > Сергей Цыпанов 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 six additional > commits since the last revision: > > - 8282662: Revert wrong copyright year change > - 8282662: Revert ProxyGenerator > - 8282662: Revert ProxyGenerator > - 8282662: Revert dubious changes in MethodType > - 8282662: Revert dubious changes > - 8282662: Use List/Set.of() factory methods to save memory Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8287497: Use String.contains() instead of String.indexOf() in java.naming
On Sun, 29 May 2022 14:00:20 GMT, Andrey Turbanov wrote: > `String.contains` was introduced in Java 5. > Some code in java.naming still uses old approach with `String.indexOf` to > check if String contains specified substring. > I propose to migrate such usages. Makes code shorter and easier to read. Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8938
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v20]
On Wed, 25 May 2022 00:35:24 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 32 additional commits since > the last revision: > > - Target JDK 20 rather than 19. > - Merge branch 'master' into JDK-8266670 > - Add mask values to constants' javadoc. > - Implement review feedback from mlchung. > - Fix type in @throws tag. > - Merge branch 'master' into JDK-8266670 > - Respond to review feedback. > - Merge branch 'master' into JDK-8266670 > - Make workding changes suggested in review feedback. > - Merge branch 'master' into JDK-8266670 > - ... and 22 more: > https://git.openjdk.java.net/jdk/compare/f2d5c3d7...05cf2d8b As for retaining SUPER, both SUPER and IDENTITY have location class and according to the BasicAccessFlagTest, the bit patterns should be disjoint - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v20]
On Wed, 25 May 2022 00:35:24 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 32 additional commits since > the last revision: > > - Target JDK 20 rather than 19. > - Merge branch 'master' into JDK-8266670 > - Add mask values to constants' javadoc. > - Implement review feedback from mlchung. > - Fix type in @throws tag. > - Merge branch 'master' into JDK-8266670 > - Respond to review feedback. > - Merge branch 'master' into JDK-8266670 > - Make workding changes suggested in review feedback. > - Merge branch 'master' into JDK-8266670 > - ... and 22 more: > https://git.openjdk.java.net/jdk/compare/fb4068e0...05cf2d8b Please comment here: https://github.com/openjdk/valhalla/pull/698 The AccessFlag API identifies Modifier as *the* primary definition of source elements. - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v20]
On Wed, 25 May 2022 00:35:24 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 32 additional commits since > the last revision: > > - Target JDK 20 rather than 19. > - Merge branch 'master' into JDK-8266670 > - Add mask values to constants' javadoc. > - Implement review feedback from mlchung. > - Fix type in @throws tag. > - Merge branch 'master' into JDK-8266670 > - Respond to review feedback. > - Merge branch 'master' into JDK-8266670 > - Make workding changes suggested in review feedback. > - Merge branch 'master' into JDK-8266670 > - ... and 22 more: > https://git.openjdk.java.net/jdk/compare/75552cfd...05cf2d8b To reinforce the argument not to propagate ACC_SUPER to the new API. It is NOT currently defined in j.l.reflect.Modifier, so there are no existing dependencies on a symbol. Class.getModifiers() *never* returns ACC_SUPER; the VM explicitly removes it (since 2007, for all class file versions). The note in the JVMS spec indicates it is retained only for source compatibility with earlier releases. The JVMS spec considers the flag to always be set. and It is always unset in the modifiers returned from getModifiers(). Defining the value where it has not been defined before has no useful purpose. - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: 8287003: InputStreamReader::read() can return zero despite writing a char in the buffer
On Wed, 25 May 2022 23:08:38 GMT, Brian Burkhalter wrote: > If only a leftover `char` remains in the stream, do not add `-1` to the > return value in `lockedRead()`. Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8895
Re: RFR: 8287384: Speed up jdk.test.lib.util.ForceGC
On Thu, 26 May 2022 18:50:07 GMT, Xue-Lei Andrew Fan wrote: > Hi, > > May I have this test update reviewed? > > The ForceGC could be enhanced by using smaller wait/sleep time, and shared > cleaner. > > Thanks, > Xuelei ok, the updates look fine. - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8907
Re: RFR: 8287384: Speed up jdk.test.lib.util.ForceGC
On Thu, 26 May 2022 18:50:07 GMT, Xue-Lei Andrew Fan wrote:
> Hi,
>
> May I have this test update reviewed?
>
> The ForceGC could be enhanced by using smaller wait/sleep time, and shared
> cleaner.
>
> Thanks,
> Xuelei
Even using a Cleaner is a more overhead than necessary.
I would have skipped the overhead of a cleaner and Atomic classes with
something more self contained as a static method:
/**
* The garbage collector is invoked to find unreferenced objects.
* A new object is created and then freed. The method returns
* when the object has been reclaimed or the GC did not complete in 10
seconds.
*
* @return true if GC was complete, false if did not complete after 10
Seconds
*/
public static boolean waitForGC() {
ReferenceQueue queue = new ReferenceQueue<>();
Object obj = new Object();
PhantomReference ref = new PhantomReference<>(obj, queue);
obj = null;
Reference.reachabilityFence(obj);
Reference.reachabilityFence(ref);
System.gc();
for (int retries = 100; retries > 0; retries--) {
try {
var r = queue.remove(100L);
if (r != null) {
return true;
}
} catch (InterruptedException ie) {
// ignore, the loop will try again
}
System.gc();
}
return false;
}
YMMV
-
PR: https://git.openjdk.java.net/jdk/pull/8907
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v20]
On Wed, 25 May 2022 00:35:24 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 32 additional commits since > the last revision: > > - Target JDK 20 rather than 19. > - Merge branch 'master' into JDK-8266670 > - Add mask values to constants' javadoc. > - Implement review feedback from mlchung. > - Fix type in @throws tag. > - Merge branch 'master' into JDK-8266670 > - Respond to review feedback. > - Merge branch 'master' into JDK-8266670 > - Make workding changes suggested in review feedback. > - Merge branch 'master' into JDK-8266670 > - ... and 22 more: > https://git.openjdk.java.net/jdk/compare/65e59633...05cf2d8b AccessFlags.SUPER can/should be removed; it is unused and will be redefined in the [Value Objects JEP](https://openjdk.java.net/jeps/8277163). It will be a cleaner transition if there is no opportunity to create a dependency on the obsolete flag. - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v2]
On Fri, 20 May 2022 22:18:42 GMT, liach wrote:
>> Simplify opcode handling, use `final` in `PrimitiveTypeInfo`, and replace
>> the hash map with a simple lookup, similar to what's done in
>> [JDK-8284880](https://bugs.openjdk.java.net/browse/JDK-8284880) (#8242)
>
> liach has updated the pull request incrementally with one additional commit
> since the last revision:
>
> Convert PrimitiveTypeInfo to an enum
Looks good.
Did you consider switch (class) {...} in PrimitiveTypeInfo.get?
The `if` cascade may be quicker given the expected distribution of lookups.
-
Marked as reviewed by rriggs (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/8801
Re: RFR: 8287181: Avoid redundant HashMap.containsKey calls in InternalLocaleBuilder.setExtension
On Sat, 30 Apr 2022 17:10:55 GMT, Andrey Turbanov wrote: > No need to separately perform `HashMap.containsKey` before `HashMap.remove` > call. If key is present - it will be removed anyway. If it's not present, > nothing will be deleted. Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8488
Re: RFR: JDK-6725221 Standardize obtaining boolean properties with defaults [v2]
On Mon, 23 May 2022 18:58:34 GMT, Mark Powers wrote: >> JDK-6725221 Standardize obtaining boolean properties with defaults > > Mark Powers has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains six commits: > > - Alan and Jamil comments > - Merge > - reverse true.equals and false.equals changes > - Merge > - Merge > - first iteration LGTM - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8559
Re: RFR: JDK-6725221 Standardize obtaining boolean properties with defaults
On Tue, 10 May 2022 19:24:24 GMT, Mark Powers wrote:
>> src/java.base/share/classes/java/lang/reflect/AccessibleObject.java line 777:
>>
>>> 775: if (!printStackPropertiesSet && VM.initLevel() >= 1) {
>>> 776: printStackWhenAccessFails = GetBooleanAction.
>>> 777:
>>> privilegedGetProperty("sun.reflect.debugModuleAccessChecks");
>>
>> Running with `-Dsun.reflect.debugModuleAccessChecks` should set
>> printStackWhenAccessFails to true, not false.
>
> You are right. The old way maps the null string to true, and the new way maps
> it to false. I did not notice that. At this point, I see no value in making
> the "true".equals and "false".equals changes. Too much can break. I'll
> reverse these changes.
This change still needs to be reversed.
-
PR: https://git.openjdk.java.net/jdk/pull/8559
Re: RFR: 8284638: store skip buffers in InputStream Object [v12]
On Wed, 20 Apr 2022 16:52:31 GMT, XenoAmess wrote:
>> @jmehrens what about this then?
>> I think it safe now(actually this mechanism is learned from Reader)
>
> XenoAmess has updated the pull request incrementally with one additional
> commit since the last revision:
>
> add documents
src/java.base/share/classes/java/io/InputStream.java line 72:
> 70: *
> 71: * @param size minimum length that the skip byte array must have.
> 72: * notice that this param input MUST be equal or less
> than {@link #MAX_SKIP_BUFFER_SIZE MAX_SKIP_BUFFER_SIZE}.
The "MUST be equal" reads like something will go wrong if that condition isn't
satisfied.
This method does not care about the size, except in potentially resizing if the
requested size is larger.
The "notice..." statement is making a statement about code in the caller, and
so doesn't belong here.
src/java.base/share/classes/java/io/InputStream.java line 75:
> 73: * @return the byte array.
> 74: */
> 75: private byte[] skipBufferReference(int size) {
The method name would match the return value better if named `skipBuffer(size)`.
src/java.base/share/classes/java/io/InputStream.java line 78:
> 76: SoftReference ref = this.skipBufferReference;
> 77: byte[] buffer;
> 78: if (ref == null || (buffer = ref.get()) == null || buffer.length
> < size) {
A comment here or in the method javadoc to the effect that a new buffer is
allocated unless the existing buffer is large enough might head off doubt that
buffer is always non-null at the return. As would inverting the if:
if (ref != null && (buffer = ref.get()) != null && buffer.length >= size) {
return buffer;
}
// allocate new or larger buffer
buffer = new byte[size];
this.skipBufferReference = new SoftReference<>(buffer);
return buffer;
-
PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo
On Fri, 20 May 2022 04:55:37 GMT, liach wrote:
> Simplify opcode handling, use `final` in `PrimitiveTypeInfo`, and replace the
> hash map with a simple lookup, similar to what's done in
> [JDK-8284880](https://bugs.openjdk.java.net/browse/JDK-8284880) (#8242)
src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 932:
> 930: */
> 931: private static final class PrimitiveTypeInfo {
> 932: private static final PrimitiveTypeInfo BYTE = new
> PrimitiveTypeInfo(byte.class, 0);
Can this be `private enum PrimitiveTypeInfo...` or perhaps a record class?
-
PR: https://git.openjdk.java.net/jdk/pull/8801
Re: RFR: 8285401: Proxy class initializer should use 3-arg `Class.forName` to avoid unnecessary class initialization [v3]
On Fri, 20 May 2022 18:22:47 GMT, liach wrote: >> Simplify calls `Class.forName(String, boolean, ClassLoader)` instead of >> `Class.forName(String)`. `make test >> TEST="jtreg:test/jdk/java/lang/reflect/Proxy"` passes, with the new >> `LazyInitializationTest` failing the eager initialization check on the >> baseline and passing with this patch. >> >> On a side note, this might reduce the number of methods that can be encoded >> in a proxy due to code attribute size restrictions; we probably would >> address that in another issue, as we never mandated a count of methods that >> the proxy must be able to implement. >> >> Mandy, would you mind review this? > > liach has updated the pull request incrementally with one additional commit > since the last revision: > > Move the try catch block as it doesn't throw checked exceptions Looks fine. - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8800
Re: RFR: 8279185: Support for IsoFields in JapaneseDate/MinguoDate/ThaiBuddhistDate [v10]
On Tue, 17 May 2022 23:40:04 GMT, Naoto Sato wrote: >> Supporting `IsoFields` temporal fields in chronologies that are similar to >> ISO chronology. Corresponding CSR has also been drafted. > > Naoto Sato 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 13 additional commits since > the last revision: > > - Merge branch 'master' into JDK-8279185 > - Addressing review comments > - Revert to use the default method > - Removed unnecessary package names > - Modified class desc of `IsoFields` > - abstract class -> top level interface > - interface -> abstract class > - Removed the method > - Changed to use a type to determine ISO based or not > - Renamed the new method > - ... and 3 more: > https://git.openjdk.java.net/jdk/compare/2761b08b...3f69909f Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7683
Re: RFR: 8287053: Avoid redundant HashMap.containsKey calls in ZoneInfoFile.getZoneInfo0
On Sat, 30 Apr 2022 17:00:03 GMT, Andrey Turbanov wrote: > Instead of pair `HashMap.containsKey`/`HashMap.get` method calls, we can use > single call `HashMap.getOrDefault`. > It's faster and clearer. Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8487
Re: RFR: 8280035: Use Class.isInstance instead of Class.isAssignableFrom where applicable [v2]
On Thu, 31 Mar 2022 08:03:23 GMT, Andrey Turbanov wrote:
>> Method `Class.isAssignableFrom` is often used in form of:
>>
>> if (clazz.isAssignableFrom(obj.getClass())) {
>> Such condition could be simplified to more shorter and performarnt code
>>
>> if (clazz.isInstance(obj)) {
>>
>> Replacement is equivalent if it's known that `obj != null`.
>> In JDK codebase there are many such places.
>>
>> I tried to measure performance via JMH
>>
>> Class integerClass = Integer.class;
>> Class numberClass = Number.class;
>>
>> Object integerObject = 45666;
>> Object doubleObject = 8777d;
>>
>> @Benchmark
>> public boolean integerInteger_isInstance() {
>> return integerClass.isInstance(integerObject);
>> }
>>
>> @Benchmark
>> public boolean integerInteger_isAssignableFrom() {
>> return integerClass.isAssignableFrom(integerObject.getClass());
>> }
>>
>> @Benchmark
>> public boolean integerDouble_isInstance() {
>> return integerClass.isInstance(doubleObject);
>> }
>>
>> @Benchmark
>> public boolean integerDouble_isAssignableFrom() {
>> return integerClass.isAssignableFrom(doubleObject.getClass());
>> }
>>
>> @Benchmark
>> public boolean numberDouble_isInstance() {
>> return numberClass.isInstance(doubleObject);
>> }
>>
>> @Benchmark
>> public boolean numberDouble_isAssignableFrom() {
>> return numberClass.isAssignableFrom(doubleObject.getClass());
>> }
>>
>> @Benchmark
>> public boolean numberInteger_isInstance() {
>> return numberClass.isInstance(integerObject);
>> }
>>
>> @Benchmark
>> public boolean numberInteger_isAssignableFrom() {
>> return numberClass.isAssignableFrom(integerObject.getClass());
>> }
>>
>> @Benchmark
>> public void numberIntegerDouble_isInstance(Blackhole bh) {
>> bh.consume(numberClass.isInstance(integerObject));
>> bh.consume(numberClass.isInstance(doubleObject));
>> }
>>
>> @Benchmark
>> public void integerIntegerDouble_isAssignableFrom(Blackhole bh) {
>> bh.consume(integerClass.isAssignableFrom(integerObject.getClass()));
>> bh.consume(integerClass.isAssignableFrom(doubleObject.getClass()));
>> }
>>
>> `isInstance` is a bit faster than `isAssignableFrom`
>>
>> Benchmark Mode Cnt Score Error Units
>> integerDouble_isAssignableFrom avgt5 1,173 ± 0,026 ns/op
>> integerDouble_isInstance avgt5 0,939 ± 0,038 ns/op
>> integerIntegerDouble_isAssignableFrom avgt5 2,106 ± 0,068 ns/op
>> numberIntegerDouble_isInstance avgt5 1,516 ± 0,046 ns/op
>> integerInteger_isAssignableFromavgt5 1,175 ± 0,029 ns/op
>> integerInteger_isInstance avgt5 0,886 ± 0,017 ns/op
>> numberDouble_isAssignableFrom avgt5 1,172 ± 0,007 ns/op
>> numberDouble_isInstanceavgt5 0,891 ± 0,030 ns/op
>> numberInteger_isAssignableFrom avgt5 1,169 ± 0,014 ns/op
>> numberInteger_isInstance avgt5 0,887 ± 0,016 ns/op
>
> Andrey Turbanov has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8280035: Use Class.isInstance instead of Class.isAssignableFrom where
> applicable
> apply suggestion to avoid second Method.invoke call
Marked as reviewed by rriggs (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/7061
Re: RFR: 8285517: System.getenv() returns unexpected value if environment variable has non ASCII character [v9]
On Tue, 17 May 2022 01:19:30 GMT, Ichiroh Takiguchi
wrote:
>> On JDK19 with Linux ja_JP.eucjp locale,
>> System.getenv() returns unexpected value if environment variable has
>> Japanese EUC characters.
>> It seems this issue happens because of JEP 400.
>> Arguments for ProcessBuilder have same kind of issue.
>
> Ichiroh Takiguchi has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8285517: System.getenv() returns unexpected value if environment variable
> has non ASCII character
Looks fine.
(I don't personally like how {@return looks in the source; but its done now.
Usually it is preferred to stick to the style in the file and not mix stylistic
changes with functional changes).
Thanks for the updates.
-
Marked as reviewed by rriggs (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/8378
Re: RFR: 8267038: Update IANA Language Subtag Registry to Version 2022-03-02
On Wed, 18 May 2022 16:28:02 GMT, Naoto Sato wrote: > This is to incorporate the latest language subtag registry definition > (version 2022-03-02) into JDK19. Looks fine. - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8776
Re: RFR: 8286810: Use public [Double|Float].PRECISION fields in jdk.internal.math.[Double|Float]Consts [v2]
On Mon, 16 May 2022 15:54:25 GMT, Raffaello Giulietti wrote: >> Please review these simple changes in jdk.internal.math.[Double|Float]Consts > > Raffaello Giulietti has updated the pull request incrementally with one > additional commit since the last revision: > > 8286810: Use public [Double|Float].PRECISION fields in > jdk.internal.math.[Double|Float]Consts LGTM, thanks - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8729
Re: RFR: 8286810: Use public [Double|Float].PRECISION fields in jdk.internal.math.[Double|Float]Consts
On Mon, 16 May 2022 14:48:43 GMT, Raffaello Giulietti wrote: > Please review these simple changes in jdk.internal.math.[Double|Float]Consts src/java.base/share/classes/jdk/internal/math/DoubleConsts.java line 28: > 26: package jdk.internal.math; > 27: > 28: import static java.lang.Double.*; I'd rather see explicit static imports, especially if there is any ambiguity as to where they come from. When using ordinary text search in a file, it can quickly identify a static import as the source of the symbol. As in this case where the same symbol has different values for Float vs Double. YMMV - PR: https://git.openjdk.java.net/jdk/pull/8729
Re: RFR: 8286399: Address possibly lossy conversions in JDK Build Tools
On Fri, 13 May 2022 17:05:43 GMT, Naoto Sato wrote: > Applied required casts for the upcoming warning. Verified by cherry-picking > Adam's patch. Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8706
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v19]
On Tue, 3 May 2022 21:35:48 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 incrementally with one additional > commit since the last revision: > > Add mask values to constants' javadoc. This seems to have reached a stable plateau. The CSR should have another reviewer. - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7445
Integrated: 8286393: Address possibly lossy conversions in java.rmi
On Thu, 12 May 2022 16:47:43 GMT, Roger Riggs wrote: > Updates to modules java.rmi and java.smartcardio to remove warnings about > lossy-conversions introduced by PR#8599. > > Explicit casts are inserted where implicit casts occur. > > 8286393: Address possibly lossy conversions in java.rmi > 8286388: Address possibly lossy conversions in java.smartcardio This pull request has now been integrated. Changeset: 237f2801 Author:Roger Riggs URL: https://git.openjdk.java.net/jdk/commit/237f28014ab9d27d2cdfe3fdc4a5b0a0680f8e95 Stats: 6 lines in 2 files changed: 0 ins; 0 del; 6 mod 8286393: Address possibly lossy conversions in java.rmi 8286388: Address possibly lossy conversions in java.smartcardio Reviewed-by: lancea, dfuchs, smarks - PR: https://git.openjdk.java.net/jdk/pull/8683
Re: RFR: 8286378: Address possibly lossy conversions in java.base [v3]
On Fri, 13 May 2022 05:54:15 GMT, Alan Bateman wrote: >> src/java.base/linux/classes/sun/nio/ch/EPollSelectorImpl.java line 128: >> >>> 126: // timed poll interrupted so need to adjust timeout >>> 127: long adjust = System.nanoTime() - startTime; >>> 128: to =- (int) TimeUnit.NANOSECONDS.toMillis(adjust); >> >> This will now always assign a negative number to `to`. >> >> >> >> `=-` is not a compound assignment, it’s negation followed by a normal >> assignment. > > Well spotted, I don't think that change was intentionally. Ouch; Will fix: I took Alan's earlier comment literally: "This one can also be changed to: to =- (int) TimeUnit.NANOSECONDS.toMillis(adjust);" - PR: https://git.openjdk.java.net/jdk/pull/8642
Re: RFR: 8286200: SequenceInputStream::read(b, off, 0) returns -1 at EOF [v3]
On Thu, 12 May 2022 16:01:50 GMT, Brian Burkhalter wrote: >> Modify the specification of `SequenceInputStream.read(byte[],int,int)` to >> indicate that `-1` is returned at the EOF of the last stream even if `len` >> is zero. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8286200: Change @throws NPE clause of read(byte[],int,int) to better match > specification verbiage In the throws clauses, I think I would have put the additional conditional at the end of the sentence since the existing throws text corresponds to the exception. But the logic is correct as is. - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8664
RFR: 8286393: Address possibly lossy conversions in java.rmi
Updates to modules java.rmi and java.smartcardio to remove warnings about lossy-conversions introduced by PR#8599. Explicit casts are inserted where implicit casts occur. 8286393: Address possibly lossy conversions in java.rmi 8286388: Address possibly lossy conversions in java.smartcardio - Commit messages: - 8286393: Address possibly lossy conversions in java.rmi Changes: https://git.openjdk.java.net/jdk/pull/8683/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8683=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286393 Stats: 6 lines in 2 files changed: 0 ins; 0 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/8683.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8683/head:pull/8683 PR: https://git.openjdk.java.net/jdk/pull/8683
Integrated: 8286378: Address possibly lossy conversions in java.base
On Tue, 10 May 2022 21:32:10 GMT, Roger Riggs wrote: > PR#8599 8244681: proposes to add compiler warnings for possible lossy > conversions > From the CSR: > > "If the type of the right-hand operand of a compound assignment is not > assignment compatible with the type of the variable, a cast is implied and > possible lossy conversion may silently occur. While similar situations are > resolved as compilation errors for primitive assignments, there are no > similar rules defined for compound assignments." > > This PR anticipates the warnings and adds explicit casts to replace the > implicit casts. > In most cases, the cast matches the type of the right-hand side to type of > the compound assignment. > Since these casts are truncating the value, there might be a different > refactoring that avoids the cast > but a direct replacement was chosen here. > > (Advise and suggestions will inform similar updates to other OpenJDK modules). This pull request has now been integrated. Changeset: 17c52789 Author:Roger Riggs URL: https://git.openjdk.java.net/jdk/commit/17c52789b79a4ccd65308f90c4e02c1732b206be Stats: 71 lines in 32 files changed: 0 ins; 3 del; 68 mod 8286378: Address possibly lossy conversions in java.base Reviewed-by: naoto, xuelei, bpb, alanb - PR: https://git.openjdk.java.net/jdk/pull/8642
Re: RFR: 8285517: System.getenv() returns unexpected value if environment variable has non ASCII character [v7]
On Mon, 9 May 2022 12:30:19 GMT, Ichiroh Takiguchi wrote: >> On JDK19 with Linux ja_JP.eucjp locale, >> System.getenv() returns unexpected value if environment variable has >> Japanese EUC characters. >> It seems this issue happens because of JEP 400. >> Arguments for ProcessBuilder have same kind of issue. > > Ichiroh Takiguchi has updated the pull request with a new target base due to > a merge or a rebase. The pull request now contains seven commits: > > - Merge branch 'master' into 8285517 > - 8285517: System.getenv() returns unexpected value if environment variable > has non ASCII character > - 8285517: System.getenv() returns unexpected value if environment variable > has non ASCII character > - 8285517: System.getenv() returns unexpected value if environment variable > has non ASCII character > - 8285517: System.getenv() returns unexpected value if environment variable > has non ASCII character > - 8285517: System.getenv() returns unexpected value if environment variable > has non ASCII character > - 8285517: System.getenv() returns unexpected value if environment variable > has non ASCII character src/java.base/unix/classes/java/lang/ProcessEnvironment.java line 68: > 66: private static final Map theUnmodifiableEnvironment; > 67: static final int MIN_NAME_LENGTH = 0; > 68: private static final Charset jnuCharset = StaticProperty.jnuCharset(); Since the Charset is cached in StaticProperty, I don't think its worthwhile to make a local copy of the same ref. Just refer to `StaticProperty.jnuCharset()` where it is needed. (Here and in ProcessImpl) - PR: https://git.openjdk.java.net/jdk/pull/8378
Re: RFR: 8286378: Address possibly lossy conversions in java.base [v3]
> PR#8599 8244681: proposes to add compiler warnings for possible lossy > conversions > From the CSR: > > "If the type of the right-hand operand of a compound assignment is not > assignment compatible with the type of the variable, a cast is implied and > possible lossy conversion may silently occur. While similar situations are > resolved as compilation errors for primitive assignments, there are no > similar rules defined for compound assignments." > > This PR anticipates the warnings and adds explicit casts to replace the > implicit casts. > In most cases, the cast matches the type of the right-hand side to type of > the compound assignment. > Since these casts are truncating the value, there might be a different > refactoring that avoids the cast > but a direct replacement was chosen here. > > (Advise and suggestions will inform similar updates to other OpenJDK modules). Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Updated copyrights Fixed cast style to add a space after cast, (where consistent with file style) Improved code per review comments in PollSelectors. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8642/files - new: https://git.openjdk.java.net/jdk/pull/8642/files/7ff806a5..a6679ce7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8642=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8642=01-02 Stats: 41 lines in 24 files changed: 0 ins; 0 del; 41 mod Patch: https://git.openjdk.java.net/jdk/pull/8642.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8642/head:pull/8642 PR: https://git.openjdk.java.net/jdk/pull/8642
Re: RFR: 8286287: Reading file as UTF-16 causes Error which "shouldn't happen" [v2]
On Wed, 11 May 2022 15:52:59 GMT, Naoto Sato wrote: >> `String.decodeWithDecoder()` method requires the `CharsetDecoder` parameter >> replaces on malformed/unmappable characters with replacements. However, >> there was a code path that lacked to set the `CodingErrorAction.REPLACE` on >> the decoder. Unrelated, one typo in a test was also fixed. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Reverted the typo fix Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8640
Re: RFR: 8244681: Add a warning for possibly lossy conversion in compound assignments [v3]
On Wed, 11 May 2022 13:27:38 GMT, Adam Sotona wrote: >> That's good to know. I think the tricky part is mostly about keeping track >> of all these disabled warnings, so they are not kept around longer than >> necessary. And that needs coordination with all the subtasks of the umbrella >> issue. > > Thanks for quick reaction. > I'll keep my eyes on this race of patches and update this pull request > accordingly or create a new PR. I put out a PR for java.base, but thought I'd wait until the javac fixe were pushed before integrating and would re-enable the warnings at the same time. - PR: https://git.openjdk.java.net/jdk/pull/8599
Re: RFR: 8286378: Address possibly lossy conversions in java.base [v2]
> PR#8599 8244681: proposes to add compiler warnings for possible lossy > conversions > From the CSR: > > "If the type of the right-hand operand of a compound assignment is not > assignment compatible with the type of the variable, a cast is implied and > possible lossy conversion may silently occur. While similar situations are > resolved as compilation errors for primitive assignments, there are no > similar rules defined for compound assignments." > > This PR anticipates the warnings and adds explicit casts to replace the > implicit casts. > In most cases, the cast matches the type of the right-hand side to type of > the compound assignment. > Since these casts are truncating the value, there might be a different > refactoring that avoids the cast > but a direct replacement was chosen here. > > (Advise and suggestions will inform similar updates to other OpenJDK modules). Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: remove stray file - Changes: - all: https://git.openjdk.java.net/jdk/pull/8642/files - new: https://git.openjdk.java.net/jdk/pull/8642/files/e72ce85c..7ff806a5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8642=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8642=00-01 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8642.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8642/head:pull/8642 PR: https://git.openjdk.java.net/jdk/pull/8642
RFR: 8286378: Address possibly lossy conversions in java.base
PR#8599 8244681: proposes to add compiler warnings for possible lossy conversions >From the CSR: "If the type of the right-hand operand of a compound assignment is not assignment compatible with the type of the variable, a cast is implied and possible lossy conversion may silently occur. While similar situations are resolved as compilation errors for primitive assignments, there are no similar rules defined for compound assignments." This PR anticipates the warnings and adds explicit casts to replace the implicit casts. In most cases, the cast matches the type of the right-hand side to type of the compound assignment. Since these casts are truncating the value, there might be a different refactoring that avoids the cast but a direct replacement was chosen here. (Advise and suggestions will inform similar updates to other OpenJDK modules). - Commit messages: - 8286378: Address possibly lossy conversions in java.base Changes: https://git.openjdk.java.net/jdk/pull/8642/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8642=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286378 Stats: 57 lines in 33 files changed: 1 ins; 3 del; 53 mod Patch: https://git.openjdk.java.net/jdk/pull/8642.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8642/head:pull/8642 PR: https://git.openjdk.java.net/jdk/pull/8642
Re: RFR: 8285517: System.getenv() returns unexpected value if environment variable has non ASCII character [v5]
On Fri, 6 May 2022 20:03:35 GMT, Naoto Sato wrote: >> Ichiroh Takiguchi has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8285517: System.getenv() returns unexpected value if environment variable >> has non ASCII character > > src/java.base/share/classes/jdk/internal/util/StaticProperty.java line 77: > >> 75: SUN_JNU_ENCODING = getProperty(props, "sun.jnu.encoding"); >> 76: jnuCharset = Charset.forName(SUN_JNU_ENCODING, >> Charset.defaultCharset()); >> 77: } > > I am not sure it is OK to initialize `Charset` here, as `sun_jnu_encoding` is > initialized in `System.initPhase1()` and pulling `Charset` there may cause > some init order change. I'd only cache the encoding string here. Note the initialization of `sun.jnu.encoding` in System.java:2142'ish. This happens before StaticProperty is initialized; at line 2147. If the `sun.jnu.encoding` is not supported (this is before Providers are enabled) then it is forced to `UTF-8`. So it is the case that the encoding is supported by that point. Note that `Charset.isSupported(...)` does the full lookup in its implementation. If it is not supported, the system still comes up using UTF-8 and a warning is printed at line 2282. Comparing the class initialization order may be a useful thing to cross check. - PR: https://git.openjdk.java.net/jdk/pull/8378
Re: RFR: 8286191: misc tests fail due to JDK-8285987 [v3]
On Fri, 6 May 2022 06:41:31 GMT, Matthias Baesken wrote: >> The isMusl method had to be handled in >> test/lib-test/jdk/test/lib/TestMutuallyExclusivePlatformPredicates.java . >> Additionally, the vm.musl predicate seem not to be available in the >> langtools tests. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > restore year in ExternalEditorTest, remove test exclusion Looks good. Thanks for resolving both ProblemLists. Hopefully, the real problem and solution on Musl can be found separately. - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8556
Re: RFR: 8285517: System.getenv() returns unexpected value if environment variable has non ASCII character [v2]
On Wed, 4 May 2022 03:01:19 GMT, Ichiroh Takiguchi wrote: >> Do we need to verify the intermediate byte encoding? Could we simply compare >> the text given to environ.put() in Start process and System.getenv() in >> Verify process match? > > Hello @naotoj . > I think if 2nd process' encoder (like UTF-8) and 3rd process' decoder are > same encoding, System.getenv() returns expected data. > Actually the testcase checks 3 parts on Verify process: > 1. Expected environment variable is defined or not (it uses System.getenv()) > 2. Expected argument is received or not > 3. Expected environment variable is encoded by proper encoding > > When I ran this testcase with jdk18.0.1, I got following result: > > Unexpected argument was received: \u6F22\u5B57<->\u7FB2\u221A\uFFFD > Unexpected environment variables: > \xE6\xBC\xA2\xE5\xAD\x97\x3D\xE6\xBC\xA2\xE5\xAD\x97 > > It means 1st test was passed, 2nd and 3rd test were failed. > I don't think environment variable issue can be seen without 3rd test. > Please let me know if you find out another way. This part of the test is very brittle; I'm pretty sure it will fail on AIX that adds its own environment variables. It should not fail if it finds the two entries it expects. It should ignore other entries. I don't see what value it has over checking the entries from System.getEnv(), please elaborate. - PR: https://git.openjdk.java.net/jdk/pull/8378
Re: RFR: 8285517: System.getenv() returns unexpected value if environment variable has non ASCII character [v5]
On Fri, 6 May 2022 14:23:00 GMT, Ichiroh Takiguchi
wrote:
>> On JDK19 with Linux ja_JP.eucjp locale,
>> System.getenv() returns unexpected value if environment variable has
>> Japanese EUC characters.
>> It seems this issue happens because of JEP 400.
>> Arguments for ProcessBuilder have same kind of issue.
>
> Ichiroh Takiguchi has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8285517: System.getenv() returns unexpected value if environment variable
> has non ASCII character
test/jdk/java/lang/ProcessBuilder/Basic.java line 606:
> 604: ? Charset.forName(jnuEncoding, Charset.defaultCharset())
> 605: : Charset.defaultCharset();
> 606: if (new String(tested.getBytes(cs), cs).equals(tested)) {
Isn't it always true that the round trip encoding to bytes and back (using the
same Charset) to a string is equal()?
And if it is always true, then the if(...) can be removed.
test/jdk/java/lang/System/i18nEnvArg.java line 104:
> 102: String s = System.getenv(EUC_JP_TEXT);
> 103: if (!EUC_JP_TEXT.equals(s)) {
> 104: System.err.println("ERROR: getenv() returns unexpected
> data");
Please add the unexpected data `s` to the output string.
test/jdk/java/lang/System/i18nEnvArg.java line 108:
> 106: if (!EUC_JP_TEXT.equals(args[0])) {
> 107: System.err.print("ERROR: Unexpected argument was
> received: ");
> 108: for(char ch : EUC_JP_TEXT.toCharArray()) {
This is the expected value, the previous "Unexpected" labeling might be
mis-understood.
test/jdk/java/lang/System/i18nEnvArg.java line 111:
> 109:System.err.printf("\\u%04X", (int)ch);
> 110: }
> 111: System.err.print("<->");
This might be clearer if labeled as the actual/incorrect value and on a
separate line.
-
PR: https://git.openjdk.java.net/jdk/pull/8378
Integrated: 8286199: ProblemList jdk/jshell/ExternalEditorTest.java
On Thu, 5 May 2022 15:30:15 GMT, Roger Riggs wrote: > Put jdk/jshell/ExternalEditorTest.java on the problem list due to 8286191. This pull request has now been integrated. Changeset: 2f995c8d Author: Roger Riggs URL: https://git.openjdk.java.net/jdk/commit/2f995c8d2b8650e45e6a360f3c958bfaf46b2ef3 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod 8286199: ProblemList jdk/jshell/ExternalEditorTest.java Reviewed-by: dcubed - PR: https://git.openjdk.java.net/jdk/pull/8557
Re: RFR: 8286199: ProblemList jdk/jshell/ExternalEditorTest.java [v2]
On Thu, 5 May 2022 19:56:36 GMT, Roger Riggs wrote: >> Put jdk/jshell/ExternalEditorTest.java on the problem list due to 8286191. > > Roger Riggs 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 three additional > commits since the last revision: > > - Merge branch 'master' into 8286199-problem-jshell > - 8286199: ProblemList jdk/jshell/ExternalEditorTest.java > - 8286195: ProblemList > test/lib-test/jdk/test/lib/TestMutuallyExclusivePlatformPredicates.java ProblemListing the test to clean up the CI. #8556 seems to have been delayed - PR: https://git.openjdk.java.net/jdk/pull/8557
Re: RFR: 8272352: Java launcher can not parse Chinese character when system locale is set to UTF-8 [v2]
On Tue, 3 May 2022 16:17:00 GMT, Roger Riggs wrote: >> Naoto Sato has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Default to UTF-8 if malloc fails > > src/java.base/windows/native/libjava/java_props_md.c line 695: > >> 693:_encoding); >> 694: >> 695: sprops.sun_jnu_encoding = getEncodingInternal(0); > > How should NULL from `getEncodingInternal` be handled? (only if malloc > fails). Ignore the repeated comment. The code looks fine. - PR: https://git.openjdk.java.net/jdk/pull/8434
Re: RFR: 8286199: ProblemList jdk/jshell/ExternalEditorTest.java [v2]
On Thu, 5 May 2022 19:23:53 GMT, Daniel D. Daugherty wrote: >> Roger Riggs 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 three additional >> commits since the last revision: >> >> - Merge branch 'master' into 8286199-problem-jshell >> - 8286199: ProblemList jdk/jshell/ExternalEditorTest.java >> - 8286195: ProblemList >> test/lib-test/jdk/test/lib/TestMutuallyExclusivePlatformPredicates.java > > test/lib-test/ProblemList.txt line 40: > >> 38: # >> 39: >> # >> 40: jdk/test/lib/TestMutuallyExclusivePlatformPredicates.java 8286191 >> generic-all > > This change shouldn't be here. I think you need to update > your repo to sync with your earlier fix. Updated, after the merge only the langtools ProblemList.txt is modified - PR: https://git.openjdk.java.net/jdk/pull/8557
Re: RFR: 8286199: ProblemList jdk/jshell/ExternalEditorTest.java [v2]
> Put jdk/jshell/ExternalEditorTest.java on the problem list due to 8286191. Roger Riggs 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 three additional commits since the last revision: - Merge branch 'master' into 8286199-problem-jshell - 8286199: ProblemList jdk/jshell/ExternalEditorTest.java - 8286195: ProblemList test/lib-test/jdk/test/lib/TestMutuallyExclusivePlatformPredicates.java - Changes: - all: https://git.openjdk.java.net/jdk/pull/8557/files - new: https://git.openjdk.java.net/jdk/pull/8557/files/1962634e..cd0d157c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8557=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8557=00-01 Stats: 656 lines in 10 files changed: 269 ins; 209 del; 178 mod Patch: https://git.openjdk.java.net/jdk/pull/8557.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8557/head:pull/8557 PR: https://git.openjdk.java.net/jdk/pull/8557
Re: RFR: 8272352: Java launcher can not parse Chinese character when system locale is set to UTF-8 [v2]
On Tue, 3 May 2022 18:55:52 GMT, Naoto Sato wrote: >> Java runtime has been detecting the Windows system locale encoding using >> `GetLocaleInfo(GetSystemDefaultLCID(), LOCALE_IDEFAULTANSICODEPAGE, ...)`, >> but it returns the *legacy* ANSI code page value, e.g, 1252 for US-English. >> In order to detect whether the user has selected `UTF-8` as the default, the >> code page has to be queried with `GetACP()`. >> Also, the case if the call to `GetLocaleInfo` fails changed to fall back to >> `UTF-8` instead of `Cp1252`. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Default to UTF-8 if malloc fails Looks good. src/java.base/windows/native/libjava/java_props_md.c line 695: > 693:_encoding); > 694: > 695: sprops.sun_jnu_encoding = getEncodingInternal(0); How should NULL from `getEncodingInternal` be handled? (only if malloc fails). - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8434
Withdrawn: 8286199: ProblemList jdk/jshell/ExternalEditorTest.java
On Thu, 5 May 2022 15:30:15 GMT, Roger Riggs wrote: > Put jdk/jshell/ExternalEditorTest.java on the problem list due to 8286191. This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/8557
Re: RFR: 8286199: ProblemList jdk/jshell/ExternalEditorTest.java
On Thu, 5 May 2022 15:30:15 GMT, Roger Riggs wrote: > Put jdk/jshell/ExternalEditorTest.java on the problem list due to 8286191. The PR is not needed, the issue will be fixed by PR#8556. - PR: https://git.openjdk.java.net/jdk/pull/8557
Re: RFR: JDK-8286191: misc tests fail due to JDK-8286191
On Thu, 5 May 2022 15:21:23 GMT, Matthias Baesken wrote: > The isMusl method had to be handled in > test/lib-test/jdk/test/lib/TestMutuallyExclusivePlatformPredicates.java . > Additionally, the vm.musl predicate seem not to be available in the langtools > tests. LGTM - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8556
Re: RFR: 8285517: System.getenv() returns unexpected value if environment variable has non ASCII character [v4]
On Thu, 5 May 2022 15:41:44 GMT, Naoto Sato wrote:
>> Hello @naotoj .
>> If I just use `System.getProperty("sun.jnu.encoding")`, following testcases
>> were failed.
>>
>> java/lang/ProcessBuilder/SecurityManagerClinit.java
>> java/lang/ProcessHandle/PermissionTest.java
>>
>> Please give me your suggestion again.
>
> Ah, OK. Never mind.
It might be worthwhile to cache the `sun.jnu.encoding` property value in
jdk/internal/util/StaticProperty.
And perhaps even cache the Charset (not just the string).
-
PR: https://git.openjdk.java.net/jdk/pull/8378
RFR: 8286199: ProblemList jdk/jshell/ExternalEditorTest.java
Put jdk/jshell/ExternalEditorTest.java on the problem list due to 8286191. - Commit messages: - 8286199: ProblemList jdk/jshell/ExternalEditorTest.java - 8286195: ProblemList test/lib-test/jdk/test/lib/TestMutuallyExclusivePlatformPredicates.java Changes: https://git.openjdk.java.net/jdk/pull/8557/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8557=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286199 Stats: 3 lines in 2 files changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8557.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8557/head:pull/8557 PR: https://git.openjdk.java.net/jdk/pull/8557
Re: RFR: JDK-8285987: executing shell scripts without #! fails on Alpine linux
On Thu, 5 May 2022 14:47:22 GMT, Roger Riggs wrote: >> Hi Alan, thanks for the advice; do you think we can put it into the IGNORED >> group ? >> https://github.com/openjdk/jdk/blob/master/test/lib-test/jdk/test/lib/TestMutuallyExclusivePlatformPredicates.java#L53 > > Its now on the ProblemList and Created Issue: > [JDK-8286191](https://bugs.openjdk.java.net/browse/JDK-8286191) Test library > test failure in TestMutuallyExclusivePlatformPredicates.java This PR also caused a test failure in ExternalEditorTest because there is no defined value for "vm.musl" used in @\requires. What test were run before integration? - PR: https://git.openjdk.java.net/jdk/pull/8535
Re: RFR: JDK-8285987: executing shell scripts without #! fails on Alpine linux
On Thu, 5 May 2022 14:15:30 GMT, Matthias Baesken wrote:
>> test/lib/jdk/test/lib/Platform.java line 192:
>>
>>> 190: }
>>> 191:
>>> 192: public static boolean isMusl() {
>>
>> I think this will need test/lib/TestMutuallyExclusivePlatformPredicates.java
>> to be updated too.
>
> Hi Alan, thanks for the advice; do you think we can put it into the IGNORED
> group ?
> https://github.com/openjdk/jdk/blob/master/test/lib-test/jdk/test/lib/TestMutuallyExclusivePlatformPredicates.java#L53
Its now on the ProblemList and Created Issue:
[JDK-8286191](https://bugs.openjdk.java.net/browse/JDK-8286191) Test library
test failure in TestMutuallyExclusivePlatformPredicates.java
-
PR: https://git.openjdk.java.net/jdk/pull/8535
Integrated: 8286195: ProblemList test/lib-test/jdk/test/lib/TestMutuallyExclusivePlatformPredicates.java
On Thu, 5 May 2022 14:33:51 GMT, Roger Riggs wrote: > Add a failing test library test to the ProblemList. > > test/lib-test/jdk/test/lib/TestMutuallyExclusivePlatformPredicates.java This pull request has now been integrated. Changeset: 7022543f Author: Roger Riggs URL: https://git.openjdk.java.net/jdk/commit/7022543fcfa04c628ef962749ed96c8f986dc822 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod 8286195: ProblemList test/lib-test/jdk/test/lib/TestMutuallyExclusivePlatformPredicates.java Reviewed-by: dcubed, lancea - PR: https://git.openjdk.java.net/jdk/pull/8552
RFR: 8286195: ProblemList test/lib-test/jdk/test/lib/TestMutuallyExclusivePlatformPredicates.java
Add a failing test library test to the ProblemList. test/lib-test/jdk/test/lib/TestMutuallyExclusivePlatformPredicates.java - Commit messages: - 8286195: ProblemList test/lib-test/jdk/test/lib/TestMutuallyExclusivePlatformPredicates.java Changes: https://git.openjdk.java.net/jdk/pull/8552/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8552=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286195 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8552.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8552/head:pull/8552 PR: https://git.openjdk.java.net/jdk/pull/8552
Re: RFR: JDK-8286114: [test] show real exception in bomb call in sun/rmi/runtime/Log/checkLogging/CheckLogging.java
On Wed, 4 May 2022 11:37:09 GMT, Matthias Baesken wrote: > There is one TestLibrary.bomb call in > sun/rmi/runtime/Log/checkLogging/CheckLogging.java that is not passing the > exception to bomb, that should be improved. Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8533
Re: RFR: 8272352: Java launcher can not parse Chinese character when system locale is set to UTF-8
On Wed, 27 Apr 2022 20:23:32 GMT, Naoto Sato wrote: > Java runtime has been detecting the Windows system locale encoding using > `GetLocaleInfo(GetSystemDefaultLCID(), LOCALE_IDEFAULTANSICODEPAGE, ...)`, > but it returns the *legacy* ANSI code page value, e.g, 1252 for US-English. > In order to detect whether the user has selected `UTF-8` as the default, the > code page has to be queried with `GetACP()`. > Also, the case if the call to `GetLocaleInfo` fails changed to fall back to > `UTF-8` instead of `Cp1252`. src/java.base/windows/native/libjava/java_props_md.c line 695: > 693:_encoding); > 694: > 695: sprops.sun_jnu_encoding = getEncodingInternal(0); How should NULL from `getEncodingInternal` be handled? (only if malloc fails). Perhaps just it should return the literal "UTF-8". - PR: https://git.openjdk.java.net/jdk/pull/8434
Re: RFR: 8285947: Avoid redundant HashMap.containsKey calls in ZoneName
On Fri, 29 Apr 2022 06:31:22 GMT, Andrey Turbanov wrote:
> `Map.containsKey` call is sometimes unnecessary, when it's known that Map
> doesn't contain `null` values.
> Instead we can just use Map.get and compare result with `null`.
> I found one of such place, where Map.containsKey calls could be eliminated -
> `java.time.format.ZoneName`.
> it gives a bit of performance for `toZid`.
>
>
> Benchmark Mode Cnt Score Error Units
> ZoneNamesBench.useExistingCountry avgt5 10,738 ± 0,065 ns/op
> ZoneNamesBench.useExistingCountryOld avgt5 13,679 ± 0,089 ns/op
>
>
>
> Benchmark
>
>
> @BenchmarkMode(value = Mode.AverageTime)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> @Warmup(iterations = 10, time = 1, timeUnit = TimeUnit.SECONDS)
> @Measurement(iterations = 5, time = 1, timeUnit = TimeUnit.SECONDS)
> @Fork(1)
> @State(Scope.Benchmark)
> public class ZoneNamesBench {
>
> @Benchmark
> public String useExistingCountry() {
> return ZoneName.toZid("Africa/Tunis", Locale.ITALY);
> }
>
> @Benchmark
> public String useExistingCountryOld() {
> return ZoneName.toZidOld("Africa/Tunis", Locale.ITALY);
> }
> }
>
>
>
> public static String toZid(String zid, Locale locale) {
> String mzone = zidToMzone.get(zid);
> if (mzone == null) {
> String alias = aliases.get(zid);
> if (alias != null) {
> zid = alias;
> mzone = zidToMzone.get(zid);
> }
> }
> if (mzone != null) {
> Map map = mzoneToZidL.get(mzone);
> if (map == null || ((zid = map.get(locale.getCountry())) ==
> null)) {
> zid = mzoneToZid.get(mzone);
> }
> }
> return toZid(zid);
> }
>
> public static String toZid(String zid) {
> return aliases.getOrDefault(zid, zid);
> }
>
> public static String toZidOld(String zid, Locale locale) {
> String mzone = zidToMzone.get(zid);
> if (mzone == null && aliases.containsKey(zid)) {
> zid = aliases.get(zid);
> mzone = zidToMzone.get(zid);
> }
> if (mzone != null) {
> Map map = mzoneToZidL.get(mzone);
> if (map != null && map.containsKey(locale.getCountry())) {
> zid = map.get(locale.getCountry());
> } else {
> zid = mzoneToZid.get(mzone);
> }
> }
> return toZidOld(zid);
> }
>
> public static String toZidOld(String zid) {
> if (aliases.containsKey(zid)) {
> return aliases.get(zid);
> }
> return zid;
> }
>
>
Marked as reviewed by rriggs (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/8463
Re: RFR: 8285517: System.getenv() returns unexpected value if environment variable has non ASCII character [v2]
On Sun, 1 May 2022 04:51:17 GMT, Ichiroh Takiguchi wrote: >> On JDK19 with Linux ja_JP.eucjp locale, >> System.getenv() returns unexpected value if environment variable has >> Japanese EUC characters. >> It seems this issue happens because of JEP 400. >> Arguments for ProcessBuilder have same kind of issue. > > Ichiroh Takiguchi has updated the pull request incrementally with one > additional commit since the last revision: > > 8285517: System.getenv() returns unexpected value if environment variable > has non ASCII character Can you clarify the use of different charset properties for the ProcessEnvironment vs the CommandLine strings? They are used to decode or encode strings in the APIs to native functions respectively. If I understand `native.encoding` was introduced to reflect the default encoding of file contents, while `sun.jnu.encoding` is used to encode filenames. I think I would have expected to see `sun.jnu.encoding` to be used in all contexts when encoding/decoding strings to the native representations. (Assuming its not required for backward compatibility). - PR: https://git.openjdk.java.net/jdk/pull/8378
Re: RFR: 8285947: Avoid redundant HashMap.containsKey calls in ZoneName
On Fri, 29 Apr 2022 06:31:22 GMT, Andrey Turbanov wrote:
> `Map.containsKey` call is sometimes unnecessary, when it's known that Map
> doesn't contain `null` values.
> Instead we can just use Map.get and compare result with `null`.
> I found one of such place, where Map.containsKey calls could be eliminated -
> `java.time.format.ZoneName`.
> it gives a bit of performance for `toZid`.
>
>
> Benchmark Mode Cnt Score Error Units
> ZoneNamesBench.useExistingCountry avgt5 10,738 ± 0,065 ns/op
> ZoneNamesBench.useExistingCountryOld avgt5 13,679 ± 0,089 ns/op
>
>
>
> Benchmark
>
>
> @BenchmarkMode(value = Mode.AverageTime)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> @Warmup(iterations = 10, time = 1, timeUnit = TimeUnit.SECONDS)
> @Measurement(iterations = 5, time = 1, timeUnit = TimeUnit.SECONDS)
> @Fork(1)
> @State(Scope.Benchmark)
> public class ZoneNamesBench {
>
> @Benchmark
> public String useExistingCountry() {
> return ZoneName.toZid("Africa/Tunis", Locale.ITALY);
> }
>
> @Benchmark
> public String useExistingCountryOld() {
> return ZoneName.toZidOld("Africa/Tunis", Locale.ITALY);
> }
> }
>
>
>
> public static String toZid(String zid, Locale locale) {
> String mzone = zidToMzone.get(zid);
> if (mzone == null) {
> String alias = aliases.get(zid);
> if (alias != null) {
> zid = alias;
> mzone = zidToMzone.get(zid);
> }
> }
> if (mzone != null) {
> Map map = mzoneToZidL.get(mzone);
> if (map == null || ((zid = map.get(locale.getCountry())) ==
> null)) {
> zid = mzoneToZid.get(mzone);
> }
> }
> return toZid(zid);
> }
>
> public static String toZid(String zid) {
> return aliases.getOrDefault(zid, zid);
> }
>
> public static String toZidOld(String zid, Locale locale) {
> String mzone = zidToMzone.get(zid);
> if (mzone == null && aliases.containsKey(zid)) {
> zid = aliases.get(zid);
> mzone = zidToMzone.get(zid);
> }
> if (mzone != null) {
> Map map = mzoneToZidL.get(mzone);
> if (map != null && map.containsKey(locale.getCountry())) {
> zid = map.get(locale.getCountry());
> } else {
> zid = mzoneToZid.get(mzone);
> }
> }
> return toZidOld(zid);
> }
>
> public static String toZidOld(String zid) {
> if (aliases.containsKey(zid)) {
> return aliases.get(zid);
> }
> return zid;
> }
>
>
src/java.base/share/classes/java/time/format/ZoneName.java.template line 60:
> 58:
> 59: public static String toZid(String zid) {
> 60: return aliases.getOrDefault(zid, zid);
Is the behavior if zid == null the same? aliases.getOrDefault will throw NPE
on null.
neither Hashmap.containsKey or .get throw on null.
-
PR: https://git.openjdk.java.net/jdk/pull/8463
Re: RFR: 8282227: Locale information for nb is not working properly
On Tue, 26 Apr 2022 00:32:48 GMT, Naoto Sato wrote: > This was caused by incorporating CLDR v39, which switched the Norwegian > locale inheritance from `no` -> `nb` to `nb` ->`no` and moved the resources > from `nb` to `no`, which now contradicts Java's locale fallback. Explicitly > inheriting `no` from `nb` will fix the issue. LGTM - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8394
Re: RFR: 8285517: System.getenv() returns unexpected value if environment variable has non ASCII character
On Sun, 24 Apr 2022 09:18:54 GMT, Ichiroh Takiguchi
wrote:
> On JDK19 with Linux ja_JP.eucjp locale,
> System.getenv() returns unexpected value if environment variable has Japanese
> EUC characters.
> It seems this issue happens because of JEP 400.
> Arguments for ProcessBuilder have same kind of issue.
src/java.base/unix/classes/java/lang/ProcessEnvironment.java line 68:
> 66: private static final Map theUnmodifiableEnvironment;
> 67: static final int MIN_NAME_LENGTH = 0;
> 68: static final Charset cs =
> Charset.forName(StaticProperty.nativeEncoding(),
cs should have an all-CAPS name to make it clear its a static value throughout
the implementation.
And `private` too.
test/jdk/java/lang/System/i18nEnvArg.java line 41:
> 39:
> 40: public class i18nEnvArg {
> 41: final static String text = "\u6F22\u5B57";
Can this be renamed to indicate it is the string under test? like KANJI_TEXT
or EUC_JP_TEXT or similar.
test/jdk/java/lang/System/i18nEnvArg.java line 49:
> 47: final static int maxSize = 4096;
> 48:
> 49: public static void main(String[] args) throws Exception {
There are 3 main()'s in this test; it would be useful to add a comment
indicating the purpose of each.
test/jdk/java/lang/System/i18nEnvArg.java line 60:
> 58: Process p = pb.start();
> 59: InputStream is = p.getInputStream();
> 60: byte[] ba = new byte[maxSize];
You can use `InputStream.readAllBytes()` here to return a byte buffer.
Its not clear why all the bytes are read? I assume its only for a possible
error from the child.
test/jdk/java/lang/System/i18nEnvArg.java line 128:
> 126: Method enviorn_mid = cls.getDeclaredMethod("environ");
> 127: enviorn_mid.setAccessible(true);
> 128: byte[][] environ = (byte[][]) enviorn_mid.invoke(null,
typo: "enviorn_mid" -> "environ_mid".
test/jdk/java/lang/System/i18nEnvArg.java line 138:
> 136: bb.put(environ[i+1]);
> 137: byte[] envb = Arrays.copyOf(ba, bb.position());
> 138: if (Arrays.equals(kanji, envb)) return;
It seems odd to exit the loop on the first match.
I think AIX always has environment variables not set by the caller.
test/jdk/java/lang/System/i18nEnvArg.java line 146:
> 144: sb.append((char)b);
> 145: } else {
> 146: sb.append(String.format("\\x%02X", (int)b &
> 0xFF));
The methods of java.util.HexFormat may be useful here.
It seems that the "5C" would fit into this "else" clause and not need a
separate statement.
-
PR: https://git.openjdk.java.net/jdk/pull/8378
Re: RFR: 8285485: Fix typos in corelibs
On Fri, 22 Apr 2022 15:08:51 GMT, Magnus Ihse Bursie wrote:
> I ran `codespell` on modules owned by core-libs, and accepted those changes
> where it indeed discovered real typos.
>
> I will update copyright years using a script before pushing (otherwise like
> every second change would be a copyright update, making reviewing much
> harder).
>
> The long term goal here is to make tooling support for running `codespell`.
> The trouble with automating this is of course all false positives. But before
> even trying to solve that issue, all true positives must be fixed. Hence this
> PR.
Since you've changed some code; you need to run tests for tiers 1-3.
(Note that even for trivial changes, hundreds of OpenJDK developers are
notified and distracted; be considerate of other developers).
src/java.xml/share/classes/com/sun/xml/internal/stream/XMLEventReaderImpl.java
line 140:
> 138: } else if(type == XMLEvent.START_ELEMENT) {
> 139: throw new XMLStreamException(
> 140: "elementGetText() function expects text only element but
> START_ELEMENT was encountered.", event.getLocation());
Should we be fixing typos in third party software; it can easily get wiped out
in an update.
src/java.xml/share/classes/com/sun/xml/internal/stream/writers/WriterUtility.java
line 97:
> 95: }
> 96: else{
> 97: //attempt to retrieve default fEncoderoder
"fEncoderoder" looks out of place, please recheck.
-
PR: https://git.openjdk.java.net/jdk/pull/8364
Re: RFR: 8285690: CloneableReference subtest should not throw CloneNotSupportedException [v2]
On Wed, 27 Apr 2022 10:11:15 GMT, Kim Barrett wrote: >> Please review this fix to test/jdk/java/lang/ref/ReferenceClone.java. It was >> passing if CloneableReference::clone were to throw >> CloneNotSupportedException. >> That should be a failure. >> >> Testing: >> Locally (linux-x64) verified test still passes. Temporarily changed >> CloneableReference::clone to throw and verified the expected failure gets >> reported, unlike before. > > Kim Barrett has updated the pull request incrementally with one additional > commit since the last revision: > > update copyright, @bug list LGTM - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8418
Re: RFR: 8285452: Support new API to replace a file content using FileUtils.java [v3]
On Fri, 22 Apr 2022 14:35:14 GMT, Sibabrata Sahoo wrote: >> A new API to support replacing selective lines with desired content. > > Sibabrata Sahoo has updated the pull request incrementally with one > additional commit since the last revision: > > Update FileUtils.java Please change the issue/pr title to indicate it is a new API in the **test** library. The problem with single purpose APIs with not enough forethought is that they are not discoverable in the library and fall into disuse or are not appropriate for someone else to find and use. - PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Support new API to replace a file content using FileUtils.java [v3]
On Fri, 22 Apr 2022 14:35:14 GMT, Sibabrata Sahoo wrote: >> A new API to support replacing selective lines with desired content. > > Sibabrata Sahoo has updated the pull request incrementally with one > additional commit since the last revision: > > Update FileUtils.java If it is just for one/few tests, it would fit better in the directory with the test or in the test itself. This doesn't seem like enough of a general purpose function to be in the test library. Overwriting the input file seems like a bad choice, it will be a trap for someone. It prevents the use of the function in a case where the output is written to a different file. - PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Support new API to replace a file content using FileUtils.java [v3]
On Fri, 22 Apr 2022 14:35:14 GMT, Sibabrata Sahoo wrote: >> A new API to support replacing selective lines with desired content. > > Sibabrata Sahoo has updated the pull request incrementally with one > additional commit since the last revision: > > Update FileUtils.java Can you elaborate on the use case, what tests would it be used in? The hardcoded `from` and `to` seem very rigid and require knowledge of the exact format. That might lead to very fragile tests. Is this equivalent to looking for a substring aka `String.contains(xxx)` of the input file to replace (with line ending normalization)? - PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285440: Typo in Collections.addAll method javadoc
On Fri, 31 Dec 2021 18:58:43 GMT, Johnny Lim wrote: > This PR fixes a typo. Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6942
Re: RFR: 8285255: refine StringLatin1.regionMatchesCI_UTF16 [v3]
On Wed, 20 Apr 2022 21:08:19 GMT, XenoAmess wrote:
>> some thoughts after watching 8285001: Simplify StringLatin1.regionMatches
>> https://github.com/openjdk/jdk/pull/8292/
>>
>> if (Character.toLowerCase(u1) == Character.toLowerCase(u2)) {
>> continue;
>> }
>>
>> should be changed to
>>
>> if (((u1 == c1) ? CharacterDataLatin1.instance.toLowerCase(c1) :
>> c1) == Character.toLowerCase(u2)) {
>> continue;
>> }
>>
>> as:
>>
>> 1. c1 is LATIN1, so CharacterDataLatin1.instance.toLowerCase seems faster.
>> 2. because c1 is LATIN1, so if u1 != c1, then c1 is already lowercase, and
>> don't need a lowercase cauculation.
>
> XenoAmess has updated the pull request incrementally with one additional
> commit since the last revision:
>
> remove = check
Can you run the JMH against the code before either change (or an existing JDK).
It would be interesting to quantify the improvements of going straight to
Latin1.
(Understanding current hardware architectures and their parallelism is hard to
understand well.
They do clever things with branch prediction and potentially optimistically
executing both paths
and then discarding the non-branch case. The existing code for toLower and
toUpper already includes a branch or two; adding one more branch to the
sequence likely can't be optimized.)
These interactions at the instruction level is why measuring is important.
Thanks
-
PR: https://git.openjdk.java.net/jdk/pull/8308
Re: RFR: 8285255: refine StringLatin1.regionMatchesCI_UTF16 [v2]
On Wed, 20 Apr 2022 20:09:06 GMT, XenoAmess wrote:
>> some thoughts after watching 8285001: Simplify StringLatin1.regionMatches
>> https://github.com/openjdk/jdk/pull/8292/
>>
>> if (Character.toLowerCase(u1) == Character.toLowerCase(u2)) {
>> continue;
>> }
>>
>> should be changed to
>>
>> if (((u1 == c1) ? CharacterDataLatin1.instance.toLowerCase(c1) :
>> c1) == Character.toLowerCase(u2)) {
>> continue;
>> }
>>
>> as:
>>
>> 1. c1 is LATIN1, so CharacterDataLatin1.instance.toLowerCase seems faster.
>> 2. because c1 is LATIN1, so if u1 != c1, then c1 is already lowercase, and
>> don't need a lowercase cauculation.
>
> XenoAmess has updated the pull request incrementally with one additional
> commit since the last revision:
>
> add jmh test
Thanks for the JMH tests and data.
The only part needed from the JMH run is the last 9 lines. The rest is noise.
If it was formatted as a literal it would be easier to read.
What I see is that the run with == is quite a bit slower.
With the == check:
StringOther.regionMatchesU1024LL avgt 15 187.258 ± 1.038 ns/op
StringOther.regionMatchesU1024LU avgt 15 2589.833 ± 8.823 ns/op
StringOther.regionMatchesU1024UL avgt 15 2379.645 ± 6.481 ns/op
StringOther.regionMatchesU1024UU avgt 15 191.587 ± 7.069 ns/op
Without the == check:
StringOther.regionMatchesU1024LL avgt 15 187.732 ± 1.914 ns/op
StringOther.regionMatchesU1024LU avgt 15 1324.156 ± 11.761 ns/op
StringOther.regionMatchesU1024UL avgt 15 1331.857 ± 22.509 ns/op
StringOther.regionMatchesU1024UU avgt 15 188.872 ± 2.396 ns/op
In the JMH cases, does the long prefix of latin1 characters distort the timings?
-
PR: https://git.openjdk.java.net/jdk/pull/8308
Re: RFR: 8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner
On Wed, 20 Apr 2022 00:32:25 GMT, Brent Christian wrote: > Please review this change to replace the finalizer in > `AbstractLdapNamingEnumeration` with a Cleaner. > > The pieces of state required for cleanup (`LdapCtx homeCtx`, `LdapResult > res`, and `LdapClient enumClnt`) are moved to a static inner class . From > there, the change is fairly mechanical. > > Details of note: > 1. Some operations need to change the state values (the update() method is > probably the most interesting). > 2. Subclasses need to access `homeCtx`; I added a `homeCtx()` method to read > `homeCtx` from the superclass's `state`. > > The test case is based on a copy of > `com/sun/jndi/ldap/blits/AddTests/AddNewEntry.java`. A more minimal test case > might be possible, but this was done for expediency. > > The test only confirms that the new Cleaner use does not keep the object > reachable. It only tests `LdapSearchEnumeration` (not `LdapNamingEnumeration` > or `LdapBindingEnumeration`, though all are subclasses of > `AbstractLdapNamingEnumeration`). > > Thanks. src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java line 132: > 130: this.state = new CleaningAction(homeCtx, answer, > homeCtx.clnt); > 131: // Ensures that context won't get closed from underneath us > 132: this.state.homeCtx.incEnumCount(); Readability might be improved by using the new `homeCtx()` method instead of `state.homeCtx` in this file. It would then be consistent with how subclasses access it. It depends which style you prefer to be more consistent with. - PR: https://git.openjdk.java.net/jdk/pull/8311
Re: RFR: 8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner
On Wed, 20 Apr 2022 00:32:25 GMT, Brent Christian wrote:
> Please review this change to replace the finalizer in
> `AbstractLdapNamingEnumeration` with a Cleaner.
>
> The pieces of state required for cleanup (`LdapCtx homeCtx`, `LdapResult
> res`, and `LdapClient enumClnt`) are moved to a static inner class . From
> there, the change is fairly mechanical.
>
> Details of note:
> 1. Some operations need to change the state values (the update() method is
> probably the most interesting).
> 2. Subclasses need to access `homeCtx`; I added a `homeCtx()` method to read
> `homeCtx` from the superclass's `state`.
>
> The test case is based on a copy of
> `com/sun/jndi/ldap/blits/AddTests/AddNewEntry.java`. A more minimal test case
> might be possible, but this was done for expediency.
>
> The test only confirms that the new Cleaner use does not keep the object
> reachable. It only tests `LdapSearchEnumeration` (not `LdapNamingEnumeration`
> or `LdapBindingEnumeration`, though all are subclasses of
> `AbstractLdapNamingEnumeration`).
>
> Thanks.
src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
line 59:
> 57: * cleanup, which happens by calling cleanup(), or is done by the
> Cleaner.
> 58: */
> 59: private static class CleaningAction implements Runnable {
This nested class might be more aptly named `EnumCtx` since it is used during
the enumeration process.
The mention of the `cleanup()` method in the nested class javadoc is a bit
ambiguous, it seems there should be a cleanup() method in the class, but it is
in AbstractLdapNamingEnumeration.
The `state` field might also be renamed `enumCtx`.
-
PR: https://git.openjdk.java.net/jdk/pull/8311
Re: RFR: 8285255: refine StringLatin1.regionMatchesCI_UTF16
On Tue, 19 Apr 2022 21:15:29 GMT, XenoAmess wrote:
> some thoughts after watching 8285001: Simplify StringLatin1.regionMatches
> https://github.com/openjdk/jdk/pull/8292/
>
> if (Character.toLowerCase(u1) == Character.toLowerCase(u2)) {
> continue;
> }
>
> should be changed to
>
> if (((u1 == c1) ? CharacterDataLatin1.instance.toLowerCase(c1) :
> c1) == Character.toLowerCase(u2)) {
> continue;
> }
>
> as:
>
> 1. c1 is LATIN1, so CharacterDataLatin1.instance.toLowerCase seems faster.
> 2. because c1 is LATIN1, so if u1 != c1, then c1 is already lowercase, and
> don't need a lowercase cauculation.
The test should be able to use public string classes with selected Latin1 and
utf-16 arguments.
The implementation of regionMatches will end up using the corresponding
implementations.
The JMH in PR#8292 might be a start.
-
PR: https://git.openjdk.java.net/jdk/pull/8308
Re: RFR: 8285255: refine StringLatin1.regionMatchesCI_UTF16
On Tue, 19 Apr 2022 21:15:29 GMT, XenoAmess wrote:
> some thoughts after watching 8285001: Simplify StringLatin1.regionMatches
> https://github.com/openjdk/jdk/pull/8292/
>
> if (Character.toLowerCase(u1) == Character.toLowerCase(u2)) {
> continue;
> }
>
> should be changed to
>
> if (((u1 == c1) ? CharacterDataLatin1.instance.toLowerCase(c1) :
> c1) == Character.toLowerCase(u2)) {
> continue;
> }
>
> as:
>
> 1. c1 is LATIN1, so CharacterDataLatin1.instance.toLowerCase seems faster.
> 2. because c1 is LATIN1, so if u1 != c1, then c1 is already lowercase, and
> don't need a lowercase cauculation.
Point 1 make sense.
Point 2 adds a branch and branches aren't free.
Can you show some jmh data for these cases?
-
PR: https://git.openjdk.java.net/jdk/pull/8308
Re: RFR: 8284930: Re-examine FilterInputStream mark/reset
On Wed, 20 Apr 2022 11:56:20 GMT, Jaikiran Pai wrote: > I wonder if it should be removed from InputStream at the same time. I took the presence of synchronized on those empty methods as a hint to subclasses that they too should be synchronized. - PR: https://git.openjdk.java.net/jdk/pull/8309
Re: RFR: 8279185: Support for IsoFields in JapaneseDate/MinguoDate/ThaiBuddhistDate [v9]
On Tue, 19 Apr 2022 17:43:21 GMT, Naoto Sato wrote: >> Supporting `IsoFields` temporal fields in chronologies that are similar to >> ISO chronology. Corresponding CSR has also been drafted. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Addressing review comments Looks good, thanks - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7683
Re: RFR: 8279185: Support for IsoFields in JapaneseDate/MinguoDate/ThaiBuddhistDate [v4]
On Mon, 7 Mar 2022 18:47:17 GMT, Naoto Sato wrote:
>> src/java.base/share/classes/java/time/chrono/IsoChronology.java line 689:
>>
>>> 687: */
>>> 688: @Override
>>> 689: public boolean isIsoBased() {
>>
>> Is this meant to be 'default'? The CSR indicated adding default methods.
>
> The `default` method has been added to `java.time.chrono.Chronology`
> interface. This is its overridden method implemented in `IsoChronology`
> concrete class.
To make navigation easier, change the @code references to be @links and include
the IsoField name.
I don't think the @implspec is needed, its redundant with @return.
Apply similar changes to the other ISO chronologies javadoc.
-
PR: https://git.openjdk.java.net/jdk/pull/7683
Re: RFR: 8279185: Support for IsoFields in JapaneseDate/MinguoDate/ThaiBuddhistDate [v7]
On Fri, 15 Apr 2022 18:47:53 GMT, Naoto Sato wrote:
>> Supporting `IsoFields` temporal fields in chronologies that are similar to
>> ISO chronology. Corresponding CSR has also been drafted.
>
> Naoto Sato 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 15 additional commits since
> the last revision:
>
> - Revert to use the default method
> - Merge branch 'master' into JDK-8279185
> - Removed unnecessary package names
> - Modified class desc of `IsoFields`
> - abstract class -> top level interface
> - interface -> abstract class
> - Merge branch 'master' into JDK-8279185
> - Removed the method
> - Merge branch 'master' into JDK-8279185
> - Changed to use a type to determine ISO based or not
> - ... and 5 more:
> https://git.openjdk.java.net/jdk/compare/5ee1a816...82339ec6
src/java.base/share/classes/java/time/chrono/Chronology.java line 794:
> 792: * @since 19
> 793: */
> 794: default boolean isIsoBased() {
Can the description be more specific. Each of the chronologies mentioned
say they have the same number of months, the number of days in each month, and
day-of-year and leap years are the same as ISO. The week-based fields in
IsoFields also depend ISO defined concepts.
Perhaps it should say that this method should return true only if all of the
fields of IsoFields are supported for the chronology.
The chronology names could be links and omit the @see tags, including
ISOChronology.
In the @return add ", otherwise return {@code false}."
src/java.base/share/classes/java/time/temporal/IsoFields.java line 600:
> 598: if (!isIsoBased(temporal)) {
> 599: throw new DateTimeException("Resolve requires ISO based
> chronology: " +
> 600: Chronology.from(temporal));
The name change doesn't add much, I'd leave both `ensureIso` and `isIso` method
names unchanged.
src/java.base/share/classes/java/time/temporal/IsoFields.java line 739:
> 737:
> 738: static boolean isIsoBased(TemporalAccessor temporal) {
> 739: return Chronology.from(temporal).isIsoBased();
Can this method be private static?
Also, move the `ensureIso` method to be next to `isIso`.
-
PR: https://git.openjdk.java.net/jdk/pull/7683
Re: RFR: 8285001: Simplify StringLatin1.regionMatches
On Tue, 19 Apr 2022 08:50:09 GMT, Claes Redestad wrote: > There is no pair of character values in the latin1 range (0-255) that will > make the condition at line 401 in `StringLatin1.java` true, so this test can > be removed. > > Added a test and a microbenchmark (which as expected sees a few ns/op > improvement from this change). Took the opportunity to tune the default > settings for the micro to reduce runtime from 30+ minutes to 3 with no > discernible degradation of quality. Nice Catch! - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8292
Integrated: JDK-8282008: Incorrect handling of quoted arguments in ProcessBuilder
On Fri, 4 Mar 2022 23:20:21 GMT, Roger Riggs wrote: > Quoting related changes in https://bugs.openjdk.java.net/browse/JDK-8250568 > modified the way that > process builder recognized argument strings, causing some arguments to be > doubly quoted and malformed. > > ProcessBuilder encodes command arguments in two ways, a looser legacy encoding > and stricter encoding that prevents quotes from being misinterpreted. > The system property jdk.lang.Process.allowAmbiguousCommands controls which is > in effect. > > When the property is "true" or not set, arguments are inserted into the > Windows command line > with minimal changes. Arguments containing space or tab are quoted to > prevent them being split. > Arguments that start and end with double-quote are left alone. > Some executables interpret a backslash before the final quote as an escape; > if the argument > contains first and last quotes, backslashes are ignored. > > When the allowAmbigousCommands property is `false`, care is taken to ensure > that > the final quote of an argument is the closing quote for the argument and is > not > interpreted as a literal quote by a preceding quote (or an odd number of > quotes). > > The PR includes a test matrix of the cases where an argument with spaces and > a final backslash > is passed with each combination of `allowAmbiguousCommands = true and false`, > launched executable, java, .cmd, and .vbs and when the argument is surrounded > with double-quotes. > > The priority for allowAmbiguousCommands = false is that no argument is split > or joined to another argument. > In some cases, backslashes are doubled to prevent a double-quote from being > interpreted incorrectly. > The trailing backslash in an argument occurs rarely exception when the > argument is a directory. > In that case, the addition of trailing backslashes is benign when the string > is used as a filesystem path. > > See also PR#7504, for background and a proposal. This pull request has now been integrated. Changeset: 897d6c0d Author:Roger Riggs URL: https://git.openjdk.java.net/jdk/commit/897d6c0dc7cdfb3ad92f864f9ad4b50e642197e4 Stats: 343 lines in 2 files changed: 332 ins; 6 del; 5 mod 8282008: Incorrect handling of quoted arguments in ProcessBuilder Reviewed-by: bchristi - PR: https://git.openjdk.java.net/jdk/pull/7709
Re: RFR: JDK-8282008: Incorrect handling of quoted arguments in ProcessBuilder [v3]
On Tue, 5 Apr 2022 16:39:33 GMT, Roger Riggs wrote: >> Quoting related changes in https://bugs.openjdk.java.net/browse/JDK-8250568 >> modified the way that >> process builder recognized argument strings, causing some arguments to be >> doubly quoted and malformed. >> >> ProcessBuilder encodes command arguments in two ways, a looser legacy >> encoding >> and stricter encoding that prevents quotes from being misinterpreted. >> The system property jdk.lang.Process.allowAmbiguousCommands controls which >> is in effect. >> >> When the property is "true" or not set, arguments are inserted into the >> Windows command line >> with minimal changes. Arguments containing space or tab are quoted to >> prevent them being split. >> Arguments that start and end with double-quote are left alone. >> Some executables interpret a backslash before the final quote as an escape; >> if the argument >> contains first and last quotes, backslashes are ignored. >> >> When the allowAmbigousCommands property is `false`, care is taken to ensure >> that >> the final quote of an argument is the closing quote for the argument and is >> not >> interpreted as a literal quote by a preceding quote (or an odd number of >> quotes). >> >> The PR includes a test matrix of the cases where an argument with spaces and >> a final backslash >> is passed with each combination of `allowAmbiguousCommands = true and false`, >> launched executable, java, .cmd, and .vbs and when the argument is >> surrounded with double-quotes. >> >> The priority for allowAmbiguousCommands = false is that no argument is split >> or joined to another argument. >> In some cases, backslashes are doubled to prevent a double-quote from being >> interpreted incorrectly. >> The trailing backslash in an argument occurs rarely exception when the >> argument is a directory. >> In that case, the addition of trailing backslashes is benign when the string >> is used as a filesystem path. >> >> See also PR#7504, for background and a proposal. > > Roger Riggs 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 five additional > commits since the last revision: > > - Merge branch 'master' into 8282008-quoted-escape > - Add count of skipped tests and improve comments > - Refactored ArgCheck test to be more readable and easier to maintain and > backport > - Cleanup comment and copyright > - JDK-8282008: Incorrect handling of quoted arguments in ProcessBuilder This change required a CSR, now approved. - PR: https://git.openjdk.java.net/jdk/pull/7709
Re: RFR: JDK-8284874: Add comment to ProcessHandle/OnExitTest to describe zombie problem [v3]
On Thu, 14 Apr 2022 18:29:21 GMT, Thomas Stuefe wrote: >> ProcessHandle/OnExitTest is vulnerable to misconfigured systems that do not >> reap zombies in a timely fashion. >> [JDK-8284282](https://bugs.openjdk.java.net/browse/JDK-8284282) describes >> this problem in detail. >> >> Until we figure out a way to fix that (if at all - see comments in JBS), let >> us have a clarifying comment in the test itself. > > Thomas Stuefe has updated the pull request incrementally with one additional > commit since the last revision: > > fix copyright Marked as reviewed by rriggs (Reviewer). Core-libs issues only require 1 reviewer; though some more complex issues deserve more than 1. Thanks - PR: https://git.openjdk.java.net/jdk/pull/8240
