Re: RFR: 8273000: Remove WeakReference-based class initialisation barrier implementation [v2]
On Wed, 1 Sep 2021 18:28:30 GMT, Paul Sandoz wrote:
>> Vladimir Ivanov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Improve comments
>
> src/java.base/share/classes/jdk/internal/misc/Unsafe.java line 1152:
>
>> 1150: * The call returns when either class {@code c} is fully
>> initialized or
>> 1151: * class {@code c} is being initialized and the call is performed
>> from
>> 1152: * the initializing thread.
>
> Suggestion:
>
> * The call returns when either class {@code c} is fully initialized or
> * class {@code c} is being initialized and the call is performed from
> * the initializing thread. In the latter case a subsequent call to
> * {@link #shouldBeInitialized}, from the calling thread of this call,
> * will return {@code true}.
Aren't "the calling thread of this call" and "the initializing thread" the same
thread in the latter case?
-
PR: https://git.openjdk.java.net/jdk/pull/5258
Re: RFR: 8272600: (test) Use native "sleep" in Basic.java [v3]
On Sat, 28 Aug 2021 02:34:48 GMT, Roger Riggs wrote: >> The intermittent test in java/lang/ProcessBuilder/Basic.java has identified >> unexpected messages from a child Java VM >> as the cause of the test failure. Attempts to control the output of the >> child VM have failed, the VM is unrepentant . >> >> There is no functionality in the child except to wait long enough for the >> test to finish and the child is destroyed. >> The fix is to switch from using a Java child to using a native child; a new >> executable `sleepmillis`. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Revised to use native /bin/sleep program on Unix* (non-Windows). > For Windows, a native "BasicSleep" program is used. BTW if running the tests on Windows under Cygwin etc then it should find /usr/bin/sleep anyway. - PR: https://git.openjdk.java.net/jdk/pull/5239
Re: RFR: 8272600: (test) Use native "sleep" in Basic.java [v3]
On Wed, 1 Sep 2021 15:12:16 GMT, Roger Riggs wrote: > I've had very inconsistent results using the Windows 'timeout' command. > There appear to multiple versions. Some give an error message "Type "TIMEOUT > /?" for usage.", (Windows 10) > others a error message "Try 'timeout --help' for more information." (Windows > 2016) > And in some cases, it does not appear to wait the entire time requested and > one of the test fails. The `timeout --help` does not appear to be the Windows command, but a bash variant so I'm assuming you were running under Cygwin or some such environment? Note it is completely different to the Windows timeout command and not what you want. - PR: https://git.openjdk.java.net/jdk/pull/5239
Re: RFR: 8272600: (test) Use native "sleep" in Basic.java [v3]
On Wed, 1 Sep 2021 15:02:44 GMT, Roger Riggs wrote:
>> test/jdk/java/lang/ProcessBuilder/Basic.java line 2665:
>>
>>> 2663:
>>> 2664: Path exePath =
>>> Path.of(TEST_NATIVEPATH).resolve("BasicSleep.exe");
>>> 2665: System.out.println("exePath: " + exePath + ", canExec: "
>>> + exePath.toFile().canExecute());
>>
>> What is this for??
>
> Ioi was concerned that the fallback to Java would be used silently and the
> old intermittent issue would return.
Well Ioi asked for a comment explaining why the fallback was necessary, but the
println doesn't explain anything. I assume it should say something like "Unable
to find ... falling back to Java version"?
-
PR: https://git.openjdk.java.net/jdk/pull/5239
Re: RFR: 8273250: Address javadoc issues in Deflater::setDictionationary
On Wed, 1 Sep 2021 19:26:17 GMT, Lance Andersen wrote: > Hi, > > Please review this trivial fix to the javadoc which addresses an issue shown > via Intellij where the error: "Symbol 'getAdler' is inaccessible from here" > is generated for the "@See Inflater#getAlder" references. > > Best > Lance Marked as reviewed by bpb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5340
RFR: JDK-8273194: Document the two possible cases when Lookup::ensureInitialized returns
Improve the specification to document the cases when `Lookup::ensureInitialized` returns as specified JVMS 5.5 and matches the implementation. Please also review CSR: https://bugs.openjdk.java.net/browse/JDK-8273253 - Commit messages: - JDK-8273194: Document the two possible cases when Lookup::ensureInitialized returns Changes: https://git.openjdk.java.net/jdk/pull/5343/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5343=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8273194 Stats: 7 lines in 1 file changed: 6 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5343.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5343/head:pull/5343 PR: https://git.openjdk.java.net/jdk/pull/5343
RFR: 8271602: Add Math.ceilDiv() family parallel to Math.floorDiv() family
This PR ideally continues #5285, which has been closed as a consequence of inadvertently removing the branch on my repo. See there for initial discussion. Sorry for the mess. - Commit messages: - 8271602: Add Math.ceilDiv() family parallel to Math.floorDiv() family Changes: https://git.openjdk.java.net/jdk/pull/5341/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5341=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8271602 Stats: 1093 lines in 4 files changed: 1040 ins; 0 del; 53 mod Patch: https://git.openjdk.java.net/jdk/pull/5341.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5341/head:pull/5341 PR: https://git.openjdk.java.net/jdk/pull/5341
Re: RFR: 8273250: Address javadoc issues in Deflater::setDictionationary
On Wed, 1 Sep 2021 19:26:17 GMT, Lance Andersen wrote: > Hi, > > Please review this trivial fix to the javadoc which addresses an issue shown > via Intellij where the error: "Symbol 'getAdler' is inaccessible from here" > is generated for the "@See Inflater#getAlder" references. > > Best > Lance Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5340
Re: RFR: 8273250: Address javadoc issues in Deflater::setDictionationary
On Wed, 1 Sep 2021 19:26:17 GMT, Lance Andersen wrote: > Hi, > > Please review this trivial fix to the javadoc which addresses an issue shown > via Intellij where the error: "Symbol 'getAdler' is inaccessible from here" > is generated for the "@See Inflater#getAlder" references. > > Best > Lance Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5340
Re: RFR: 8273250: Address javadoc issues in Deflater::setDictionationary
On Wed, 1 Sep 2021 19:26:17 GMT, Lance Andersen wrote: > Hi, > > Please review this trivial fix to the javadoc which addresses an issue shown > via Intellij where the error: "Symbol 'getAdler' is inaccessible from here" > is generated for the "@See Inflater#getAlder" references. > > Best > Lance Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5340
Re: RFR: 8214761: Bug in parallel Kahan summation implementation [v4]
> 8214761: Bug in parallel Kahan summation implementation Ian Graves has updated the pull request incrementally with one additional commit since the last revision: Fixing compensation test - Changes: - all: https://git.openjdk.java.net/jdk/pull/4674/files - new: https://git.openjdk.java.net/jdk/pull/4674/files/905450d8..722826fc Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4674=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4674=02-03 Stats: 13 lines in 1 file changed: 5 ins; 4 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/4674.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4674/head:pull/4674 PR: https://git.openjdk.java.net/jdk/pull/4674
RFR: 8273250: Address javadoc issues in Deflater::setDictionationary
Hi, Please review this trivial fix to the javadoc which addresses an issue shown via Intellij where the error: "Symbol 'getAdler' is inaccessible from here" is generated for the "@See Inflater#getAlder" references. Best Lance - Commit messages: - Address JDK-8273250 Changes: https://git.openjdk.java.net/jdk/pull/5340/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5340=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8273250 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/5340.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5340/head:pull/5340 PR: https://git.openjdk.java.net/jdk/pull/5340
Re: RFR: 8273000: Remove WeakReference-based class initialisation barrier implementation [v2]
On Wed, 1 Sep 2021 16:16:38 GMT, Vladimir Ivanov wrote:
>> Get rid of WeakReference-based logic in
>> DirectMethodHandle::checkInitialized() and reimplement it with
>> `Unsafe::ensureClassInitialized()`/`shouldBeInitialized()`.
>>
>> The key observation is that `Unsafe::ensureClassInitialized()` does not
>> block the initializing thread.
>>
>> Also, removed `Unsafe::shouldBeInitialized()` in
>> `DMH::shouldBeInitialized(MemberName)` to save on calling into the VM.
>> `Unsafe::ensureClassInitialized()` already has a fast-path check which
>> checks whether the class is fully initialized or not.
>>
>> Testing: tier1 - tier6
>
> Vladimir Ivanov has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Improve comments
Looks good, just a minor suggestion up to you to accept or not.
src/java.base/share/classes/jdk/internal/misc/Unsafe.java line 1152:
> 1150: * The call returns when either class {@code c} is fully
> initialized or
> 1151: * class {@code c} is being initialized and the call is performed
> from
> 1152: * the initializing thread.
Suggestion:
* The call returns when either class {@code c} is fully initialized or
* class {@code c} is being initialized and the call is performed from
* the initializing thread. In the latter case a subsequent call to
* {@link #shouldBeInitialized}, from the calling thread of this call,
* will return {@code true}.
-
Marked as reviewed by psandoz (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/5258
Withdrawn: 8271602: Add Math.ceilDiv() family parallel to Math.floorDiv() family
On Fri, 27 Aug 2021 18:53:23 GMT, Raffaello Giulietti wrote: > Please review this PR to add officially endorsed `ceilDiv()` and `ceilMod()` > methods do `j.l.[Strict]Math`. > > Beside adding fresh tests to `test/jdk/java/lang/Math/DivModTests.java`, this > PR also corrects small typos in it and exercises tests that were already > present but which were never invoked. > Let me know if this is acceptable for a test or if there's a need of a > separate issue in the JBS. This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/5285
Re: RFR: 8273248: ProblemList java/lang/instrument/BootClassPath/BootClassPathTest.sh on all configs
On Wed, 1 Sep 2021 17:35:12 GMT, Naoto Sato wrote: >> A trivial fix to ProblemList >> java/lang/instrument/BootClassPath/BootClassPathTest.sh on all configs. > > Marked as reviewed by naoto (Reviewer). @naotoj - Thanks for the review! - PR: https://git.openjdk.java.net/jdk/pull/5337
Integrated: 8273248: ProblemList java/lang/instrument/BootClassPath/BootClassPathTest.sh on all configs
On Wed, 1 Sep 2021 17:33:13 GMT, Daniel D. Daugherty wrote: > A trivial fix to ProblemList > java/lang/instrument/BootClassPath/BootClassPathTest.sh on all configs. This pull request has now been integrated. Changeset: 4ee0dace Author:Daniel D. Daugherty URL: https://git.openjdk.java.net/jdk/commit/4ee0dacecd5afc5876ea839ffbb5df962ff6cd08 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8273248: ProblemList java/lang/instrument/BootClassPath/BootClassPathTest.sh on all configs Reviewed-by: naoto - PR: https://git.openjdk.java.net/jdk/pull/5337
Re: Integrated: 8273197: ProblemList 2 jtools tests due to JDK-8273187
On Tue, 31 Aug 2021 20:02:37 GMT, Naoto Sato wrote: >> Trivial fixes to reduce the noise in the JDK18 CI: >> JDK-8273197 ProblemList 2 jtools tests due to JDK-8273187 >> JDK-8273198 ProblemList >> java/lang/instrument/BootClassPath/BootClassPathTest.sh due to JDK-8273188 >> >> These failures happen in Tier5 so I'm ProblemListing them now to give >> @naotoj time to >> work on the issues introduced by JDK-8260265 UTF-8 by Default. > > Marked as reviewed by naoto (Reviewer). @naotoj - Thanks for the review! (I forgot to post yesterday) - PR: https://git.openjdk.java.net/jdk/pull/5321
Re: RFR: 8273248: ProblemList java/lang/instrument/BootClassPath/BootClassPathTest.sh on all configs
On Wed, 1 Sep 2021 17:33:13 GMT, Daniel D. Daugherty wrote: > A trivial fix to ProblemList > java/lang/instrument/BootClassPath/BootClassPathTest.sh on all configs. Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5337
RFR: 8273248: ProblemList java/lang/instrument/BootClassPath/BootClassPathTest.sh on all configs
A trivial fix to ProblemList java/lang/instrument/BootClassPath/BootClassPathTest.sh on all configs. - Commit messages: - 8273248: ProblemList java/lang/instrument/BootClassPath/BootClassPathTest.sh on all configs Changes: https://git.openjdk.java.net/jdk/pull/5337/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5337=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8273248 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5337.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5337/head:pull/5337 PR: https://git.openjdk.java.net/jdk/pull/5337
Re: RFR: 8078641: MethodHandle.asTypeCache can retain classes from unloading [v2]
On Wed, 1 Sep 2021 16:23:46 GMT, Vladimir Ivanov wrote:
>> `MethodHandle.asTypeCache` keeps a strong reference to adapted
>> `MethodHandle` and it can introduce a class loader leak through its
>> `MethodType`.
>>
>> Proposed fix introduces a 2-level cache (1 element each) where 1st level can
>> only contain `MethodHandle`s which are guaranteed to not introduce any
>> dependencies on new class loaders compared to the original `MethodHandle`.
>> 2nd level is backed by a `SoftReference` and is used as a backup when the
>> result of `MethodHandle.asType()` conversion can't populate the higher level
>> cache.
>>
>> The fix is based on [the
>> work](http://cr.openjdk.java.net/~plevart/jdk9-dev/MethodHandle.asTypeCacheLeak/)
>> made by Peter Levart @plevart back in 2015.
>>
>> Testing: tier1 - tier6
>
> Vladimir Ivanov has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Address review comments
src/java.base/share/classes/java/lang/invoke/MethodHandle.java line 926:
> 924: /* Returns true when {@code loader} keeps {@code cls} either
> directly or indirectly through the loader delegation chain. */
> 925: private static boolean keepsAlive(Class cls, ClassLoader loader) {
> 926: return keepsAlive(cls.getClassLoader(), loader);
Suggestion:
ClassLoader defLoader = cls.getClassLoader();
if (isBuiltinLoader(defLoader)) {
return true; // built-in loaders are always reachable
}
return keepsAlive(defLoader, loader);
I think it's clearer to check if `cls` is not defined by any builtin loader
here and then check if `loader` keeps `cls` alive.
So `keepsAlive(ClassLoader loader1, ClassLoader loader2)` is not needed and
replace line 935 and 940 to `keepsAlive(Class, ClassLoader)` instead.
-
PR: https://git.openjdk.java.net/jdk/pull/5246
Re: RFR: 8272137: Make Collection and Optional classes streamable
On Mon, 9 Aug 2021 12:28:23 GMT, CC007 wrote: > create Streamable and ParallelStreamable interface and use them in Collection > and Optional > _Mailing list message from [Alan Snyder](mailto:javali...@cbfiddle.com) on > [core-libs-dev](mailto:core-libs-...@mail.openjdk.java.net):_ > > Ah, if only one could define a type alias Streamable = > Supplier>... If you use that, you'd lose some semantics that the method name would have given you. I feel that @liach 's solution is cleaner, since that preserves this method name. - PR: https://git.openjdk.java.net/jdk/pull/5050
RFR: 8273242: (test) Refactor to use TestNG for RuntimeTests ExecCommand tests
The ExecCommand test of Runtime.exec is difficult to maintain; the parallel arrays are hard to keep in sync. This cleanup converts to use TestNG DataProviders and other improvements. - Commit messages: - 8273242: Refactored ExecCommand to use TestNG DataProvider for cases Changes: https://git.openjdk.java.net/jdk/pull/5335/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5335=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8273242 Stats: 270 lines in 1 file changed: 137 ins; 97 del; 36 mod Patch: https://git.openjdk.java.net/jdk/pull/5335.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5335/head:pull/5335 PR: https://git.openjdk.java.net/jdk/pull/5335
Re: RFR: 8078641: MethodHandle.asTypeCache can retain classes from unloading
On Wed, 25 Aug 2021 09:31:51 GMT, Vladimir Ivanov wrote: > `MethodHandle.asTypeCache` keeps a strong reference to adapted `MethodHandle` > and it can introduce a class loader leak through its `MethodType`. > > Proposed fix introduces a 2-level cache (1 element each) where 1st level can > only contain `MethodHandle`s which are guaranteed to not introduce any > dependencies on new class loaders compared to the original `MethodHandle`. > 2nd level is backed by a `SoftReference` and is used as a backup when the > result of `MethodHandle.asType()` conversion can't populate the higher level > cache. > > The fix is based on [the > work](http://cr.openjdk.java.net/~plevart/jdk9-dev/MethodHandle.asTypeCacheLeak/) > made by Peter Levart @plevart back in 2015. > > Testing: tier1 - tier6 Thanks for the reviews, Paul and Mandy. What do you think about the latest version? - PR: https://git.openjdk.java.net/jdk/pull/5246
Re: RFR: 8078641: MethodHandle.asTypeCache can retain classes from unloading [v2]
> `MethodHandle.asTypeCache` keeps a strong reference to adapted `MethodHandle` > and it can introduce a class loader leak through its `MethodType`. > > Proposed fix introduces a 2-level cache (1 element each) where 1st level can > only contain `MethodHandle`s which are guaranteed to not introduce any > dependencies on new class loaders compared to the original `MethodHandle`. > 2nd level is backed by a `SoftReference` and is used as a backup when the > result of `MethodHandle.asType()` conversion can't populate the higher level > cache. > > The fix is based on [the > work](http://cr.openjdk.java.net/~plevart/jdk9-dev/MethodHandle.asTypeCacheLeak/) > made by Peter Levart @plevart back in 2015. > > Testing: tier1 - tier6 Vladimir Ivanov has updated the pull request incrementally with one additional commit since the last revision: Address review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/5246/files - new: https://git.openjdk.java.net/jdk/pull/5246/files/87154817..b2c0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5246=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5246=00-01 Stats: 13 lines in 1 file changed: 4 ins; 1 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/5246.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5246/head:pull/5246 PR: https://git.openjdk.java.net/jdk/pull/5246
Re: RFR: 8273000: Remove WeakReference-based class initialisation barrier implementation [v2]
On Thu, 26 Aug 2021 23:49:07 GMT, David Holmes wrote: >> Vladimir Ivanov has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Improve comments > > src/java.base/share/classes/java/lang/invoke/DirectMethodHandle.java line 385: > >> 383: Class defc = member.getDeclaringClass(); >> 384: UNSAFE.ensureClassInitialized(defc); // initialization barrier; >> blocks unless called by the initializing thread >> 385: return !UNSAFE.shouldBeInitialized(defc); // keep the barrier >> if the initialization is still in progress > > I think some more elaborate commentary about the possibility of this being > called while of defc is already on the call stack, would be > worthwhile - the existing comments are a little too subtle IMO. > > > UNSAFE.ensureClassInitialized(defc); > // Once we get here either defc was fully initialized by another thread, or > // defc was already being initialized by the current thread. In the latter > case > // the barrier must remain. We can detect this simply by checking if > initialization > // is still needed. > return !UNSAFE.shouldBeInitialized(defc); Thanks, David. I incorporated your suggestion in the latest version. - PR: https://git.openjdk.java.net/jdk/pull/5258
Re: RFR: 8273000: Remove WeakReference-based class initialisation barrier implementation [v2]
On Tue, 31 Aug 2021 16:36:58 GMT, Mandy Chung wrote: > May i suggest that we add some JavaDoc to ensureClassInitialized Thanks, Paul. How does the latest version look? - PR: https://git.openjdk.java.net/jdk/pull/5258
Re: RFR: 8273000: Remove WeakReference-based class initialisation barrier implementation [v2]
> Get rid of WeakReference-based logic in > DirectMethodHandle::checkInitialized() and reimplement it with > `Unsafe::ensureClassInitialized()`/`shouldBeInitialized()`. > > The key observation is that `Unsafe::ensureClassInitialized()` does not block > the initializing thread. > > Also, removed `Unsafe::shouldBeInitialized()` in > `DMH::shouldBeInitialized(MemberName)` to save on calling into the VM. > `Unsafe::ensureClassInitialized()` already has a fast-path check which checks > whether the class is fully initialized or not. > > Testing: tier1 - tier6 Vladimir Ivanov has updated the pull request incrementally with one additional commit since the last revision: Improve comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/5258/files - new: https://git.openjdk.java.net/jdk/pull/5258/files/9fc92167..64f2de83 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5258=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5258=00-01 Stats: 13 lines in 2 files changed: 8 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/5258.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5258/head:pull/5258 PR: https://git.openjdk.java.net/jdk/pull/5258
Re: RFR: 8272600: (test) Use native "sleep" in Basic.java [v3]
On Sat, 28 Aug 2021 02:34:48 GMT, Roger Riggs wrote: >> The intermittent test in java/lang/ProcessBuilder/Basic.java has identified >> unexpected messages from a child Java VM >> as the cause of the test failure. Attempts to control the output of the >> child VM have failed, the VM is unrepentant . >> >> There is no functionality in the child except to wait long enough for the >> test to finish and the child is destroyed. >> The fix is to switch from using a Java child to using a native child; a new >> executable `sleepmillis`. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Revised to use native /bin/sleep program on Unix* (non-Windows). > For Windows, a native "BasicSleep" program is used. I've had very inconsistent results using the Windows 'timeout' command. There appear to multiple versions. Some give an error message "Type "TIMEOUT /?" for usage.", (Windows 10) others a error message "Try 'timeout --help' for more information." (Windows 2016) And in some cases, it does not appear to wait the entire time requested and one of the test fails. - PR: https://git.openjdk.java.net/jdk/pull/5239
Re: RFR: 8272600: (test) Use native "sleep" in Basic.java [v3]
On Tue, 31 Aug 2021 01:12:40 GMT, David Holmes wrote:
>> Roger Riggs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Revised to use native /bin/sleep program on Unix* (non-Windows).
>> For Windows, a native "BasicSleep" program is used.
>
> test/jdk/java/lang/ProcessBuilder/Basic.java line 2646:
>
>> 2644: if (exePath.toFile().canExecute()) {
>> 2645: return exePath;
>> 2646: }
>
> Not sure why this is so elaborate when elsewhere in the test we just assume
> `/usr/bin/env` exists?
True enough, will simplify. At one point, I was going to name the BasicSleep
just "sleep" and put the native path at the end of a search list. Some other
executables are not reliably in the same directories in all Unix versions.
-
PR: https://git.openjdk.java.net/jdk/pull/5239
Re: RFR: 8272600: (test) Use native "sleep" in Basic.java [v3]
On Tue, 31 Aug 2021 01:26:17 GMT, David Holmes wrote:
>> Roger Riggs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Revised to use native /bin/sleep program on Unix* (non-Windows).
>> For Windows, a native "BasicSleep" program is used.
>
> test/jdk/java/lang/ProcessBuilder/Basic.java line 2665:
>
>> 2663:
>> 2664: Path exePath =
>> Path.of(TEST_NATIVEPATH).resolve("BasicSleep.exe");
>> 2665: System.out.println("exePath: " + exePath + ", canExec: " +
>> exePath.toFile().canExecute());
>
> What is this for??
Ioi was concerned that the fallback to Java would be used silently and the old
intermittent issue would return.
-
PR: https://git.openjdk.java.net/jdk/pull/5239
Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux
On Wed, 1 Sep 2021 06:45:26 GMT, Wu Yan wrote: > Hi, > Please help me review the change to enhance getting time zone ID from > /etc/localtime on linux. > > We use `realpath` instead of `readlink` to obtain the link name of > /etc/localtime, because `readlink` can only read the value of a symbolic of > link, not the canonicalized absolute pathname. > > For example, the value of /etc/localtime is > "../usr/share/zoneinfo//Asia/Shanghai", then the linkbuf obtained by > `readlink` is "../usr/share/zoneinfo//Asia/Shanghai", and then the call of > `getZoneName(linkbuf)` will get "/Asia/Shanghai", not "Asia/Shanghai", which > consider as invalid in `ZoneInfoFile.getZoneInfo()`. Using `realpath`, you > can get “/usr/share/zoneinfo/Asia/Shanghai“ directly from “/etc/localtime“. > > Thanks, > wuyan The change looks reasonable. Please test your fix with macOS as well. - PR: https://git.openjdk.java.net/jdk/pull/5327
Re: RFR: 6957241: ClassLoader.getResources() returns only 1 instance when using jar indexing
On Tue, 31 Aug 2021 12:11:46 GMT, wxiang
wrote:
> Using jarIndex for Hibench, there is an unexpected behavior with the
> exception "Exception in thread "main"
> org.apache.hadoop.fs.UnsupportedFileSystemException: No FileSystem for scheme
> "hdfs"".
>
> After investigating it, it is related to the usage of ServiceLoader with
> JarIndex.
> The below stack shows the issue with JDK11:
>
> getResource:1016, URLClassPath$JarLoader (jdk.internal.loader)
> getResource:937, URLClassPath$JarLoader (jdk.internal.loader)
> findResource:912, URLClassPath$JarLoader (jdk.internal.loader)
> next:341, URLClassPath$1 (jdk.internal.loader)
> hasMoreElements:351, URLClassPath$1 (jdk.internal.loader)
> hasNext:355, BuiltinClassLoader$1 (jdk.internal.loader)
> hasMoreElements:363, BuiltinClassLoader$1 (jdk.internal.loader)
> next:3032, CompoundEnumeration (java.lang)
> hasMoreElements:3041, CompoundEnumeration (java.lang)
> nextProviderClass:1203, ServiceLoader$LazyClassPathLookupIterator (java.util)
> hasNextService:1221, ServiceLoader$LazyClassPathLookupIterator (java.util)
> hasNext:1265, ServiceLoader$LazyClassPathLookupIterator (java.util)
> hasNext:1300, ServiceLoader$2 (java.util)
> hasNext:1385, ServiceLoader$3 (java.util)
>
> The below API tries to get all the resources with the same name.
>
> public Enumeration findResources(final String name,
> final boolean check)
> ```
> After using JarIndex, URLClassPath.findResources only returns 1 URL.
> It is the same as the description in JDK-6957241.
>
> The issue still exists in JDK18.
>
> Root cause:
>
> public Enumeration findResources(final String name,
> final boolean check) {
> return new Enumeration<>() {
> private int index = 0;
> private URL url = null;
>
> private boolean next() {
> if (url != null) {
> return true;
> } else {
> Loader loader;
> while ((loader = getLoader(index++)) != null) {
> url = loader.findResource(name, check);
> if (url != null) {
> return true;
> }
> }
> return false;
> }
> }
> ...
> };
> }
>
> With the JarIndex, there is only one loader which is corresponding to the jar
> with the index due to the implementation in JarLoader.getResource(final
> String name, boolean check, Set visited).
>
> Loaders corresponding to other jar packages will not appear in this while.
> So it only returns 1 instance.
>
> To solve the issue, I change the implementation "private boolean next()".
> If the loader has index, traverse the index and get all the resource from the
> loader.
If there is a way to simplify the UCP code by removing the support for legacy
JAR index, and if it doesn't cause regressions, I'm all for it. But given the
intricacy of that code I suspect it will be quite an undertaking - and
reviewing such a change will probably not be trivial. The current PR is not
trivial to review and seems to be adding an other layer of complexity though,
so I agree that we should have this discussion.
-
PR: https://git.openjdk.java.net/jdk/pull/5316
Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux
On Wed, 1 Sep 2021 06:45:26 GMT, Wu Yan wrote: > Hi, > Please help me review the change to enhance getting time zone ID from > /etc/localtime on linux. > > We use `realpath` instead of `readlink` to obtain the link name of > /etc/localtime, because `readlink` can only read the value of a symbolic of > link, not the canonicalized absolute pathname. > > For example, the value of /etc/localtime is > "../usr/share/zoneinfo//Asia/Shanghai", then the linkbuf obtained by > `readlink` is "../usr/share/zoneinfo//Asia/Shanghai", and then the call of > `getZoneName(linkbuf)` will get "/Asia/Shanghai", not "Asia/Shanghai", which > consider as invalid in `ZoneInfoFile.getZoneInfo()`. Using `realpath`, you > can get “/usr/share/zoneinfo/Asia/Shanghai“ directly from “/etc/localtime“. > > Thanks, > wuyan Using `realpath` instead of `readlink` will change results on systems which use symbolic links instead of hard links to de-duplicate the timezone files. For example, on Debian, if `UTC` is configured (`/etc/localtime` points to `/usr/share/zoneinfo/UTC`), I think the result will be `Etc/UTC` instead of `UTC` after this change. But I have not actually tested this. src/java.base/unix/native/libjava/TimeZone_md.c line 292: > 290: /* canonicalize the path */ > 291: char resolvedpath[PATH_MAX + 1]; > 292: char *path = realpath(DEFAULT_ZONEINFO_FILE, resolvedpath); You really should use `realpath` with `NULL` as the second argument, to avoid any risk of buffer overflow. Future C library headers may warn about non-null arguments here. - PR: https://git.openjdk.java.net/jdk/pull/5327
Re: RFR: 6957241: ClassLoader.getResources() returns only 1 instance when using jar indexing
On Tue, 31 Aug 2021 12:11:46 GMT, wxiang
wrote:
> Using jarIndex for Hibench, there is an unexpected behavior with the
> exception "Exception in thread "main"
> org.apache.hadoop.fs.UnsupportedFileSystemException: No FileSystem for scheme
> "hdfs"".
>
> After investigating it, it is related to the usage of ServiceLoader with
> JarIndex.
> The below stack shows the issue with JDK11:
>
> getResource:1016, URLClassPath$JarLoader (jdk.internal.loader)
> getResource:937, URLClassPath$JarLoader (jdk.internal.loader)
> findResource:912, URLClassPath$JarLoader (jdk.internal.loader)
> next:341, URLClassPath$1 (jdk.internal.loader)
> hasMoreElements:351, URLClassPath$1 (jdk.internal.loader)
> hasNext:355, BuiltinClassLoader$1 (jdk.internal.loader)
> hasMoreElements:363, BuiltinClassLoader$1 (jdk.internal.loader)
> next:3032, CompoundEnumeration (java.lang)
> hasMoreElements:3041, CompoundEnumeration (java.lang)
> nextProviderClass:1203, ServiceLoader$LazyClassPathLookupIterator (java.util)
> hasNextService:1221, ServiceLoader$LazyClassPathLookupIterator (java.util)
> hasNext:1265, ServiceLoader$LazyClassPathLookupIterator (java.util)
> hasNext:1300, ServiceLoader$2 (java.util)
> hasNext:1385, ServiceLoader$3 (java.util)
>
> The below API tries to get all the resources with the same name.
>
> public Enumeration findResources(final String name,
> final boolean check)
> ```
> After using JarIndex, URLClassPath.findResources only returns 1 URL.
> It is the same as the description in JDK-6957241.
>
> The issue still exists in JDK18.
>
> Root cause:
>
> public Enumeration findResources(final String name,
> final boolean check) {
> return new Enumeration<>() {
> private int index = 0;
> private URL url = null;
>
> private boolean next() {
> if (url != null) {
> return true;
> } else {
> Loader loader;
> while ((loader = getLoader(index++)) != null) {
> url = loader.findResource(name, check);
> if (url != null) {
> return true;
> }
> }
> return false;
> }
> }
> ...
> };
> }
>
> With the JarIndex, there is only one loader which is corresponding to the jar
> with the index due to the implementation in JarLoader.getResource(final
> String name, boolean check, Set visited).
>
> Loaders corresponding to other jar packages will not appear in this while.
> So it only returns 1 instance.
>
> To solve the issue, I change the implementation "private boolean next()".
> If the loader has index, traverse the index and get all the resource from the
> loader.
Thanks for the PR/patch. I think we need a discussion on core-libs-dev as to
whether to keep the legacy JAR index support. The original motivation was
applets when it was added in JDK 1.3. I don't think findResources has ever
worked with the JAR index for cases where the same resource is in more than one
JAR files. I'm not opposed to fixing it but it does add complexity and will
likely refactoring the UCP implementation for something that will likely go
away eventually anyway.
-
PR: https://git.openjdk.java.net/jdk/pull/5316
Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux
On Wed, 1 Sep 2021 06:45:26 GMT, Wu Yan wrote: > Hi, > Please help me review the change to enhance getting time zone ID from > /etc/localtime on linux. > > We use `realpath` instead of `readlink` to obtain the link name of > /etc/localtime, because `readlink` can only read the value of a symbolic of > link, not the canonicalized absolute pathname. > > For example, the value of /etc/localtime is > "../usr/share/zoneinfo//Asia/Shanghai", then the linkbuf obtained by > `readlink` is "../usr/share/zoneinfo//Asia/Shanghai", and then the call of > `getZoneName(linkbuf)` will get "/Asia/Shanghai", not "Asia/Shanghai", which > consider as invalid in `ZoneInfoFile.getZoneInfo()`. Using `realpath`, you > can get “/usr/share/zoneinfo/Asia/Shanghai“ directly from “/etc/localtime“. > > Thanks, > wuyan I haven't come across this configuration like but changing it to use realpath seem reasonable. - PR: https://git.openjdk.java.net/jdk/pull/5327
Re: RFR: 8273140: Replace usages of Enum.class.getEnumConstants() with Enum.values() where possible [v4]
On Tue, 31 Aug 2021 20:04:23 GMT, Сергей Цыпанов
wrote:
>> Just a very tiny clean-up.
>>
>> There are some places in JDK code base where we call
>> `Enum.class.getEnumConstants()` to get all the values of the referenced
>> `enum`. This is excessive, less-readable and slower than just calling
>> `Enum.values()` as in `getEnumConstants()` we have volatile access:
>>
>> public T[] getEnumConstants() {
>> T[] values = getEnumConstantsShared();
>> return (values != null) ? values.clone() : null;
>> }
>>
>> private transient volatile T[] enumConstants;
>>
>> T[] getEnumConstantsShared() {
>> T[] constants = enumConstants;
>> if (constants == null) { /* ... */ }
>> return constants;
>> }
>>
>> Calling values() method is slightly faster:
>>
>> @BenchmarkMode(Mode.AverageTime)
>> @OutputTimeUnit(TimeUnit.NANOSECONDS)
>> @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
>> public class EnumBenchmark {
>>
>> @Benchmark
>> public Enum[] values() {
>> return Enum.values();
>> }
>>
>> @Benchmark
>> public Enum[] getEnumConstants() {
>> return Enum.class.getEnumConstants();
>> }
>>
>> private enum Enum {
>> A,
>> B
>> }
>> }
>>
>>
>> BenchmarkMode Cnt
>> Score Error Units
>> EnumBenchmark.getEnumConstants avgt 15
>> 6,265 ± 0,051 ns/op
>> EnumBenchmark.getEnumConstants:·gc.alloc.rateavgt 15
>> 2434,075 ± 19,568 MB/sec
>> EnumBenchmark.getEnumConstants:·gc.alloc.rate.norm avgt 15
>> 24,002 ± 0,001B/op
>> EnumBenchmark.getEnumConstants:·gc.churn.G1_Eden_Space avgt 15
>> 2433,709 ± 70,216 MB/sec
>> EnumBenchmark.getEnumConstants:·gc.churn.G1_Eden_Space.norm avgt 15
>> 23,998 ± 0,659B/op
>> EnumBenchmark.getEnumConstants:·gc.churn.G1_Survivor_Space avgt 15
>> 0,009 ± 0,003 MB/sec
>> EnumBenchmark.getEnumConstants:·gc.churn.G1_Survivor_Space.norm avgt 15
>> ≈ 10⁻⁴ B/op
>> EnumBenchmark.getEnumConstants:·gc.count avgt 15
>> 210,000counts
>> EnumBenchmark.getEnumConstants:·gc.time avgt 15
>> 119,000ms
>>
>> EnumBenchmark.values avgt 15
>> 4,164 ± 0,134 ns/op
>> EnumBenchmark.values:·gc.alloc.rate avgt 15
>> 3665,341 ± 120,721 MB/sec
>> EnumBenchmark.values:·gc.alloc.rate.norm avgt 15
>> 24,002 ± 0,001B/op
>> EnumBenchmark.values:·gc.churn.G1_Eden_Space avgt 15
>> 3660,512 ± 137,250 MB/sec
>> EnumBenchmark.values:·gc.churn.G1_Eden_Space.normavgt 15
>> 23,972 ± 0,529B/op
>> EnumBenchmark.values:·gc.churn.G1_Survivor_Space avgt 15
>> 0,017 ± 0,003 MB/sec
>> EnumBenchmark.values:·gc.churn.G1_Survivor_Space.normavgt 15
>> ≈ 10⁻⁴ B/op
>> EnumBenchmark.values:·gc.count avgt 15
>> 262,000counts
>> EnumBenchmark.values:·gc.timeavgt 15
>> 155,000ms
>
> Сергей Цыпанов has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8273140: Fix copyright year
Lgtm.
-
Marked as reviewed by tschatzl (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/5303
RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux
Hi, Please help me review the change to enhance getting time zone ID from /etc/localtime on linux. We use `realpath` instead of `readlink` to obtain the link name of /etc/localtime, because `readlink` can only read the value of a symbolic of link, not the canonicalized absolute pathname. For example, the value of /etc/localtime is "../usr/share/zoneinfo//Asia/Shanghai", then the linkbuf obtained by `readlink` is "../usr/share/zoneinfo//Asia/Shanghai", and then the call of `getZoneName(linkbuf)` will get "/Asia/Shanghai", not "Asia/Shanghai", which consider as invalid in `ZoneInfoFile.getZoneInfo()`. Using `realpath`, you can get “/usr/share/zoneinfo/Asia/Shanghai“ directly from “/etc/localtime“. Thanks, wuyan - Commit messages: - 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux Changes: https://git.openjdk.java.net/jdk/pull/5327/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5327=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8273111 Stats: 10 lines in 1 file changed: 2 ins; 1 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/5327.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5327/head:pull/5327 PR: https://git.openjdk.java.net/jdk/pull/5327
