Re: RFR: 8312151: Deprecate for removal the -Xdebug option
On Tue, 18 Jul 2023 06:59:52 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which proposes to deprecate for > removal the `-Xdebug` option of the `java` command? This addresses > https://bugs.openjdk.org/browse/JDK-8312151? > > As noted in the JBS issue this option is currently a no-op and has been there > only for backward compatible since even Java 8 days. > > A CSR has been drafted for this change > https://bugs.openjdk.org/browse/JDK-8312216 I would like inputs on the `test/hotspot/jtreg/runtime/6294277/SourceDebugExtension.java` test. That test launches `java` passing it the `-Xdebug` option and was introduced as part of https://bugs.openjdk.org/browse/JDK-6294277. `-Xdebug` has been a no-op for many releases now. Is this test still relevant and if not, should I go ahead and remove it as part of this change? - PR Comment: https://git.openjdk.org/jdk/pull/14918#issuecomment-1639615535
RFR: 8312151: Deprecate for removal the -Xdebug option
Can I please get a review of this change which proposes to deprecate for removal the `-Xdebug` option of the `java` command? This addresses https://bugs.openjdk.org/browse/JDK-8312151? As noted in the JBS issue this option is currently a no-op and has been there only for backward compatible since even Java 8 days. A CSR has been drafted for this change https://bugs.openjdk.org/browse/JDK-8312216 - Commit messages: - 8312151: Deprecate for removal the -Xdebug option Changes: https://git.openjdk.org/jdk/pull/14918/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14918&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8312151 Stats: 35 lines in 22 files changed: 0 ins; 8 del; 27 mod Patch: https://git.openjdk.org/jdk/pull/14918.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14918/head:pull/14918 PR: https://git.openjdk.org/jdk/pull/14918
Re: RFR: 8312151: Deprecate for removal the -Xdebug option
On Tue, 18 Jul 2023 06:59:52 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which proposes to deprecate for > removal the `-Xdebug` option of the `java` command? This addresses > https://bugs.openjdk.org/browse/JDK-8312151? > > As noted in the JBS issue this option is currently a no-op and has been there > only for backward compatible since even Java 8 days. > > A CSR has been drafted for this change > https://bugs.openjdk.org/browse/JDK-8312216 The launcher option -debug is an alias for -Xdebug so should be deprecated too. The history on these options goes back a long way, but obsolete since JDK 6 and the removal of JVMDI at least. - PR Comment: https://git.openjdk.org/jdk/pull/14918#issuecomment-1639660251
Re: RFR: 8312151: Deprecate for removal the -Xdebug option [v2]
> Can I please get a review of this change which proposes to deprecate for > removal the `-Xdebug` option of the `java` command? This addresses > https://bugs.openjdk.org/browse/JDK-8312151? > > As noted in the JBS issue this option is currently a no-op and has been there > only for backward compatible since even Java 8 days. > > A CSR has been drafted for this change > https://bugs.openjdk.org/browse/JDK-8312216 Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: update java man page for -Xdebug deprecation - Changes: - all: https://git.openjdk.org/jdk/pull/14918/files - new: https://git.openjdk.org/jdk/pull/14918/files/e165dc24..a8d472ad Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14918&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14918&range=00-01 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14918.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14918/head:pull/14918 PR: https://git.openjdk.org/jdk/pull/14918
Re: RFR: 8227229: Deprecate the launcher -Xdebug/-debug flags that have not done anything since Java 6 [v3]
> Can I please get a review of this change which proposes to deprecate for > removal the `-Xdebug` option and `-debug` option of the `java` command? This > addresses https://bugs.openjdk.org/browse/JDK-8227229. > > As noted in the JBS issue this option is currently a no-op and has been there > only for backward compatible since even Java 8 days. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: include -debug (alias) option in the deprecated log message - Changes: - all: https://git.openjdk.org/jdk/pull/14918/files - new: https://git.openjdk.org/jdk/pull/14918/files/a8d472ad..e5472701 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14918&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14918&range=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14918.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14918/head:pull/14918 PR: https://git.openjdk.org/jdk/pull/14918
Re: RFR: 8227229: Deprecate the launcher -Xdebug/-debug flags that have not done anything since Java 6 [v3]
On Tue, 18 Jul 2023 07:58:58 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to deprecate for >> removal the `-Xdebug` option and `-debug` option of the `java` command? >> This addresses https://bugs.openjdk.org/browse/JDK-8227229. >> >> As noted in the JBS issue this option is currently a no-op and has been >> there only for backward compatible since even Java 8 days. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > include -debug (alias) option in the deprecated log message Given that https://bugs.openjdk.org/browse/JDK-8227229 had more historical details about `-Xdebug` and `-debug`, I've updated this PR to link to that issue and also drafted CSR afresh https://bugs.openjdk.org/browse/JDK-8312220 - PR Comment: https://git.openjdk.org/jdk/pull/14918#issuecomment-1639725244
Re: RFR: 8302987: Add uniform and spatially equidistributed bounded double streams to RandomGenerator [v10]
On Tue, 18 Jul 2023 02:54:55 GMT, Joe Darcy wrote: >> Raffaello Giulietti has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Correction in comment. > > src/java.base/share/classes/java/util/random/RandomGenerator.java line 398: > >> 396: * afforded by nextLong(). >> 397: */ >> 398: delta = nextUp(left) - left; > > The delta computations are equivalent to some expression phrased in terms of > an ulp (Math.ulp) of a the endpoint or a value adjacent to the endpoint. If > the ulp method is not used for the computation, I suggest adding a comment > noting the equivalence. Here `left` is negative or `0.0`. What is needed is the distance to the next adjacent `double` in the direction of positive infinity. This is almost always the same as `Math.ulp(left)`, but not always. For example, when `left` is `-1.0`, `delta` as computed here is half of `Math.ulp(left)`. Analogously for the case covered by the `else` arm. - PR Review Comment: https://git.openjdk.org/jdk/pull/12719#discussion_r1266404939
Re: RFR: 8302987: Add uniform and spatially equidistributed bounded double streams to RandomGenerator [v11]
> The `default` method `nextDouble(double origin, double bound)` in > `java.util.random.RandomGenerator` aims at generating a uniformly and > spatially equidistributed random `double` in the left-closed and right-open > range [`origin`, `bound`). It does so by applying the affine transform > `origin + (bound - origin) * r` to a uniformly and spatially equidistributed > random `double` `r` in the range [0, 1). > > Since floating-point arithmetic suffers from small but noticeable rounding > errors, this ends up slightly deforming the distribution of `r` when applying > the affine transform. Raffaello Giulietti has updated the pull request incrementally with one additional commit since the last revision: Added the reason for not using Math.ulp() to compute delta. - Changes: - all: https://git.openjdk.org/jdk/pull/12719/files - new: https://git.openjdk.org/jdk/pull/12719/files/80e49d41..390ad6b1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12719&range=10 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12719&range=09-10 Stats: 5 lines in 1 file changed: 5 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/12719.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12719/head:pull/12719 PR: https://git.openjdk.org/jdk/pull/12719
Re: RFR: 8302987: Add uniform and spatially equidistributed bounded double streams to RandomGenerator [v12]
> The `default` method `nextDouble(double origin, double bound)` in > `java.util.random.RandomGenerator` aims at generating a uniformly and > spatially equidistributed random `double` in the left-closed and right-open > range [`origin`, `bound`). It does so by applying the affine transform > `origin + (bound - origin) * r` to a uniformly and spatially equidistributed > random `double` `r` in the range [0, 1). > > Since floating-point arithmetic suffers from small but noticeable rounding > errors, this ends up slightly deforming the distribution of `r` when applying > the affine transform. Raffaello Giulietti has updated the pull request incrementally with one additional commit since the last revision: Made the comment about not using ulp a bit clearer. - Changes: - all: https://git.openjdk.org/jdk/pull/12719/files - new: https://git.openjdk.org/jdk/pull/12719/files/390ad6b1..8fc7cfc6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12719&range=11 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12719&range=10-11 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/12719.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12719/head:pull/12719 PR: https://git.openjdk.org/jdk/pull/12719
Re: RFR: 8227229: Deprecate the launcher -Xdebug/-debug flags that have not done anything since Java 6 [v3]
On Tue, 18 Jul 2023 07:58:58 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to deprecate for >> removal the `-Xdebug` option and `-debug` option of the `java` command? >> This addresses https://bugs.openjdk.org/browse/JDK-8227229. >> >> As noted in the JBS issue this option is currently a no-op and has been >> there only for backward compatible since even Java 8 days. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > include -debug (alias) option in the deprecated log message src/java.base/share/classes/sun/launcher/resources/launcher.properties line 137: > 135: \-Xcheck:jni perform additional checks for JNI functions\n\ > 136: \-Xcompforces compilation of methods on first > invocation\n\ > 137: \-Xdebug deprecated and may be removed in a future > release.\n\ This drops "does nothing" so no longer clear that the option doesn't do anything. It could be changed to "does nothing; deprecated for removal in a future release". Alternatively maybe it would be better to just remove it from the -X output. - PR Review Comment: https://git.openjdk.org/jdk/pull/14918#discussion_r1266609817
Re: RFR: 8227229: Deprecate the launcher -Xdebug/-debug flags that have not done anything since Java 6 [v4]
> Can I please get a review of this change which proposes to deprecate for > removal the `-Xdebug` option and `-debug` option of the `java` command? This > addresses https://bugs.openjdk.org/browse/JDK-8227229. > > As noted in the JBS issue this option is currently a no-op and has been there > only for backward compatible since even Java 8 days. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: Alan's suggestion for -Xdebug output - Changes: - all: https://git.openjdk.org/jdk/pull/14918/files - new: https://git.openjdk.org/jdk/pull/14918/files/e5472701..78498068 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14918&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14918&range=02-03 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/14918.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14918/head:pull/14918 PR: https://git.openjdk.org/jdk/pull/14918
Re: RFR: 8227229: Deprecate the launcher -Xdebug/-debug flags that have not done anything since Java 6 [v3]
On Tue, 18 Jul 2023 11:01:33 GMT, Alan Bateman wrote: >> Jaikiran Pai has updated the pull request incrementally with one additional >> commit since the last revision: >> >> include -debug (alias) option in the deprecated log message > > src/java.base/share/classes/sun/launcher/resources/launcher.properties line > 137: > >> 135: \-Xcheck:jni perform additional checks for JNI functions\n\ >> 136: \-Xcompforces compilation of methods on first >> invocation\n\ >> 137: \-Xdebug deprecated and may be removed in a future >> release.\n\ > > This drops "does nothing" so no longer clear that the option doesn't do > anything. It could be changed to "does nothing; deprecated for removal in a > future release". Alternatively maybe it would be better to just remove it > from the -X output. Hello Alan, I updated the PR to use your suggested wording. I thought it's of no harm to let it be present in the -X output until we actually remove it (soon). - PR Review Comment: https://git.openjdk.org/jdk/pull/14918#discussion_r1266705837
RFR: 8311939: Excessive allocation of Matcher.groups array
Reduces excessive allocation of Matcher.groups array when the original Pattern has no groups or less than 9 groups. Original clamping to 10 possibly due to documented behavior from javadoc: "In this class, \1 through \9 are always interpreted as back references, " Only with Matcher changes RegExTest.backRefTest fails when backreferences to non-existing groups are present. Added a match failure condition in Pattern that fixes failing tests. As per existing `java.util.regex.Pattern.BackRef#match`: "// If the referenced group didn't match, neither can this" A group that does not exist in the original Pattern can never match so neither can a backref to that group. If the group existed in the original Pattern then it would have had space allocated in Matcher.groups for that group index. So a group index outside groups array length must never match. - Commit messages: - 8311939: Excessive allocation of Matcher.groups array Changes: https://git.openjdk.org/jdk/pull/14894/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14894&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8311939 Stats: 9 lines in 2 files changed: 7 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14894.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14894/head:pull/14894 PR: https://git.openjdk.org/jdk/pull/14894
Re: RFR: 8311188: Simplify and modernize equals and hashCode in java.text [v5]
On Mon, 17 Jul 2023 23:33:37 GMT, Pavel Rappo wrote: >> Please review this PR to use modern APIs and language features to simplify >> `equals` and `hashCode` in the java.text area. >> >> * Some changes to `equals` and `hashCode` are refactoring rather than >> modernization. Such changes can be as trivial as rearranging, adding, or >> commenting checks. >> >> * java.text area contains more classes whose `equals` and `hashCode` could >> be simplified; for example: sun.text.CompactByteArray or >> java.text.DigitList. However, I found no evidence of `equals` and `hashCode` >> in those classes being used in source or tests. Since writing new tests in >> this case seems unreasonable, I **excluded** those classes from this PR. > > Pavel Rappo has updated the pull request incrementally with one additional > commit since the last revision: > > Feedback Marked as reviewed by rriggs (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14752#pullrequestreview-1535005629
Re: RFR: JDK-8310913 Move ReferencedKeyMap to jdk.internal so it may be shared [v8]
On Thu, 13 Jul 2023 15:07:35 GMT, Jim Laskey wrote:
>> java.lang.runtime.ReferencedKeyMap was introduced to provide a concurrent
>> caching scheme for Carrier objects. The technique used is generally useful
>> for a variety of caching schemes and is being moved to be shared in other
>> parts of the jdk. The MethodType interning case is one example.
>
> Jim Laskey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update test to check for gc.
src/java.base/share/classes/jdk/internal/util/ReferencedKeyMap.java line 354:
> 352: */
> 353: @SuppressWarnings("unchecked")
> 354: K intern(K key) {
Should this be made static for type safety?
public static E intern(ReferencedKeyMap> m, E key)
which will save the 2 unchecked casts at `get` and `putIfAbsent`.
In addition, it would be nice if we can compute a more proper key for interning
than the query key, via a binary operator; for example, we want a `String` to
go through `String::intern`, or a `BaseLocale` to format its language, script,
region, and variant and intern them (see
https://github.com/openjdk/jdk/pull/14404/files#diff-c0e20847ffb6e33bcf62ff52b1b5b4c8a85520ca101a6f54128d97289cd8c2f9R169-R172)
src/java.base/share/classes/jdk/internal/util/ReferencedKeySet.java line 71:
> 69: * @param the type of elements maintained by this set
> 70: */
> 71: public class ReferencedKeySet extends AbstractSet {
Suggestion:
public final class ReferencedKeySet extends AbstractSet {
Make this class final.
src/java.base/share/classes/jdk/internal/util/ReferencedKeySet.java line 97:
> 95: * @param the type of elements maintained by this set
> 96: */
> 97: public static jdk.internal.util.ReferencedKeySet
Suggestion:
public static ReferencedKeySet
src/java.base/share/classes/jdk/internal/util/ReferencedKeySet.java line 185:
> 183: * @return the old element if present, otherwise, the new element
> 184: */
> 185: public T intern(T e) {
I would like to see an additional `T intern(T e, UnaryOperator
internFunction)`, where the `internFunction` behaves like
`LocalObjectCache::normalizeKey`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14684#discussion_r1266902706
PR Review Comment: https://git.openjdk.org/jdk/pull/14684#discussion_r1266911305
PR Review Comment: https://git.openjdk.org/jdk/pull/14684#discussion_r1266897522
PR Review Comment: https://git.openjdk.org/jdk/pull/14684#discussion_r1266914091
Re: RFR: JDK-8310913 Move ReferencedKeyMap to jdk.internal so it may be shared [v5]
On Thu, 6 Jul 2023 19:58:09 GMT, Mandy Chung wrote:
>> Jim Laskey has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add flag for reference queue type
>
> src/java.base/share/classes/jdk/internal/util/ReferencedKeyMap.java line 132:
>
>> 130: */
>> 131: public static ReferencedKeyMap
>> 132: create(boolean isSoft, boolean useNativeQueue,
>> Supplier, V>> supplier) {
>
> I suggest to keep the previous `create(boolean isSoft, Supplier supplier)`
> factory method that defaults to non-native reference queue, which is commonly
> used. Only `MethodType` needs to use native reference queue.
Mandy: The no-`NativeQueue` version is already implemented above.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14684#discussion_r1266908707
Re: RFR: 8311188: Simplify and modernize equals and hashCode in java.text [v5]
On Mon, 17 Jul 2023 23:33:37 GMT, Pavel Rappo wrote: >> Please review this PR to use modern APIs and language features to simplify >> `equals` and `hashCode` in the java.text area. >> >> * Some changes to `equals` and `hashCode` are refactoring rather than >> modernization. Such changes can be as trivial as rearranging, adding, or >> commenting checks. >> >> * java.text area contains more classes whose `equals` and `hashCode` could >> be simplified; for example: sun.text.CompactByteArray or >> java.text.DigitList. However, I found no evidence of `equals` and `hashCode` >> in those classes being used in source or tests. Since writing new tests in >> this case seems unreasonable, I **excluded** those classes from this PR. > > Pavel Rappo has updated the pull request incrementally with one additional > commit since the last revision: > > Feedback Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14752#pullrequestreview-1535273279
Integrated: 8311188: Simplify and modernize equals and hashCode in java.text
On Mon, 3 Jul 2023 11:12:32 GMT, Pavel Rappo wrote: > Please review this PR to use modern APIs and language features to simplify > `equals` and `hashCode` in the java.text area. > > * Some changes to `equals` and `hashCode` are refactoring rather than > modernization. Such changes can be as trivial as rearranging, adding, or > commenting checks. > > * java.text area contains more classes whose `equals` and `hashCode` could be > simplified; for example: sun.text.CompactByteArray or java.text.DigitList. > However, I found no evidence of `equals` and `hashCode` in those classes > being used in source or tests. Since writing new tests in this case seems > unreasonable, I **excluded** those classes from this PR. This pull request has now been integrated. Changeset: 1dfb0fb3 Author:Pavel Rappo URL: https://git.openjdk.org/jdk/commit/1dfb0fb3e22c3616fdfa3a8249be526c44dbe890 Stats: 127 lines in 15 files changed: 29 ins; 55 del; 43 mod 8311188: Simplify and modernize equals and hashCode in java.text Reviewed-by: lancea, naoto, rriggs - PR: https://git.openjdk.org/jdk/pull/14752
Re: RFR: 8309622: Re-examine the cache mechanism in BaseLocale [v4]
On Thu, 29 Jun 2023 19:28:05 GMT, Naoto Sato wrote: >> This is stemming from the PR: https://github.com/openjdk/jdk/pull/14211 >> where aggressive GC can cause NPE in `BaseLocale$Key` class. I refactored >> the in-house cache with WeakHashMap, and removed the Key class as it is no >> longer needed (thus the original NPE will no longer be possible). Also with >> the new JMH test case, it gains some performance improvement: >> >> (w/o fix) >> >> Benchmark Mode Cnt Score Error Units >> LocaleCache.testForLanguageTag avgt 20 5781.275 ± 569.580 ns/op >> LocaleCache.testLocaleOfavgt 20 62564.079 ± 406.697 ns/op >> >> (w/ fix) >> Benchmark Mode Cnt Score Error Units >> LocaleCache.testForLanguageTag avgt 20 4801.175 ± 371.830 ns/op >> LocaleCache.testLocaleOfavgt 20 60394.652 ± 352.471 ns/op > > 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 14 additional commits since > the last revision: > > - Merge branch 'pull/14684' into JDK-8309622-Cache-BaseLocale > - Use ReferencedKeyMap in place for LocaleObjectCache > - Merge branch 'pull/14684' into JDK-8309622-Cache-BaseLocale > - Addressing review comments > - Addressing comments (test grouping, synchronization), minor optimization > on loop lookup > - minor comment fix > - equals/hash fix, constructor simplification > - Removed Key > - minor fixup > - Use WeakHashMap > - ... and 4 more: https://git.openjdk.org/jdk/compare/87a0fc50...870ec1fe src/java.base/share/classes/sun/util/locale/BaseLocale.java line 167: > 165: // can subsequently be used by the Locale instance which > 166: // guarantees the locale components are properly cased/interned. > 167: return CACHE.computeIfAbsent(new BaseLocale(language, script, > region, variant), This `computeIfAbsent` will store the non-normalized base locale as a soft key in the cache, which is undesirable. I have commented on the base patch https://github.com/openjdk/jdk/pull/14684#discussion_r1266914091 to support distinct keys for querying and storing like those in the old `LocalObjectCache`, and we can then simply use a `ReferenceKeySet` for the cache. - PR Review Comment: https://git.openjdk.org/jdk/pull/14404#discussion_r1266923844
Re: RFR: 8311631: When multiple users run tools/jpackage/share/LicenseTest.java, Permission denied for writing /var/tmp/*.files [v2]
On Thu, 13 Jul 2023 07:27:22 GMT, yaqsun wrote: >> The prerequisite is to install the rpmbuild command, when multiple users >> switch to write /var/tmp/*.files will have the permission forbidden. > > yaqsun has updated the pull request incrementally with one additional commit > since the last revision: > > 8311631: When multiple users run tools/jpackage/share/LicenseTest.java, > Permission denied for writing /var/tmp/*.files Marked as reviewed by asemenyuk (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14802#pullrequestreview-1535290149
Re: RFR: 8308591: JLine as the default Console provider [v5]
On Mon, 17 Jul 2023 20:46:32 GMT, Naoto Sato wrote:
>> This is to bring the JLine-based Console implementation as the default,
>> which was the initial intention in
>> [JDK-8295803](https://bugs.openjdk.org/browse/JDK-8295803). A corresponding
>> CSR has also been drafted.
>
> Naoto Sato has updated the pull request incrementally with one additional
> commit since the last revision:
>
> `true`
src/java.base/share/classes/java/io/Console.java line 354:
> 352: * character devices are available to the {@code Console}. Otherwise
> it could
> 353: * mean that the implementation of the {@code Console} may simulate
> those
> 354: * devices within.
The implNote helps as this is a difficult method to specify in a platform
independent way.
The class description uses "interactive command line" and "will typically be
connected to the keyboard and display" and maybe we could re-use that here.
Here's a suggestion for the second paragraph:
"This method returns true if the console device, associated with the current
Java virtual machine, is a terminal, typically an interactive command line
connected to a keyboard and display."
I don't have any other comments on the implementation changes, they look fine.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14271#discussion_r1266938060
Re: RFR: JDK-8309032: jpackage does not work for module projects unless --module-path is specified
On Tue, 11 Jul 2023 23:47:30 GMT, airsquared wrote: > [JDK-8309032](https://bugs.openjdk.org/browse/JDK-8309032), > [JDK-8306488](https://bugs.openjdk.org/browse/JDK-8306488) Marked as reviewed by asemenyuk (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14840#pullrequestreview-1535324178
Integrated: JDK-8310814: Clarify the targetName parameter of Lookup::findClass
On Mon, 26 Jun 2023 21:34:40 GMT, Mandy Chung wrote: > This PR updates the spec of `Lookup::findClass` to reflect the current > behavior that requires a binary name or a string representing an array class > in the form as returned by `Class::getName`. > > `test/jdk/java/lang/invoke/accessClassAndFindClass/TestFindClass.java` > already covers the test cases of a nested class and an array class. This pull request has now been integrated. Changeset: b4dce0d6 Author:Mandy Chung URL: https://git.openjdk.org/jdk/commit/b4dce0d62479c2494c02570a60319cb1a5932940 Stats: 7 lines in 1 file changed: 6 ins; 0 del; 1 mod 8310814: Clarify the targetName parameter of Lookup::findClass Reviewed-by: liach, rriggs, bchristi - PR: https://git.openjdk.org/jdk/pull/14668
Re: RFR: JDK-8310913 Move ReferencedKeyMap to jdk.internal so it may be shared [v5]
On Tue, 18 Jul 2023 15:01:11 GMT, Chen Liang wrote:
>> src/java.base/share/classes/jdk/internal/util/ReferencedKeyMap.java line 132:
>>
>>> 130: */
>>> 131: public static ReferencedKeyMap
>>> 132: create(boolean isSoft, boolean useNativeQueue,
>>> Supplier, V>> supplier) {
>>
>> I suggest to keep the previous `create(boolean isSoft, Supplier supplier)`
>> factory method that defaults to non-native reference queue, which is
>> commonly used. Only `MethodType` needs to use native reference queue.
>
> Mandy: The no-`NativeQueue` version is already implemented above.
Yes Jim made the change per this review feedback - see
[8eff918](https://github.com/openjdk/jdk/pull/14684/commits/8eff91836f98cb33164046f2304185f11df12609)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14684#discussion_r1267020337
[jdk21] RFR: 8310814: Clarify the targetName parameter of Lookup::findClass
Hi all, This pull request contains a backport of commit [b4dce0d6](https://github.com/openjdk/jdk/commit/b4dce0d62479c2494c02570a60319cb1a5932940) from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. The commit being backported was authored by Mandy Chung on 18 Jul 2023 and was reviewed by Chen Liang, Roger Riggs and Brent Christian. Thanks! - Commit messages: - Backport b4dce0d62479c2494c02570a60319cb1a5932940 Changes: https://git.openjdk.org/jdk21/pull/136/files Webrev: https://webrevs.openjdk.org/?repo=jdk21&pr=136&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8310814 Stats: 7 lines in 1 file changed: 6 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk21/pull/136.diff Fetch: git fetch https://git.openjdk.org/jdk21.git pull/136/head:pull/136 PR: https://git.openjdk.org/jdk21/pull/136
Re: [jdk21] RFR: 8310814: Clarify the targetName parameter of Lookup::findClass
On Tue, 18 Jul 2023 16:25:17 GMT, Mandy Chung wrote: > Hi all, > > This pull request contains a backport of commit > [b4dce0d6](https://github.com/openjdk/jdk/commit/b4dce0d62479c2494c02570a60319cb1a5932940) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Mandy Chung on 18 Jul 2023 and > was reviewed by Chen Liang, Roger Riggs and Brent Christian. > > Thanks! Marked as reviewed by darcy (Reviewer). - PR Review: https://git.openjdk.org/jdk21/pull/136#pullrequestreview-1535468097
Re: RFR: 8309622: Re-examine the cache mechanism in BaseLocale [v4]
On Tue, 18 Jul 2023 15:11:04 GMT, Chen Liang wrote: >> 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 14 additional >> commits since the last revision: >> >> - Merge branch 'pull/14684' into JDK-8309622-Cache-BaseLocale >> - Use ReferencedKeyMap in place for LocaleObjectCache >> - Merge branch 'pull/14684' into JDK-8309622-Cache-BaseLocale >> - Addressing review comments >> - Addressing comments (test grouping, synchronization), minor optimization >> on loop lookup >> - minor comment fix >> - equals/hash fix, constructor simplification >> - Removed Key >> - minor fixup >> - Use WeakHashMap >> - ... and 4 more: https://git.openjdk.org/jdk/compare/b7933f62...870ec1fe > > src/java.base/share/classes/sun/util/locale/BaseLocale.java line 167: > >> 165: // can subsequently be used by the Locale instance which >> 166: // guarantees the locale components are properly cased/interned. >> 167: return CACHE.computeIfAbsent(new BaseLocale(language, script, >> region, variant), > > This `computeIfAbsent` will store the non-normalized base locale as a soft > key in the cache, which is undesirable. I have commented on the base patch > https://github.com/openjdk/jdk/pull/14684#discussion_r1266914091 to support > distinct keys for querying and storing like those in the old > `LocalObjectCache`, and we can then simply use a `ReferenceKeySet` for the > cache. The reason I used non-normalized base locale as the key was that normalizing (interning) was slower than comparing the key. I would have to double check, and see if the distinct normalized key is faster. - PR Review Comment: https://git.openjdk.org/jdk/pull/14404#discussion_r1267037551
[jdk21] Integrated: 8310814: Clarify the targetName parameter of Lookup::findClass
On Tue, 18 Jul 2023 16:25:17 GMT, Mandy Chung wrote: > Hi all, > > This pull request contains a backport of commit > [b4dce0d6](https://github.com/openjdk/jdk/commit/b4dce0d62479c2494c02570a60319cb1a5932940) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Mandy Chung on 18 Jul 2023 and > was reviewed by Chen Liang, Roger Riggs and Brent Christian. > > Thanks! This pull request has now been integrated. Changeset: b8eddaba Author:Mandy Chung URL: https://git.openjdk.org/jdk21/commit/b8eddabac0c0e256ad877931cfbe2cdfc1fc79aa Stats: 7 lines in 1 file changed: 6 ins; 0 del; 1 mod 8310814: Clarify the targetName parameter of Lookup::findClass Reviewed-by: darcy Backport-of: b4dce0d62479c2494c02570a60319cb1a5932940 - PR: https://git.openjdk.org/jdk21/pull/136
Re: RFR: 8227229: Deprecate the launcher -Xdebug/-debug flags that have not done anything since Java 6 [v4]
On Tue, 18 Jul 2023 12:41:32 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to deprecate for >> removal the `-Xdebug` option and `-debug` option of the `java` command? >> This addresses https://bugs.openjdk.org/browse/JDK-8227229. >> >> As noted in the JBS issue this option is currently a no-op and has been >> there only for backward compatible since even Java 8 days. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Alan's suggestion for -Xdebug output I think ParseArguments (in libjli/java.c) could be changed so that it doesn't translate -debug to -Xdebug, instead it can print a warning, like it does for -Xfuture. The reason is -debug is a java launcher option, it's not known to the VM and means that Arguments::parse_each_vm_init_arg doesn't need to mention -debug when it warns about -Xdebug. - PR Comment: https://git.openjdk.org/jdk/pull/14918#issuecomment-1640592323
Re: RFR: 8308591: JLine as the default Console provider [v5]
On Tue, 18 Jul 2023 15:21:50 GMT, Alan Bateman wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> `true`
>
> src/java.base/share/classes/java/io/Console.java line 354:
>
>> 352: * character devices are available to the {@code Console}.
>> Otherwise it could
>> 353: * mean that the implementation of the {@code Console} may simulate
>> those
>> 354: * devices within.
>
> The implNote helps as this is a difficult method to specify in a platform
> independent way.
>
> The class description uses "interactive command line" and "will typically be
> connected to the keyboard and display" and maybe we could re-use that here.
> Here's a suggestion for the second paragraph:
>
> "This method returns true if the console device, associated with the current
> Java virtual machine, is a terminal, typically an interactive command line
> connected to a keyboard and display."
>
> I don't have any other comments on the implementation changes, they look fine.
Thanks. Replaced the second paragraph with your suggested sentence.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14271#discussion_r1267070257
Re: RFR: 8308591: JLine as the default Console provider [v6]
> This is to bring the JLine-based Console implementation as the default, which > was the initial intention in > [JDK-8295803](https://bugs.openjdk.org/browse/JDK-8295803). A corresponding > CSR has also been drafted. Naoto Sato has updated the pull request incrementally with two additional commits since the last revision: - @code - Reworded the second paragraph of the method description - Changes: - all: https://git.openjdk.org/jdk/pull/14271/files - new: https://git.openjdk.org/jdk/pull/14271/files/df5805a0..7065f3a5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14271&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14271&range=04-05 Stats: 5 lines in 1 file changed: 0 ins; 2 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/14271.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14271/head:pull/14271 PR: https://git.openjdk.org/jdk/pull/14271
Re: RFR: JDK-8312203: Improve specification of Array.newInstance
On Tue, 18 Jul 2023 05:05:24 GMT, Chen Liang wrote:
>> Change one overload of java.lang.reflect.Array.newInstance to have an
>> `@implSpec` of calling the other method.
>>
>> I choose not to use a snippet tag here is this code is semantically only one
>> line and doesn't need to be cut-and-pasted.
>>
>> As adding an `@implSpec` is technically a (small) specification change,
>> please also review the CSR:
>>
>> https://bugs.openjdk.org/browse/JDK-8312208
>
> src/java.base/share/classes/java/lang/reflect/Array.java line 56:
>
>> 54: * as follows:
>> 55: *
>> 56: * {@code Array.newInstance(componentType, new int[]{length});}
>
> Snippet is not just for cut-and-paste; it can also save us a ``
> tag as well, and syntax highlight support in IDEs.
With the concession that more of the `...`
blocks still in the JDK should be converted to snippets, I don't think that
*all* of them should necessarily be converted.
Having done a few snippet conversions myself (JDK-8287838, JDK-8289399,
JDK-8289775, JDK-8308987), I don't think this particular one is justified as a
snippet.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14917#discussion_r1267096100
Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v26]
On Tue, 18 Jul 2023 01:57:56 GMT, Chen Liang wrote: >> As John Rose has pointed out in this issue, the current j.l.r.Proxy based >> implementation of MethodHandleProxies.asInterface has a few issues: >> 1. Exposes too much information via Proxy supertype (and WrapperInstance >> interface) >> 2. Does not allow future expansion to support SAM[^1] abstract classes >> 3. Slow (in fact, very slow) >> >> This patch addresses all 3 problems: >> 1. It updates the WrapperInstance methods to take an `Empty` to avoid method >> clashes >> 2. This patch obtains already generated classes from a ClassValue by the >> requested interface type; the ClassValue can later be updated to compute >> implementation generation for abstract classes as well. >> 3. This patch's faster than old implementation in general. >> >> Benchmark for revision 17: >> >> Benchmark Mode Cnt >> Score Error Units >> MethodHandleProxiesAsIFInstance.baselineAllocCompute avgt 15 >> 1.503 ± 0.021 ns/op >> MethodHandleProxiesAsIFInstance.baselineComputeavgt 15 >> 0.269 ± 0.005 ns/op >> MethodHandleProxiesAsIFInstance.testCall avgt 15 >> 1.806 ± 0.018 ns/op >> MethodHandleProxiesAsIFInstance.testCreate avgt 15 >> 17.332 ± 0.210 ns/op >> MethodHandleProxiesAsIFInstance.testCreateCall avgt 15 >> 19.296 ± 1.371 ns/op >> MethodHandleProxiesAsIFInstanceCall.callDoable avgt5 >> 0.419 ± 0.004 ns/op >> MethodHandleProxiesAsIFInstanceCall.callHandle avgt5 >> 0.421 ± 0.004 ns/op >> MethodHandleProxiesAsIFInstanceCall.callInterfaceInstance avgt5 >> 1.731 ± 0.018 ns/op >> MethodHandleProxiesAsIFInstanceCall.callLambda avgt5 >> 0.418 ± 0.003 ns/op >> MethodHandleProxiesAsIFInstanceCall.constantDoable avgt5 >> 0.263 ± 0.003 ns/op >> MethodHandleProxiesAsIFInstanceCall.constantHandle avgt5 >> 0.262 ± 0.002 ns/op >> MethodHandleProxiesAsIFInstanceCall.constantInterfaceInstance avgt5 >> 0.262 ± 0.002 ns/op >> MethodHandleProxiesAsIFInstanceCall.constantLambda avgt5 >> 0.267 ± 0.019 ns/op >> MethodHandleProxiesAsIFInstanceCall.direct avgt5 >> 0.266 ± 0.013 ns/op >> MethodHandleProxiesAsIFInstanceCreate.createCallInterfaceInstance avgt5 >> 18.057 ± 0.182 ... > > Chen Liang has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 54 commits: > > - Review class loader check, restore hidden class rejection spec > - Merge branch 'master' into explore/mhp-iface > - Review changees > - Fix the lazy test, thanks Jorn Vernee! > - Clean up impl comment, reorganize tests; weak ref broken > - Merge branch 'master' into explore/mhp-iface > - Fix broken null behaviors > - Merge branch 'master' into explore/mhp-iface > - Merge branch 'mh-proxies' of https://github.com/mlchung/jdk into > explore/mhp-iface > - store a WeakReference holder in the class value > - ... and 44 more: https://git.openjdk.org/jdk/compare/a53345ad...a12fba06 I think applications unlikely depend on the old implementation of dynamic proxies. A release note may serve as a good documentation in case any application observes the performance difference because the hidden classes are GC'ed. > However, I think I might be able to upgrade asInterfaceInstance to accept > protected abstract classes with a single public/protected abstract method(s) > and a no-arg public/protected constructor, like ClassValue. If that's the > case, do we still require a release note for this one, or do we defer and > merge the notes? This is a separate issue and should not combine with this. - PR Comment: https://git.openjdk.org/jdk/pull/13197#issuecomment-1640660342
Re: RFR: 8308591: JLine as the default Console provider [v6]
On Tue, 18 Jul 2023 17:08:34 GMT, Naoto Sato wrote: >> This is to bring the JLine-based Console implementation as the default, >> which was the initial intention in >> [JDK-8295803](https://bugs.openjdk.org/browse/JDK-8295803). A corresponding >> CSR has also been drafted. > > Naoto Sato has updated the pull request incrementally with two additional > commits since the last revision: > > - @code > - Reworded the second paragraph of the method description Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14271#pullrequestreview-1535589025
Re: RFR: JDK-8312203: Improve specification of Array.newInstance
On Tue, 18 Jul 2023 04:42:31 GMT, Joe Darcy wrote: > Change one overload of java.lang.reflect.Array.newInstance to have an > `@implSpec` of calling the other method. > > I choose not to use a snippet tag here is this code is semantically only one > line and doesn't need to be cut-and-pasted. > > As adding an `@implSpec` is technically a (small) specification change, > please also review the CSR: > > https://bugs.openjdk.org/browse/JDK-8312208 CSR and PR look fine. - Marked as reviewed by bpb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14917#pullrequestreview-1535584529
Re: RFR: JDK-8312203: Improve specification of Array.newInstance
On Tue, 18 Jul 2023 04:42:31 GMT, Joe Darcy wrote: > Change one overload of java.lang.reflect.Array.newInstance to have an > `@implSpec` of calling the other method. > > I choose not to use a snippet tag here is this code is semantically only one > line and doesn't need to be cut-and-pasted. > > As adding an `@implSpec` is technically a (small) specification change, > please also review the CSR: > > https://bugs.openjdk.org/browse/JDK-8312208 Marked as reviewed by mchung (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14917#pullrequestreview-1535607485
Re: RFR: 8311631: When multiple users run tools/jpackage/share/LicenseTest.java, Permission denied for writing /var/tmp/*.files [v2]
On Thu, 13 Jul 2023 07:27:22 GMT, yaqsun wrote: >> The prerequisite is to install the rpmbuild command, when multiple users >> switch to write /var/tmp/*.files will have the permission forbidden. > > yaqsun has updated the pull request incrementally with one additional commit > since the last revision: > > 8311631: When multiple users run tools/jpackage/share/LicenseTest.java, > Permission denied for writing /var/tmp/*.files Marked as reviewed by almatvee (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14802#pullrequestreview-1535814977
Re: RFR: 8039165: [Doc] MessageFormat null locale generates NullPointerException [v2]
> Please review this PR and [CSR](https://bugs.openjdk.org/browse/JDK-8312197) > which updates the javadoc for the constructor of MessageFormat regarding a > `null` locale, > > `MessageFormat` when created with a `null` locale may throw a > `NullPointerException` either during the object creation, or later when > `format()` is called by the `MessageFormat` object (test file has examples of > both). This change updates the specification of MessageFormat to make this > apparent. The wording is specifically chosen as 'may throw' since whether an > NPE is thrown depends on the subformat used by MessageFormat (see test > example of construction with null locale and no exception thrown). > > The test for this change was merged with `Bug6481179.java` into > `MessageFormatExceptions.java` (As they both test exceptions). In addition, > some other exception testing regarding MessageFormat was added. > > Thanks Justin Lu has updated the pull request incrementally with two additional commits since the last revision: - Slight wording adjustment - Review: Explicitly declare when NPE thrown instead of 'may' - Changes: - all: https://git.openjdk.org/jdk/pull/14911/files - new: https://git.openjdk.org/jdk/pull/14911/files/05ac9193..4092e3ad Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14911&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14911&range=00-01 Stats: 14 lines in 2 files changed: 10 ins; 1 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/14911.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14911/head:pull/14911 PR: https://git.openjdk.org/jdk/pull/14911
Re: RFR: 8039165: [Doc] MessageFormat null locale generates NullPointerException [v2]
On Mon, 17 Jul 2023 21:54:37 GMT, Naoto Sato wrote:
>> Justin Lu has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Slight wording adjustment
>> - Review: Explicitly declare when NPE thrown instead of 'may'
>
> src/java.base/share/classes/java/text/MessageFormat.java line 396:
>
>> 394: * {@code NullPointerException} if {@code locale} is {@code null}
>> 395: * either during the creation of the {@code MessageFormat} object
>> or later
>> 396: * when {@code format()} is called by the constructed {@code
>> MessageFormat} object.
>
> It looks a bit vague as it contains `may`. I think it would be clearer if it
> explains a bit more, e.g., it throws NPE if any of the subformats require the
> locale, or along with those lines. Probably the same wording is needed at
> `format()` and other public methods that use the `locale` field.
The only other public methods that use `locale` are setLocale(), getLocale(),
toString(), and equals(). In all of these cases I don't believe if a
MessageFormat that was created with a null locale (and did not throw NPE) calls
them, a NPE will occur in any case.
Thus I have updated the spec to not use 'may' and explicitly state NPE is
thrown if a subformat is localized and needed by the MessageFormat.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14911#discussion_r1267298674
Re: RFR: 8039165: [Doc] MessageFormat null locale generates NullPointerException [v2]
On Tue, 18 Jul 2023 20:48:13 GMT, Justin Lu wrote:
>> Please review this PR and [CSR](https://bugs.openjdk.org/browse/JDK-8312197)
>> which updates the javadoc for the constructor of MessageFormat regarding a
>> `null` locale,
>>
>> `MessageFormat` when created with a `null` locale may throw a
>> `NullPointerException` either during the object creation, or later when
>> `format()` is called by the `MessageFormat` object (test file has examples
>> of both). This change updates the specification of MessageFormat to make
>> this apparent. The wording is specifically chosen as 'may throw' since
>> whether an NPE is thrown depends on the subformat used by MessageFormat (see
>> test example of construction with null locale and no exception thrown).
>>
>> The test for this change was merged with `Bug6481179.java` into
>> `MessageFormatExceptions.java` (As they both test exceptions). In addition,
>> some other exception testing regarding MessageFormat was added.
>>
>> Thanks
>
> Justin Lu has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Slight wording adjustment
> - Review: Explicitly declare when NPE thrown instead of 'may'
src/java.base/share/classes/java/text/MessageFormat.java line 398:
> 396: * when {@code format()} is called by the constructed {@code
> MessageFormat}
> 397: * instance and the implementation utilizes a subformat that requires
> 398: * localization.
Sorry for the nitpick, but since localization itself is not a requirement,
`locale-dependent subformat` would read better to me. (and wherever `localized`
is used)
src/java.base/share/classes/java/text/MessageFormat.java line 407:
> 405: * @throwsNullPointerException This method throws a
> 406: *{@code NullPointerException} if {@code locale} is
> {@code null}
> 407: *and a localized subformat is used.
NPE throws tags should be combined. Applies to other locations too.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14911#discussion_r1267346080
PR Review Comment: https://git.openjdk.org/jdk/pull/14911#discussion_r1267347900
Integrated: JDK-8312203: Improve specification of Array.newInstance
On Tue, 18 Jul 2023 04:42:31 GMT, Joe Darcy wrote: > Change one overload of java.lang.reflect.Array.newInstance to have an > `@implSpec` of calling the other method. > > I choose not to use a snippet tag here is this code is semantically only one > line and doesn't need to be cut-and-pasted. > > As adding an `@implSpec` is technically a (small) specification change, > please also review the CSR: > > https://bugs.openjdk.org/browse/JDK-8312208 This pull request has now been integrated. Changeset: e5ecbff6 Author:Joe Darcy URL: https://git.openjdk.org/jdk/commit/e5ecbff69eeb83abbe70421b7f1540a5c382441a Stats: 6 lines in 1 file changed: 1 ins; 3 del; 2 mod 8312203: Improve specification of Array.newInstance Reviewed-by: bpb, mchung - PR: https://git.openjdk.org/jdk/pull/14917
RFR: 8199149: Improve the exception message thrown by VarHandle of unsupported operation
`VarForm::getMemberName` currently throws UOE with no information if the requested access mode is unsupported. To provide the var handle information, move the access mode check to `VarHandle` so that the exception message can include the var handle information. Changes include: 1. change `VarHandle::checkAccessModeThenIsDirect` to check if the access mode is unsupported. This check is only needed for direct var handle. 2. change `VarHandle::getMethodHandleUncached` to call `getMemberNameOrNull` and throw UOE with an informative exception message if the access mode is unsupported The error message looks like: java.lang.UnsupportedOperationException: compareAndSet is not supported for VarHandle[varType=java.lang.String, coord=[class Foo$Goo]] - Commit messages: - 8199149: Improve the exception message thrown by VarHandle of unsupported operation Changes: https://git.openjdk.org/jdk/pull/14928/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14928&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8199149 Stats: 35 lines in 3 files changed: 31 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/14928.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14928/head:pull/14928 PR: https://git.openjdk.org/jdk/pull/14928
Re: RFR: 8199149: Improve the exception message thrown by VarHandle of unsupported operation
On Wed, 19 Jul 2023 00:12:53 GMT, Mandy Chung wrote:
> `VarForm::getMemberName` currently throws UOE with no information if the
> requested access mode is unsupported. To provide the var handle
> information, move the access mode check to `VarHandle` so that the exception
> message can include the var handle information. Changes include:
>
> 1. change `VarHandle::checkAccessModeThenIsDirect` to check if the access
> mode is unsupported. This check is only needed for direct var handle.
> 2. change `VarHandle::getMethodHandleUncached` to call `getMemberNameOrNull`
> and throw UOE with an informative exception message if the access mode is
> unsupported
>
> The error message looks like:
>
> java.lang.UnsupportedOperationException: compareAndSet is not supported for
> VarHandle[varType=java.lang.String, coord=[class Foo$Goo]]
src/java.base/share/classes/java/lang/invoke/IndirectVarHandle.java line 93:
> 91: @ForceInline
> 92: boolean checkAccessModeThenIsDirect(VarHandle.AccessDescriptor ad) {
> 93: if (exact && accessModeType(ad.type) !=
> ad.symbolicMethodTypeExact) {
Can we add a comment that the detailed UOE is thrown via
`directTarget.getMethodHandleUncached`?
src/java.base/share/classes/java/lang/invoke/VarHandle.java line 2020:
> 2018:
> 2019: static AccessMode valueFromOrdinal(int mode) {
> 2020: return modeToAccessMode.get(mode);
Can't this be simplified to `AccessMode.values()[mode]` since this is only
rarely used in the exception logic?
If we do need a cache, I would recommend a stable array like
private static final @Stable AccessMode[] VALUES = values();
and users call this accessor instead.
The code in `getMethodHandleUncached` can benefit from this caching mechanism.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14928#discussion_r1267424106
PR Review Comment: https://git.openjdk.org/jdk/pull/14928#discussion_r1267422748
Re: RFR: 8199149: Improve the exception message thrown by VarHandle of unsupported operation [v2]
On Wed, 19 Jul 2023 00:52:49 GMT, Chen Liang wrote:
>> Mandy Chung has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> review feedback
>
> src/java.base/share/classes/java/lang/invoke/IndirectVarHandle.java line 93:
>
>> 91: @ForceInline
>> 92: boolean checkAccessModeThenIsDirect(VarHandle.AccessDescriptor ad) {
>> 93: if (exact && accessModeType(ad.type) !=
>> ad.symbolicMethodTypeExact) {
>
> Can we add a comment that the detailed UOE is thrown via
> `directTarget.getMethodHandleUncached`?
Actually `VarHandle::checkAccessModeThenIsDirect` can call
`isAccessModeSupported` so that this will check properly for
`IndirectVarHandle`.
> src/java.base/share/classes/java/lang/invoke/VarHandle.java line 2020:
>
>> 2018:
>> 2019: static AccessMode valueFromOrdinal(int mode) {
>> 2020: return modeToAccessMode.get(mode);
>
> Can't this be simplified to `AccessMode.values()[mode]` since this is only
> rarely used in the exception logic?
>
> If we do need a cache, I would recommend a stable array like
>
> private static final @Stable AccessMode[] VALUES = values();
>
> and users call this accessor instead.
>
> The code in `getMethodHandleUncached` can benefit from this caching mechanism.
Good suggestion.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14928#discussion_r1267459610
PR Review Comment: https://git.openjdk.org/jdk/pull/14928#discussion_r1267459744
Re: RFR: 8199149: Improve the exception message thrown by VarHandle of unsupported operation [v2]
> `VarForm::getMemberName` currently throws UOE with no information if the > requested access mode is unsupported. To provide the var handle > information, move the access mode check to `VarHandle` so that the exception > message can include the var handle information. Changes include: > > 1. change `VarHandle::checkAccessModeThenIsDirect` to check if the access > mode is unsupported. This check is only needed for direct var handle. > 2. change `VarHandle::getMethodHandleUncached` to call `getMemberNameOrNull` > and throw UOE with an informative exception message if the access mode is > unsupported > > The error message looks like: > > java.lang.UnsupportedOperationException: compareAndSet is not supported for > VarHandle[varType=java.lang.String, coord=[class Foo$Goo]] Mandy Chung has updated the pull request incrementally with one additional commit since the last revision: review feedback - Changes: - all: https://git.openjdk.org/jdk/pull/14928/files - new: https://git.openjdk.org/jdk/pull/14928/files/cc66b08c..176df233 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14928&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14928&range=00-01 Stats: 15 lines in 2 files changed: 0 ins; 11 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/14928.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14928/head:pull/14928 PR: https://git.openjdk.org/jdk/pull/14928
Re: RFR: 8199149: Improve the exception message thrown by VarHandle of unsupported operation [v2]
On Wed, 19 Jul 2023 02:19:58 GMT, Mandy Chung wrote: >> `VarForm::getMemberName` currently throws UOE with no information if the >> requested access mode is unsupported. To provide the var handle >> information, move the access mode check to `VarHandle` so that the exception >> message can include the var handle information. Changes include: >> >> 1. change `VarHandle::checkAccessModeThenIsDirect` to check if the access >> mode is unsupported. This check is only needed for direct var handle. >> 2. change `VarHandle::getMethodHandleUncached` to call `getMemberNameOrNull` >> and throw UOE with an informative exception message if the access mode is >> unsupported >> >> The error message looks like: >> >> java.lang.UnsupportedOperationException: compareAndSet is not supported for >> VarHandle[varType=java.lang.String, coord=[class Foo$Goo]] > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > review feedback Marked as reviewed by liach (Author). - PR Review: https://git.openjdk.org/jdk/pull/14928#pullrequestreview-1536143735
Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v26]
On Tue, 18 Jul 2023 01:57:56 GMT, Chen Liang wrote: >> As John Rose has pointed out in this issue, the current j.l.r.Proxy based >> implementation of MethodHandleProxies.asInterface has a few issues: >> 1. Exposes too much information via Proxy supertype (and WrapperInstance >> interface) >> 2. Does not allow future expansion to support SAM[^1] abstract classes >> 3. Slow (in fact, very slow) >> >> This patch addresses all 3 problems: >> 1. It updates the WrapperInstance methods to take an `Empty` to avoid method >> clashes >> 2. This patch obtains already generated classes from a ClassValue by the >> requested interface type; the ClassValue can later be updated to compute >> implementation generation for abstract classes as well. >> 3. This patch's faster than old implementation in general. >> >> Benchmark for revision 17: >> >> Benchmark Mode Cnt >> Score Error Units >> MethodHandleProxiesAsIFInstance.baselineAllocCompute avgt 15 >> 1.503 ± 0.021 ns/op >> MethodHandleProxiesAsIFInstance.baselineComputeavgt 15 >> 0.269 ± 0.005 ns/op >> MethodHandleProxiesAsIFInstance.testCall avgt 15 >> 1.806 ± 0.018 ns/op >> MethodHandleProxiesAsIFInstance.testCreate avgt 15 >> 17.332 ± 0.210 ns/op >> MethodHandleProxiesAsIFInstance.testCreateCall avgt 15 >> 19.296 ± 1.371 ns/op >> MethodHandleProxiesAsIFInstanceCall.callDoable avgt5 >> 0.419 ± 0.004 ns/op >> MethodHandleProxiesAsIFInstanceCall.callHandle avgt5 >> 0.421 ± 0.004 ns/op >> MethodHandleProxiesAsIFInstanceCall.callInterfaceInstance avgt5 >> 1.731 ± 0.018 ns/op >> MethodHandleProxiesAsIFInstanceCall.callLambda avgt5 >> 0.418 ± 0.003 ns/op >> MethodHandleProxiesAsIFInstanceCall.constantDoable avgt5 >> 0.263 ± 0.003 ns/op >> MethodHandleProxiesAsIFInstanceCall.constantHandle avgt5 >> 0.262 ± 0.002 ns/op >> MethodHandleProxiesAsIFInstanceCall.constantInterfaceInstance avgt5 >> 0.262 ± 0.002 ns/op >> MethodHandleProxiesAsIFInstanceCall.constantLambda avgt5 >> 0.267 ± 0.019 ns/op >> MethodHandleProxiesAsIFInstanceCall.direct avgt5 >> 0.266 ± 0.013 ns/op >> MethodHandleProxiesAsIFInstanceCreate.createCallInterfaceInstance avgt5 >> 18.057 ± 0.182 ... > > Chen Liang has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 54 commits: > > - Review class loader check, restore hidden class rejection spec > - Merge branch 'master' into explore/mhp-iface > - Review changees > - Fix the lazy test, thanks Jorn Vernee! > - Clean up impl comment, reorganize tests; weak ref broken > - Merge branch 'master' into explore/mhp-iface > - Fix broken null behaviors > - Merge branch 'master' into explore/mhp-iface > - Merge branch 'mh-proxies' of https://github.com/mlchung/jdk into > explore/mhp-iface > - store a WeakReference holder in the class value > - ... and 44 more: https://git.openjdk.org/jdk/compare/a53345ad...a12fba06 I have made a release note documenting the potential performance impact: https://bugs.openjdk.org/browse/JDK-8312331 Please help review and revise. Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/13197#issuecomment-1641300644
