Re: RFR: 8279508: Auto-vectorize Math.round API [v9]
On Fri, 25 Feb 2022 06:22:42 GMT, Jatin Bhateja wrote: >> Summary of changes: >> - Intrinsify Math.round(float) and Math.round(double) APIs. >> - Extend auto-vectorizer to infer vector operations on encountering scalar >> IR nodes for above intrinsics. >> - Test creation using new IR testing framework. >> >> Following are the performance number of a JMH micro included with the patch >> >> Test System: Intel(R) Xeon(R) Platinum 8380 CPU @ 2.30GHz (Icelake Server) >> >> >> Benchmark | TESTSIZE | Baseline AVX3 (ops/ms) | Withopt AVX3 (ops/ms) | Gain >> ratio | Baseline AVX2 (ops/ms) | Withopt AVX2 (ops/ms) | Gain ratio >> -- | -- | -- | -- | -- | -- | -- | -- >> FpRoundingBenchmark.test_round_double | 1024.00 | 504.15 | 2209.54 | 4.38 | >> 510.36 | 548.39 | 1.07 >> FpRoundingBenchmark.test_round_double | 2048.00 | 293.64 | 1271.98 | 4.33 | >> 293.48 | 274.01 | 0.93 >> FpRoundingBenchmark.test_round_float | 1024.00 | 825.99 | 4754.66 | 5.76 | >> 751.83 | 2274.13 | 3.02 >> FpRoundingBenchmark.test_round_float | 2048.00 | 412.22 | 2490.09 | 6.04 | >> 388.52 | 1334.18 | 3.43 >> >> >> Kindly review and share your feedback. >> >> Best Regards, >> Jatin > > Jatin Bhateja has updated the pull request incrementally with one additional > commit since the last revision: > > 8279508: Adding descriptive comments. As per SDM, if post conversion a floating point number is non-representable in destination format e.g. a floating point value 3.4028235E10 post integer conversion will overflow the value range of integer primitive type, hence a -0.0 value or 0x8000 is returned here. Similarly for +/- NaN and +/-Inf post conversion value returns is -0.0. All these cases i.e. post conversion non-representable floating point values and NaN/Inf values are handled in a special manner where algorithm first performs an unordered comparison b/w original source value and returns a 0 in case of NaN, this weeds out the NaN case and for rest of the special values we check the MSB bit of the source and either return an Integer.MAX_VALUE for +ve numbers or a Integer.MIN_VALUE to adhere to the semantics of Math.round API. Existing tests were enhanced to cover various special cases (NaN/Inf/+ve/-ve value/values which may be inexact after adding 0.5/ values which post conversion overflow integer value range). - PR: https://git.openjdk.java.net/jdk/pull/7094
Re: RFR: 8279508: Auto-vectorize Math.round API [v9]
On Sat, 26 Feb 2022 03:02:51 GMT, Sandhya Viswanathan wrote: >> src/hotspot/cpu/x86/x86.ad line 7263: >> >>> 7261: __ vector_round_float_avx($dst$$XMMRegister, $src$$XMMRegister, >>> $xtmp1$$XMMRegister, >>> 7262: $xtmp2$$XMMRegister, >>> $xtmp3$$XMMRegister, $xtmp4$$XMMRegister, >>> 7263: >>> ExternalAddress(vector_float_signflip()), new_mxcsr, $scratch$$Register, >>> vlen_enc); >> >> The vector_float_signflip() here should be replaced by vector_all_bits_set(). >> cvtps2dq description: >> If a converted result cannot be represented in the destination >> format, the floating-point invalid exception is raised, and if this >> exception is masked, the indefinite integer value >> (2w-1, where w represents the number of bits in the destination format) is >> returned. > > Clarification, the number in my comments above is (2^w - 1). This is from > Intel SDM > (https://www.intel.com/content/www/us/en/developer/articles/technical/intel-sdm.html). > Also you will need to take care when the valid unoverflowed result is -1 i.e. > 0x (2^32 - 1). I believe the indefinite value should be 2^(w - 1) (a.k.a 0x8000) and the documentation is typoed. If you look at `cvtss2si`, the indefinite value is also written as 2^w - 1 but yet in `MacroAssembler::convert_f2i` we compare it with 0x8000. In addition, choosing -1 as an indefinite value is weird enough and to complicate it as 2^w - 1 is really unusual. - PR: https://git.openjdk.java.net/jdk/pull/7094
Re: RFR: 8279508: Auto-vectorize Math.round API [v9]
On Sat, 26 Feb 2022 03:37:32 GMT, Quan Anh Mai wrote: >> Clarification, the number in my comments above is (2^w - 1). This is from >> Intel SDM >> (https://www.intel.com/content/www/us/en/developer/articles/technical/intel-sdm.html). >> Also you will need to take care when the valid unoverflowed result is -1 >> i.e. 0x (2^32 - 1). > > I believe the indefinite value should be 2^(w - 1) (a.k.a 0x8000) and the > documentation is typoed. If you look at `cvtss2si`, the indefinite value is > also written as 2^w - 1 but yet in `MacroAssembler::convert_f2i` we compare > it with 0x8000. In addition, choosing -1 as an indefinite value is weird > enough and to complicate it as 2^w - 1 is really unusual. `MacroAssembler::convert_f2i` https://github.com/openjdk/jdk/blob/c5c6058fd57d4b594012035eaf18a57257f4ad85/src/hotspot/cpu/x86/macroAssembler_x86.cpp#L8919 - PR: https://git.openjdk.java.net/jdk/pull/7094
Re: RFR: 8282023: PropertiesStoreTest and StoreReproducibilityTest jtreg failures due to en_CA locale [v3]
On Fri, 25 Feb 2022 04:44:45 GMT, Naoto Sato wrote: >> That's a very good point. I've updated the PR to now explicitly use a >> mutable `Set` instead of using `Collectors.toSet()` > > This is ok, although `Collectors.toCollection(HashSet::new)` is a bit concise. Hello Naoto, I've updated the PR to use `HashSet::new`. - PR: https://git.openjdk.java.net/jdk/pull/7558
Re: RFR: 8282023: PropertiesStoreTest and StoreReproducibilityTest jtreg failures due to en_CA locale [v5]
On Fri, 25 Feb 2022 08:44:42 GMT, Jaikiran Pai wrote: >> Can I please get a review of this test only change which fixes the issue >> noted in https://bugs.openjdk.java.net/browse/JDK-8282023? >> >> As noted in that JBS issue these tests fail when the default locale under >> which those tests are run isn't `en_US`. Both these tests were introduced as >> part of https://bugs.openjdk.java.net/browse/JDK-8231640. At a high level, >> these test do the following: >> - Use Properties.store(...) APIs to write out a properties file. This >> internally ends up writing a date comment via a call to >> `java.util.Date.toString()` method. >> - The test then runs a few checks to make sure that the written out `Date` >> is an expected one. To run these checks it uses the >> `java.time.format.DateTimeFormatter` to parse the date comment out of the >> properties file. >> >> All this works fine when the default locale is (for example) `en_US`. >> However, when the default locale is (for example `en_CA` or even `hi_IN`) >> the tests fail with an exception in the latter step above while parsing the >> date comment using the `DateTimeFormatter` instance. The exception looks >> like: >> >> >> Using locale: he for Properties#store(OutputStream) test >> test PropertiesStoreTest.testStoreOutputStreamDateComment(he): failure >> java.lang.AssertionError: Unexpected date comment: Mon Feb 21 19:10:31 IST >> 2022 >> at org.testng.Assert.fail(Assert.java:87) >> at PropertiesStoreTest.testDateComment(PropertiesStoreTest.java:255) >> at >> PropertiesStoreTest.testStoreOutputStreamDateComment(PropertiesStoreTest.java:223) >> at >> java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104) >> at java.base/java.lang.reflect.Method.invoke(Method.java:577) >> at >> org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:132) >> at org.testng.internal.TestInvoker.invokeMethod(TestInvoker.java:599) >> at >> org.testng.internal.TestInvoker.invokeTestMethod(TestInvoker.java:174) >> at org.testng.internal.MethodRunner.runInSequence(MethodRunner.java:46) >> at >> org.testng.internal.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:822) >> at >> org.testng.internal.TestInvoker.invokeTestMethods(TestInvoker.java:147) >> at >> org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:146) >> at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:128) >> at java.base/java.util.ArrayList.forEach(ArrayList.java:1511) >> at org.testng.TestRunner.privateRun(TestRunner.java:764) >> at org.testng.TestRunner.run(TestRunner.java:585) >> at org.testng.SuiteRunner.runTest(SuiteRunner.java:384) >> at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:378) >> at org.testng.SuiteRunner.privateRun(SuiteRunner.java:337) >> at org.testng.SuiteRunner.run(SuiteRunner.java:286) >> at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:53) >> at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:96) >> at org.testng.TestNG.runSuitesSequentially(TestNG.java:1218) >> at org.testng.TestNG.runSuitesLocally(TestNG.java:1140) >> at org.testng.TestNG.runSuites(TestNG.java:1069) >> at org.testng.TestNG.run(TestNG.java:1037) >> at >> com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:94) >> at >> com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:54) >> at >> java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104) >> at java.base/java.lang.reflect.Method.invoke(Method.java:577) >> at >> com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127) >> at java.base/java.lang.Thread.run(Thread.java:828) >> Caused by: java.time.format.DateTimeParseException: Text 'Mon Feb 21 >> 19:10:31 IST 2022' could not be parsed at index 0 >> at >> java.base/java.time.format.DateTimeFormatter.parseResolved0(DateTimeFormatter.java:2106) >> at >> java.base/java.time.format.DateTimeFormatter.parse(DateTimeFormatter.java:1934) >> at PropertiesStoreTest.testDateComment(PropertiesStoreTest.java:253) >> ... 30 more >> >> (in the exception above a locale with language `he` is being used) >> >> The root cause of this failure lies (only) within the tests - the >> `DateTimeFormatter` used in the tests is locale sensitive and uses the >> current (`Locale.getDefault()`) locale by default for parsing the date text. >> This parsing fails because, although `Date.toString()` javadoc states >> nothing about locales, it does mention the exact characters that will be >> used to write out the date comment. In other words, this date comment >> written out is locale insensitive and as such when parsing using >> `DateTimeFormatter` a neutral locale is appropriate on the >>
Re: RFR: 8279508: Auto-vectorize Math.round API [v9]
On Sat, 26 Feb 2022 01:06:21 GMT, Sandhya Viswanathan wrote: >> Jatin Bhateja has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8279508: Adding descriptive comments. > > src/hotspot/cpu/x86/x86.ad line 7263: > >> 7261: __ vector_round_float_avx($dst$$XMMRegister, $src$$XMMRegister, >> $xtmp1$$XMMRegister, >> 7262: $xtmp2$$XMMRegister, >> $xtmp3$$XMMRegister, $xtmp4$$XMMRegister, >> 7263: >> ExternalAddress(vector_float_signflip()), new_mxcsr, $scratch$$Register, >> vlen_enc); > > The vector_float_signflip() here should be replaced by vector_all_bits_set(). > cvtps2dq description: > If a converted result cannot be represented in the destination > format, the floating-point invalid exception is raised, and if this exception > is masked, the indefinite integer value > (2w-1, where w represents the number of bits in the destination format) is > returned. Clarification, the number in my comments above is (2^w - 1). This is from Intel SDM (https://www.intel.com/content/www/us/en/developer/articles/technical/intel-sdm.html). Also you will need to take care when the valid unoverflowed result is -1 i.e. 0x (2^32 - 1). - PR: https://git.openjdk.java.net/jdk/pull/7094
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v10]
On Fri, 25 Feb 2022 23:27:28 GMT, Yasser Bazzi wrote: > I prefered to keep the interface explicit to the reader but this object's goal is to serve as a random than a randomgenerator - PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8279508: Auto-vectorize Math.round API [v9]
On Fri, 25 Feb 2022 06:22:42 GMT, Jatin Bhateja wrote: >> Summary of changes: >> - Intrinsify Math.round(float) and Math.round(double) APIs. >> - Extend auto-vectorizer to infer vector operations on encountering scalar >> IR nodes for above intrinsics. >> - Test creation using new IR testing framework. >> >> Following are the performance number of a JMH micro included with the patch >> >> Test System: Intel(R) Xeon(R) Platinum 8380 CPU @ 2.30GHz (Icelake Server) >> >> >> Benchmark | TESTSIZE | Baseline AVX3 (ops/ms) | Withopt AVX3 (ops/ms) | Gain >> ratio | Baseline AVX2 (ops/ms) | Withopt AVX2 (ops/ms) | Gain ratio >> -- | -- | -- | -- | -- | -- | -- | -- >> FpRoundingBenchmark.test_round_double | 1024.00 | 504.15 | 2209.54 | 4.38 | >> 510.36 | 548.39 | 1.07 >> FpRoundingBenchmark.test_round_double | 2048.00 | 293.64 | 1271.98 | 4.33 | >> 293.48 | 274.01 | 0.93 >> FpRoundingBenchmark.test_round_float | 1024.00 | 825.99 | 4754.66 | 5.76 | >> 751.83 | 2274.13 | 3.02 >> FpRoundingBenchmark.test_round_float | 2048.00 | 412.22 | 2490.09 | 6.04 | >> 388.52 | 1334.18 | 3.43 >> >> >> Kindly review and share your feedback. >> >> Best Regards, >> Jatin > > Jatin Bhateja has updated the pull request incrementally with one additional > commit since the last revision: > > 8279508: Adding descriptive comments. Other than this the patch looks good to me. What testing have you done? src/hotspot/cpu/x86/x86.ad line 7263: > 7261: __ vector_round_float_avx($dst$$XMMRegister, $src$$XMMRegister, > $xtmp1$$XMMRegister, > 7262: $xtmp2$$XMMRegister, $xtmp3$$XMMRegister, > $xtmp4$$XMMRegister, > 7263: ExternalAddress(vector_float_signflip()), > new_mxcsr, $scratch$$Register, vlen_enc); The vector_float_signflip() here should be replaced by vector_all_bits_set(). cvtps2dq description: If a converted result cannot be represented in the destination format, the floating-point invalid exception is raised, and if this exception is masked, the indefinite integer value (2w-1, where w represents the number of bits in the destination format) is returned. src/hotspot/cpu/x86/x86.ad line 7280: > 7278: __ vector_round_float_evex($dst$$XMMRegister, $src$$XMMRegister, > $xtmp1$$XMMRegister, > 7279:$xtmp2$$XMMRegister, $ktmp1$$KRegister, > $ktmp2$$KRegister, > 7280: > ExternalAddress(vector_float_signflip()), new_mxcsr, $scratch$$Register, > vlen_enc); The vector_float_signflip() here should be replaced by vector_all_bits_set(). src/hotspot/cpu/x86/x86.ad line 7295: > 7293: __ vector_round_double_evex($dst$$XMMRegister, $src$$XMMRegister, > $xtmp1$$XMMRegister, > 7294: $xtmp2$$XMMRegister, $ktmp1$$KRegister, > $ktmp2$$KRegister, > 7295: > ExternalAddress(vector_double_signflip()), new_mxcsr, $scratch$$Register, > vlen_enc); The vector_double_signflip() here should be replaced by vector_all_bits_set(). vcvtpd2qq description: If a converted result cannot be represented in the destination format, the floating-point invalid exception is raised, and if this exception is masked, the indefinite integer value (2w-1, where w represents the number of bits in the destination format) is returned. - PR: https://git.openjdk.java.net/jdk/pull/7094
Re: RFR: 8282131: java.time.ZoneId should be a sealed abstract class
On Fri, 25 Feb 2022 19:02:55 GMT, Naoto Sato wrote: > Refactoring `java.time.ZoneId` class to be a sealed class. A CSR has also > been drafted. Marked as reviewed by scolebourne (Author). - PR: https://git.openjdk.java.net/jdk/pull/7625
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v10]
On Fri, 25 Feb 2022 23:05:48 GMT, Marcono1234 wrote:
>> Yasser Bazzi has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Use var instead of ArrayList raw
>>
>> Co-authored-by: liach <7806504+li...@users.noreply.github.com>
>
> src/java.base/share/classes/java/util/Random.java line 93:
>
>> 91:
>> 92: @SuppressWarnings("serial")
>> 93: private static class RandomWrapper extends Random implements
>> RandomGenerator {
>
> Isn't ` implements RandomGenerator` redundant because `Random` already
> implements `RandomGenerator`?
I prefered to keep the interface explicit to the reader
> src/java.base/share/classes/java/util/Random.java line 107:
>
>> 105: return rand;
>> 106:
>> 107: return (Random) new Random.RandomWrapper(random);
>
> Isn't the `(Random)` cast redundant?
Eclipse did not complain about it being redundant, also it does not impact the
reading of it and it shows the reader it is a Random instance that is returning
-
PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v11]
> Hi, could i get a review on this implementation proposed by Stuart Marks, i > decided to use the > https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/RandomGenerator.html > interface to create the default method `asRandom()` that wraps around the > newer algorithms to be used on classes that do not accept the new interface. > > Some things to note as proposed by the bug report, the protected method > next(int bits) is not overrided and setSeed() method if left blank up to > discussion on what to do with it. > > Small test done on > https://gist.github.com/YShow/da678561419cda8e32fccf3a27a649d4 Yasser Bazzi has updated the pull request incrementally with one additional commit since the last revision: Update javadoc - Changes: - all: https://git.openjdk.java.net/jdk/pull/7001/files - new: https://git.openjdk.java.net/jdk/pull/7001/files/7bc45057..60c1e382 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7001=10 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7001=09-10 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/7001.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7001/head:pull/7001 PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v10]
On Fri, 25 Feb 2022 22:56:36 GMT, Yasser Bazzi wrote:
>> Hi, could i get a review on this implementation proposed by Stuart Marks, i
>> decided to use the
>> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/RandomGenerator.html
>> interface to create the default method `asRandom()` that wraps around the
>> newer algorithms to be used on classes that do not accept the new interface.
>>
>> Some things to note as proposed by the bug report, the protected method
>> next(int bits) is not overrided and setSeed() method if left blank up to
>> discussion on what to do with it.
>>
>> Small test done on
>> https://gist.github.com/YShow/da678561419cda8e32fccf3a27a649d4
>
> Yasser Bazzi has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Use var instead of ArrayList raw
>
> Co-authored-by: liach <7806504+li...@users.noreply.github.com>
src/java.base/share/classes/java/util/Random.java line 92:
> 90: */
> 91:
> 92: @SuppressWarnings("serial")
Is this really fine, or should a `serialVersionUID` be defined?
(And maybe `initialized` should be made transient then)
src/java.base/share/classes/java/util/Random.java line 93:
> 91:
> 92: @SuppressWarnings("serial")
> 93: private static class RandomWrapper extends Random implements
> RandomGenerator {
Isn't ` implements RandomGenerator` redundant because `Random` already
implements `RandomGenerator`?
src/java.base/share/classes/java/util/Random.java line 107:
> 105: return rand;
> 106:
> 107: return (Random) new Random.RandomWrapper(random);
Isn't the `(Random)` cast redundant?
src/java.base/share/classes/java/util/Random.java line 290:
> 288: /**
> 289: * Returns an instance of {@link Random} based on this
> 290: * {@code java.util.random.RandomGenerator}. If this generator is
> already an instance of
Would it make sense to use a `{@link ...}` here instead?
(and maybe remove the `{@link ...}` from the `@param` and the `@return`; since
that might be rather uncommon for OpenJDK javadoc?)
-
PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v6]
On Sun, 20 Feb 2022 03:15:22 GMT, Yasser Bazzi wrote:
>> Hi, could i get a review on this implementation proposed by Stuart Marks, i
>> decided to use the
>> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/RandomGenerator.html
>> interface to create the default method `asRandom()` that wraps around the
>> newer algorithms to be used on classes that do not accept the new interface.
>>
>> Some things to note as proposed by the bug report, the protected method
>> next(int bits) is not overrided and setSeed() method if left blank up to
>> discussion on what to do with it.
>>
>> Small test done on
>> https://gist.github.com/YShow/da678561419cda8e32fccf3a27a649d4
>
> Yasser Bazzi has updated the pull request incrementally with one additional
> commit since the last revision:
>
> remove missed whitespace
src/java.base/share/classes/java/util/Random.java line 298:
> 296: */
> 297: public static Random from(RandomGenerator random) {
> 298: return RandomWrapper.wrap(random);
Might be good to check here or in the called methods / constructors for `null`.
Currently `null` would not be noticed until the first method is called on the
created `Random`, which makes it difficult for the user to track down bugs in
their code.
-
PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v6]
On Tue, 22 Feb 2022 23:19:14 GMT, Marcono1234 wrote:
>> Yasser Bazzi has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> remove missed whitespace
>
> src/java.base/share/classes/java/util/Random.java line 298:
>
>> 296: */
>> 297: public static Random from(RandomGenerator random) {
>> 298: return RandomWrapper.wrap(random);
>
> Might be good to check here or in the called methods / constructors for
> `null`. Currently `null` would not be noticed until the first method is
> called on the created `Random`, which makes it difficult for the user to
> track down bugs in their code.
Suggestion:
Objects.requireNonNull(random);
return RandomWrapper.wrap(random);
fyi this is the original change suggested by marcono1234. Notice that this
might need a csr update; maybe a javadoc update is needed too
-
PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v10]
> Hi, could i get a review on this implementation proposed by Stuart Marks, i > decided to use the > https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/RandomGenerator.html > interface to create the default method `asRandom()` that wraps around the > newer algorithms to be used on classes that do not accept the new interface. > > Some things to note as proposed by the bug report, the protected method > next(int bits) is not overrided and setSeed() method if left blank up to > discussion on what to do with it. > > Small test done on > https://gist.github.com/YShow/da678561419cda8e32fccf3a27a649d4 Yasser Bazzi has updated the pull request incrementally with one additional commit since the last revision: Use var instead of ArrayList raw Co-authored-by: liach <7806504+li...@users.noreply.github.com> - Changes: - all: https://git.openjdk.java.net/jdk/pull/7001/files - new: https://git.openjdk.java.net/jdk/pull/7001/files/b002205d..7bc45057 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7001=09 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7001=08-09 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7001.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7001/head:pull/7001 PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v9]
On Fri, 25 Feb 2022 01:13:41 GMT, Yasser Bazzi wrote:
>> Hi, could i get a review on this implementation proposed by Stuart Marks, i
>> decided to use the
>> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/RandomGenerator.html
>> interface to create the default method `asRandom()` that wraps around the
>> newer algorithms to be used on classes that do not accept the new interface.
>>
>> Some things to note as proposed by the bug report, the protected method
>> next(int bits) is not overrided and setSeed() method if left blank up to
>> discussion on what to do with it.
>>
>> Small test done on
>> https://gist.github.com/YShow/da678561419cda8e32fccf3a27a649d4
>
> Yasser Bazzi has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - change wrong indentation
> - change indentation
test/jdk/java/util/Random/RandomTest.java line 420:
> 418: */
> 419: public void testShufflingList() {
> 420: final ArrayList listTest = new ArrayList();
Suggestion:
final var listTest = new ArrayList();
Using `ArrayList` is fine too, but right now this is a raw type.
-
PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v9]
On Fri, 25 Feb 2022 01:13:41 GMT, Yasser Bazzi wrote: >> Hi, could i get a review on this implementation proposed by Stuart Marks, i >> decided to use the >> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/RandomGenerator.html >> interface to create the default method `asRandom()` that wraps around the >> newer algorithms to be used on classes that do not accept the new interface. >> >> Some things to note as proposed by the bug report, the protected method >> next(int bits) is not overrided and setSeed() method if left blank up to >> discussion on what to do with it. >> >> Small test done on >> https://gist.github.com/YShow/da678561419cda8e32fccf3a27a649d4 > > Yasser Bazzi has updated the pull request incrementally with two additional > commits since the last revision: > > - change wrong indentation > - change indentation Ok i think all changes are in place, should we consider @liach changes also on this PR? - PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: JDK-8266431: Dual-Pivot Quicksort improvements (Radix sort) [v11]
On Wed, 12 Jan 2022 14:22:03 GMT, iaroslavski wrote: >> Sorting: >> >> - adopt radix sort for sequential and parallel sorts on >> int/long/float/double arrays (almost random and length > 6K) >> - fix tryMergeRuns() to better handle case when the last run is a single >> element >> - minor javadoc and comment changes >> >> Testing: >> - add new data inputs in tests for sorting >> - add min/max/infinity values to float/double testing >> - add tests for radix sort > > iaroslavski has updated the pull request incrementally with one additional > commit since the last revision: > > JDK-8266431: Dual-Pivot Quicksort improvements (Radix sort) > > * Updated javadoc > * Improved mixed insertion > * Optimized sort networking > * Improved pivot partitioning New update is coming - PR: https://git.openjdk.java.net/jdk/pull/3938
Integrated: 8279995: jpackage --add-launcher option should allow overriding description
On Wed, 9 Feb 2022 07:37:42 GMT, Alexander Matveev wrote: > Added ability to override description for additional launchers via > "description" property. This pull request has now been integrated. Changeset: fb8bf818 Author:Alexander Matveev URL: https://git.openjdk.java.net/jdk/commit/fb8bf81842b55355f226ac9d8717646abd509721 Stats: 75 lines in 7 files changed: 65 ins; 0 del; 10 mod 8279995: jpackage --add-launcher option should allow overriding description Reviewed-by: asemenyuk - PR: https://git.openjdk.java.net/jdk/pull/7399
Re: RFR: JDK-8282144 RandomSupport.convertSeedBytesToLongs sign extension overwrites previous bytes
On Thu, 24 Feb 2022 14:47:50 GMT, Jim Laskey wrote:
> Class: ./java.base/share/classes/jdk/internal/util/random/RandomSupport.java
> Method: public static long[] convertSeedBytesToLongs(byte[] seed, int n, int
> z)
>
> The method attempts to create an array of longs by consuming the input bytes
> most significant bit first. New bytes are appended to the existing long using
> the OR operator on the signed byte. Due to sign extension this will overwrite
> all the existing bits from 63 to 8 if the next byte is negative.
test/jdk/java/util/Random/T8282144.java line 39:
> 37: public class T8282144 {
> 38: public static void main(String[] args) {
> 39: RandomGenerator rng =
> RandomGeneratorFactory.of("L64X128MixRandom").create(42);
Does `rng` always produce the same sequence? If so, then perhaps the seed,
`42`, should be a random value that is printed.
test/jdk/java/util/Random/T8282144.java line 52:
> 50: for (int k = 0; k < existing.length; k++) {
> 51: if (existing[k] != testing[k]) {
> 52: throw new
> RuntimeException("convertSeedBytesToLongs incorrect");
Should `i`, `j`, and `k` be included in the exception message?
-
PR: https://git.openjdk.java.net/jdk/pull/7614
Re: RFR: 8282131: java.time.ZoneId should be a sealed abstract class
On Fri, 25 Feb 2022 19:02:55 GMT, Naoto Sato wrote: > Refactoring `java.time.ZoneId` class to be a sealed class. A CSR has also > been drafted. Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7625
Re: RFR: 8282131: java.time.ZoneId should be a sealed abstract class
On Fri, 25 Feb 2022 19:02:55 GMT, Naoto Sato wrote: > Refactoring `java.time.ZoneId` class to be a sealed class. A CSR has also > been drafted. Thanks for doing this. Looks good. - Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7625
Re: RFR: 8282131: java.time.ZoneId should be a sealed abstract class
On Fri, 25 Feb 2022 19:02:55 GMT, Naoto Sato wrote: > Refactoring `java.time.ZoneId` class to be a sealed class. A CSR has also > been drafted. Marked as reviewed by bpb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7625
Re: RFR: 8282219: jdk/java/lang/ProcessBuilder/Basic.java fails on AIX [v5]
On Fri, 25 Feb 2022 17:45:39 GMT, Ichiroh Takiguchi wrote: >> Run jtreg:jdk/java/lang/ProcessBuilder/Basic.java on AIX. >> The test was failed by: >> Incorrect handling of envstrings containing NULs >> >> According to my investigation, this issue was happened after following >> change was applied. >> JDK-8272600: (test) Use native "sleep" in Basic.java >> >> test.nativepath value was added into AIX's LIBPATH during running this >> testcase. >> On AIX, test.nativepath value should be removed from LIBPATH value before >> comparing the values. > > Ichiroh Takiguchi has updated the pull request incrementally with one > additional commit since the last revision: > > Pass LIBPATH setting for AIX Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7574
Re: RFR: 8282131: java.time.ZoneId should be a sealed abstract class
On Fri, 25 Feb 2022 19:02:55 GMT, Naoto Sato wrote: > Refactoring `java.time.ZoneId` class to be a sealed class. A CSR has also > been drafted. Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7625
Re: RFR: 8282131: java.time.ZoneId should be a sealed abstract class
On Fri, 25 Feb 2022 19:02:55 GMT, Naoto Sato wrote: > Refactoring `java.time.ZoneId` class to be a sealed class. A CSR has also > been drafted. Associated CSR also reviewed. - Marked as reviewed by iris (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7625
RFR: 8282131: java.time.ZoneId should be a sealed abstract class
Refactoring `java.time.ZoneId` class to be a sealed class. A CSR has also been drafted. - Commit messages: - 8282131: java.time.ZoneId should be a sealed abstract class Changes: https://git.openjdk.java.net/jdk/pull/7625/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7625=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8282131 Stats: 10 lines in 1 file changed: 0 ins; 4 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/7625.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7625/head:pull/7625 PR: https://git.openjdk.java.net/jdk/pull/7625
Re: RFR: 8282219: jdk/java/lang/ProcessBuilder/Basic.java fails on AIX [v4]
On Fri, 25 Feb 2022 15:03:19 GMT, Roger Riggs wrote:
>> Ichiroh Takiguchi has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Add null check
>
> My preference is to pass the unmodified LIBPATH in the environment in each of
> the three cases.
> The expected checks already expect the unmodified LIBPATH so the only change
> is in the setup of the environment to be passed to the child.
>
> Already covered at line 1366:
>
> Insert at line 1873 in the if/then/else:
>
> } else if (AIX.is()) {
> envp = new String[] {"=ExitValue=3", "=C:=\",
> "LIBPATH="+libpath};
> } else {
>
> (Or assign it to a local as is done for windows, your preference).
>
> And at line 1921'ish:
> ```
>} else if (AIX.is()) {
> envp = new String[] {"LC_ALL=C\u\u", // Yuck!
> "FO\u=B\uR", "LIBPATH="+libpath};
>
>
> I looked using `sh -c env` but I think it would be more work to change the
> way the output is compared.
> The output of `env` is different and has some other values that get inserted.
Many thanks, @RogerRiggs .
I tried your suggested code, it worked fine on my AIX system.
Could you review this change again ?
-
PR: https://git.openjdk.java.net/jdk/pull/7574
Re: RFR: 8282219: jdk/java/lang/ProcessBuilder/Basic.java fails on AIX [v5]
> Run jtreg:jdk/java/lang/ProcessBuilder/Basic.java on AIX. > The test was failed by: > Incorrect handling of envstrings containing NULs > > According to my investigation, this issue was happened after following change > was applied. > JDK-8272600: (test) Use native "sleep" in Basic.java > > test.nativepath value was added into AIX's LIBPATH during running this > testcase. > On AIX, test.nativepath value should be removed from LIBPATH value before > comparing the values. Ichiroh Takiguchi has updated the pull request incrementally with one additional commit since the last revision: Pass LIBPATH setting for AIX - Changes: - all: https://git.openjdk.java.net/jdk/pull/7574/files - new: https://git.openjdk.java.net/jdk/pull/7574/files/b6d6afe7..60f8e96e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7574=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7574=03-04 Stats: 27 lines in 1 file changed: 8 ins; 17 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7574.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7574/head:pull/7574 PR: https://git.openjdk.java.net/jdk/pull/7574
Re: RFR: 8282047: Enhance StringDecode/Encode microbenchmarks [v3]
On Fri, 25 Feb 2022 11:08:49 GMT, Claes Redestad wrote: >> Splitting out these micro changes from #7231 >> >> - Clean up and simplify setup and code >> - Add variants with different inputs with varying lengths and encoding >> weights, but also relevant mixes of each so that we both cover interesting >> corner cases but also verify that performance behave when there's a >> multitude of input shapes. Both simple and mixed variants are interesting >> diagnostic tools. >> - Drop all charsets from the default run configuration except UTF-8. >> Motivation: Defaults should give good coverage but try to keep runtimes at >> bay. Additionally if the charset tested can't encode the higher codepoints >> used in these micros the results might be misleading. If you for example >> test using ISO-8859-1 the UTF-16 bytes in StringDecode.decodeUTF16 will have >> all been replaced by `?`, so the test is effectively the same as testing >> ASCII-only. > > Claes Redestad has updated the pull request incrementally with two additional > commits since the last revision: > > - Fix typos, add missing short variants to latin1 and utf16 short decode > methods > - Increase coverage of and weight of short strings Marked as reviewed by bchristi (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7516
Re: RFR: 8282219: jdk/java/lang/ProcessBuilder/Basic.java fails on AIX [v4]
On Wed, 23 Feb 2022 18:49:22 GMT, Ichiroh Takiguchi wrote: >> Run jtreg:jdk/java/lang/ProcessBuilder/Basic.java on AIX. >> The test was failed by: >> Incorrect handling of envstrings containing NULs >> >> According to my investigation, this issue was happened after following >> change was applied. >> JDK-8272600: (test) Use native "sleep" in Basic.java >> >> test.nativepath value was added into AIX's LIBPATH during running this >> testcase. >> On AIX, test.nativepath value should be removed from LIBPATH value before >> comparing the values. > > Ichiroh Takiguchi has updated the pull request incrementally with one > additional commit since the last revision: > > Add null check I think it's good to keep the test on AIX since we have good solutions to the failure. I added +1 to Roger's suggestion above. Looks like it should work well. - PR: https://git.openjdk.java.net/jdk/pull/7574
Re: RFR: 8282219: jdk/java/lang/ProcessBuilder/Basic.java fails on AIX [v4]
IMHO this is not hard to fix (we already have multiple proposals) and we'd
should have this test on AIX too.
Cheers, Thomas
On Fri, Feb 25, 2022 at 6:23 PM Joe Darcy wrote:
> How about excluding the test from running on AIX?
>
> -Joe
>
> On 2/25/2022 7:07 AM, Roger Riggs wrote:
> > On Wed, 23 Feb 2022 18:49:22 GMT, Ichiroh Takiguchi <
> itakigu...@openjdk.org> wrote:
> >
> >>> Run jtreg:jdk/java/lang/ProcessBuilder/Basic.java on AIX.
> >>> The test was failed by:
> >>>Incorrect handling of envstrings containing NULs
> >>>
> >>> According to my investigation, this issue was happened after following
> change was applied.
> >>> JDK-8272600: (test) Use native "sleep" in Basic.java
> >>>
> >>> test.nativepath value was added into AIX's LIBPATH during running this
> testcase.
> >>> On AIX, test.nativepath value should be removed from LIBPATH value
> before comparing the values.
> >> Ichiroh Takiguchi has updated the pull request incrementally with one
> additional commit since the last revision:
> >>
> >>Add null check
> > My preference is to pass the unmodified LIBPATH in the environment in
> each of the three cases.
> > The expected checks already expect the unmodified LIBPATH so the only
> change is in the setup of the environment to be passed to the child.
> >
> > Already covered at line 1366:
> >
> > Insert at line 1873 in the if/then/else:
> >
> > } else if (AIX.is()) {
> > envp = new String[] {"=ExitValue=3", "=C:=\",
> "LIBPATH="+libpath};
> > } else {
> >
> > (Or assign it to a local as is done for windows, your preference).
> >
> > And at line 1921'ish:
> > ```} else if (AIX.is()) {
> > envp = new String[] {"LC_ALL=C\u\u", // Yuck!
> > "FO\u=B\uR", "LIBPATH="+libpath};
> >
> >
> > I looked using `sh -c env` but I think it would be more work to change
> the way the output is compared.
> > The output of `env` is different and has some other values that get
> inserted.
> >
> > -
> >
> > PR: https://git.openjdk.java.net/jdk/pull/7574
>
Re: RFR: 8282219: jdk/java/lang/ProcessBuilder/Basic.java fails on AIX [v4]
How about excluding the test from running on AIX?
-Joe
On 2/25/2022 7:07 AM, Roger Riggs wrote:
On Wed, 23 Feb 2022 18:49:22 GMT, Ichiroh Takiguchi
wrote:
Run jtreg:jdk/java/lang/ProcessBuilder/Basic.java on AIX.
The test was failed by:
Incorrect handling of envstrings containing NULs
According to my investigation, this issue was happened after following change
was applied.
JDK-8272600: (test) Use native "sleep" in Basic.java
test.nativepath value was added into AIX's LIBPATH during running this testcase.
On AIX, test.nativepath value should be removed from LIBPATH value before
comparing the values.
Ichiroh Takiguchi has updated the pull request incrementally with one
additional commit since the last revision:
Add null check
My preference is to pass the unmodified LIBPATH in the environment in each of
the three cases.
The expected checks already expect the unmodified LIBPATH so the only change is
in the setup of the environment to be passed to the child.
Already covered at line 1366:
Insert at line 1873 in the if/then/else:
} else if (AIX.is()) {
envp = new String[] {"=ExitValue=3", "=C:=\",
"LIBPATH="+libpath};
} else {
(Or assign it to a local as is done for windows, your preference).
And at line 1921'ish:
```} else if (AIX.is()) {
envp = new String[] {"LC_ALL=C\u\u", // Yuck!
"FO\u=B\uR", "LIBPATH="+libpath};
I looked using `sh -c env` but I think it would be more work to change the way
the output is compared.
The output of `env` is different and has some other values that get inserted.
-
PR: https://git.openjdk.java.net/jdk/pull/7574
Re: RFR: 8282008: Incorrect handling of quoted arguments in ProcessBuilder [v3]
On Fri, 18 Feb 2022 17:21:48 GMT, Olga Mikhaltsova
wrote:
>> This fix made equal processing of strings such as ""C:\\Program
>> Files\\Git\\"" before and after JDK-8250568.
>>
>> For example, it's needed to execute the following command on Windows:
>> `C:\Windows\SysWOW64\WScript.exe "MyVB.vbs" "C:\Program Files\Git" "Test"`
>> it's equal to:
>> `new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs",
>> ""C:\\Program Files\\Git\\"", "Test").start();`
>>
>> While processing, the 3rd argument ""C:\\Program Files\\Git\\"" treated as
>> unquoted due to the condition added in JDK-8250568.
>>
>> private static String unQuote(String str) {
>> ..
>>if (str.endsWith("\\"")) {
>> return str;// not properly quoted, treat as unquoted
>> }
>> ..
>> }
>>
>>
>> that leads to the additional surrounding by quotes in
>> ProcessImpl::createCommandLine(..) because needsEscaping(..) returns true
>> due to the space inside the string argument.
>> As a result the native function CreateProcessW
>> (src/java.base/windows/native/libjava/ProcessImpl_md.c) gets the incorrectly
>> quoted argument:
>>
>> pcmd = C:\Windows\SysWOW64\WScript.exe MyVB.vbs ""C:\Program Files\Git"" Test
>> (jdk.lang.Process.allowAmbiguousCommands = true)
>> pcmd = "C:\Windows\SysWOW64\WScript.exe" MyVB.vbs ""C:\Program Files\Git\\""
>> Test
>> (jdk.lang.Process.allowAmbiguousCommands = false)
>>
>>
>> Obviously, a string ending with `"\\""` must not be started with `"""` to
>> treat as unquoted overwise it’s should be treated as properly quoted.
>
> Olga Mikhaltsova has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add a new line to the end of test file for JDK-8282008
It is apparent there is no one "correct" way to quote, but one of the key
features of the Java ecosystem has been its backwards compatibility. In that
light, this change allows our clients to continue doing what they did the way
they did it without the need for modification of their Java code or their
(maybe even foreign) native code. FWIW.
Separately from the above, I wonder if this change would make the fix less
controversial?
-if (str.endsWith("\\"")) {
+if (str.endsWith("\\"") && !str.endsWith(""")) {
This way we verify that the end quote is really just an escaped quote, while
correctly identifying escaped backslash as having nothing to do with the
following quote.
-
PR: https://git.openjdk.java.net/jdk/pull/7504
Re: Proposed JEP: Safer Process Launch by ProcessBuilder and Runtime.exec
Hi Raffaello, Runtime.exec and ProcessBuilder have been a challenge from the beginning, on one hand to have a reasonably cross-platform way to invoke processes and on the other to accommodate the platform differences, mostly on Windows due to the various command line parsing decoding of different applications and shells. The idea is to provide greater consistency and a clear guidance about how to invoke applications while guarding against inadvertent mistakes that might refer to files or other applications. Assembling command lines from individual strings is error prone and brittle especially in cases where some of the arguments may be assembled from configuration information, external inputs, and the container or server environment. Given the variations, the API should provide the application enough control and responsibility to customize the encoding to the target application, if it is needed, but handle the usual cases so it does not require developers to hand craft for each platform and target application. Ideally, the customization should be clearly visible in the API and be clear and easy to review and audit. Regards, Roger On 2/24/22 4:22 PM, Raffaello Giulietti wrote: Hi, on Windows, the mechanism to launch a new program is for the parent to call CreateProcess(). This function accepts, among others parameters, a lpApplicationName string and a lpCommandLine string. There's a *single* lpCommandLine that encodes all the arguments to pass to the new program. The exact encoding of the arguments in the command line is imposed by how the *new* program parses (decodes) the command line to get its constituent parts. There are no fixed rules on how this happens. There are some more or less well documented rules for some runtimes (e.g., the C/C++ runtime) or for some specific programs (e.g., cmd.exe., wscript.exe). In general, however, a program can decode in any way it deems useful. Because the encoding is dictated by the target program's decoding, and because the latter is really arbitrary, there's no safe, universal procedure to encode a list of string arguments to a single command line string. It is only when the decoding rules in the target program are known that encoding in the parent becomes feasible. Thus, it might be more useful on Windows platforms to avoid the API points that expose List or String[] for the arguments to the target program and use the ones that accept a single String, instead. The client of those API points would then have to deal with the encoding specific for that program. This is a better match with the underlying OS mechanism, namely CreateProcess(), which accepts a single, already encoded string. In addition, to assist programmers unfamiliar with specific encodings, widely used specific encoders (e.g., for the C/C++ runtime [1], for cmd.exe, etc.) can be implemented separately. Greetings Raffaello [1] https://mail.openjdk.java.net/pipermail/core-libs-dev/2022-February/086105.html
Re: RFR: 8279995: jpackage --add-launcher option should allow overriding description [v6]
On Fri, 25 Feb 2022 01:27:52 GMT, Alexander Matveev wrote: >> Added ability to override description for additional launchers via >> "description" property. > > Alexander Matveev has updated the pull request incrementally with one > additional commit since the last revision: > > 8279995: jpackage --add-launcher option should allow overriding description > [v5] Marked as reviewed by asemenyuk (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7399
Re: RFR: 8282219: jdk/java/lang/ProcessBuilder/Basic.java fails on AIX [v4]
On Wed, 23 Feb 2022 18:49:22 GMT, Ichiroh Takiguchi
wrote:
>> Run jtreg:jdk/java/lang/ProcessBuilder/Basic.java on AIX.
>> The test was failed by:
>> Incorrect handling of envstrings containing NULs
>>
>> According to my investigation, this issue was happened after following
>> change was applied.
>> JDK-8272600: (test) Use native "sleep" in Basic.java
>>
>> test.nativepath value was added into AIX's LIBPATH during running this
>> testcase.
>> On AIX, test.nativepath value should be removed from LIBPATH value before
>> comparing the values.
>
> Ichiroh Takiguchi has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add null check
My preference is to pass the unmodified LIBPATH in the environment in each of
the three cases.
The expected checks already expect the unmodified LIBPATH so the only change is
in the setup of the environment to be passed to the child.
Already covered at line 1366:
Insert at line 1873 in the if/then/else:
} else if (AIX.is()) {
envp = new String[] {"=ExitValue=3", "=C:=\",
"LIBPATH="+libpath};
} else {
(Or assign it to a local as is done for windows, your preference).
And at line 1921'ish:
```} else if (AIX.is()) {
envp = new String[] {"LC_ALL=C\u\u", // Yuck!
"FO\u=B\uR", "LIBPATH="+libpath};
I looked using `sh -c env` but I think it would be more work to change the way
the output is compared.
The output of `env` is different and has some other values that get inserted.
-
PR: https://git.openjdk.java.net/jdk/pull/7574
Re: RFR: 8275731: CDS archived enums objects are recreated at runtime [v3]
On Wed, 19 Jan 2022 05:44:10 GMT, Ioi Lam wrote:
>> src/hotspot/share/cds/heapShared.cpp line 433:
>>
>>> 431: oop mirror = k->java_mirror();
>>> 432: int i = 0;
>>> 433: for (JavaFieldStream fs(k); !fs.done(); fs.next()) {
>>
>> This seems like it should also use InstanceKlass::do_local_static_fields.
>
> Converting this to InstanceKlass::do_nonstatic_fields() is difficult because
> the loop body references 7 different variables declared outside of the loop.
>
> One thing I tried is to add a new version of do_nonstatic_fields2() that
> supports C++ lambdas. You can see my experiment from here:
>
> https://github.com/openjdk/jdk/compare/master...iklam:lambda-for-instanceklass-do_local_static_fields2?expand=1
>
> I changed all my new code to use the do_nonstatic_fields2() function with
> lambda.
Ok, if it requires lambdas and additional change, never mind then.
-
PR: https://git.openjdk.java.net/jdk/pull/6653
Re: RFR: 8275731: CDS archived enums objects are recreated at runtime [v6]
On Wed, 23 Feb 2022 04:15:28 GMT, Ioi Lam wrote:
>> **Background:**
>>
>> In the Java Language, Enums can be tested for equality, so the constants in
>> an Enum type must be unique. Javac compiles an enum declaration like this:
>>
>>
>> public enum Day { SUNDAY, MONDAY ... }
>>
>>
>> to
>>
>>
>> public class Day extends java.lang.Enum {
>> public static final SUNDAY = new Day("SUNDAY");
>> public static final MONDAY = new Day("MONDAY"); ...
>> }
>>
>>
>> With CDS archived heap objects, `Day::` is executed twice: once
>> during `java -Xshare:dump`, and once during normal JVM execution. If the
>> archived heap objects references one of the Enum constants created at dump
>> time, we will violate the uniqueness requirements of the Enum constants at
>> runtime. See the test case in the description of
>> [JDK-8275731](https://bugs.openjdk.java.net/browse/JDK-8275731)
>>
>> **Fix:**
>>
>> During -Xshare:dump, if we discovered that an Enum constant of type X is
>> archived, we archive all constants of type X. At Runtime, type X will skip
>> the normal execution of `X::`. Instead, we run
>> `HeapShared::initialize_enum_klass()` to retrieve all the constants of X
>> that were saved at dump time.
>>
>> This is safe as we know that `X::` has no observable side effect --
>> it only creates the constants of type X, as well as the synthetic value
>> `X::$VALUES`, which cannot be observed until X is fully initialized.
>>
>> **Verification:**
>>
>> To avoid future problems, I added a new tool, CDSHeapVerifier, to look for
>> similar problems where the archived heap objects reference a static field
>> that may be recreated at runtime. There are some manual steps involved, but
>> I analyzed the potential problems found by the tool are they are all safe
>> (after the current bug is fixed). See cdsHeapVerifier.cpp for gory details.
>> An example trace of this tool can be found at
>> https://bugs.openjdk.java.net/secure/attachment/97242/enum_warning.txt
>>
>> **Testing:**
>>
>> Passed Oracle CI tiers 1-4. WIll run tier 5 as well.
>
> Ioi Lam has updated the pull request incrementally with one additional commit
> since the last revision:
>
> fixed whitespace
Sorry for the long delay. It's a big change, but a lot in debug so that's ok.
Looks good.
-
Marked as reviewed by coleenp (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/6653
Re: RFR: 8282047: Enhance StringDecode/Encode microbenchmarks [v2]
On Fri, 25 Feb 2022 09:41:09 GMT, Andrew Haley wrote:
>> Claes Redestad has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Brent review
>
> test/micro/org/openjdk/bench/java/lang/StringDecode.java line 72:
>
>> 70: public void setup() {
>> 71: charset = Charset.forName(charsetName);
>> 72: asciiString = LOREM.substring(0, 32).getBytes(charset);
>
> This is problematic IMO in that it's missing short strings such as "Claes".
> Average Java strings are about 32 bytes long AFAICR, and people writing
> (vectorized) ijntrinsics have a nasty habit of optimizing for long strings,
> to the detriment of typical-length ones.
> Whether we like it or not, people will optimize for benchmarks, so it's
> important that benchmark data is realistic. The shortest here is 15 bytes, as
> far as I can see. I'd certainly include a short string of just a few bytes so
> that intrinsics don't cause regressions in important cases.
All good points. I've added a number of such short variants to all(?) relevant
microbenchmarks. The tests should now better cover a mix of input lengths and
encodings.
-
PR: https://git.openjdk.java.net/jdk/pull/7516
Re: RFR: 8282047: Enhance StringDecode/Encode microbenchmarks [v2]
On Fri, 25 Feb 2022 10:19:17 GMT, Claes Redestad wrote:
>> test/micro/org/openjdk/bench/java/lang/StringDecode.java line 72:
>>
>>> 70: public void setup() {
>>> 71: charset = Charset.forName(charsetName);
>>> 72: asciiString = LOREM.substring(0, 32).getBytes(charset);
>>
>> This is problematic IMO in that it's missing short strings such as "Claes".
>> Average Java strings are about 32 bytes long AFAICR, and people writing
>> (vectorized) ijntrinsics have a nasty habit of optimizing for long strings,
>> to the detriment of typical-length ones.
>> Whether we like it or not, people will optimize for benchmarks, so it's
>> important that benchmark data is realistic. The shortest here is 15 bytes,
>> as far as I can see. I'd certainly include a short string of just a few
>> bytes so that intrinsics don't cause regressions in important cases.
>
> All good points. I've added a number of such short variants to all(?)
> relevant microbenchmarks. The tests should now better cover a mix of input
> lengths and encodings.
-
PR: https://git.openjdk.java.net/jdk/pull/7516
Re: RFR: 8282047: Enhance StringDecode/Encode microbenchmarks [v3]
> Splitting out these micro changes from #7231 > > - Clean up and simplify setup and code > - Add variants with different inputs with varying lengths and encoding > weights, but also relevant mixes of each so that we both cover interesting > corner cases but also verify that performance behave when there's a multitude > of input shapes. Both simple and mixed variants are interesting diagnostic > tools. > - Drop all charsets from the default run configuration except UTF-8. > Motivation: Defaults should give good coverage but try to keep runtimes at > bay. Additionally if the charset tested can't encode the higher codepoints > used in these micros the results might be misleading. If you for example test > using ISO-8859-1 the UTF-16 bytes in StringDecode.decodeUTF16 will have all > been replaced by `?`, so the test is effectively the same as testing > ASCII-only. Claes Redestad has updated the pull request incrementally with two additional commits since the last revision: - Fix typos, add missing short variants to latin1 and utf16 short decode methods - Increase coverage of and weight of short strings - Changes: - all: https://git.openjdk.java.net/jdk/pull/7516/files - new: https://git.openjdk.java.net/jdk/pull/7516/files/0dd04498..fc2a5b05 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7516=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7516=01-02 Stats: 77 lines in 2 files changed: 62 ins; 0 del; 15 mod Patch: https://git.openjdk.java.net/jdk/pull/7516.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7516/head:pull/7516 PR: https://git.openjdk.java.net/jdk/pull/7516
Re: RFR: 8282047: Enhance StringDecode/Encode microbenchmarks [v2]
On Fri, 25 Feb 2022 09:19:33 GMT, Claes Redestad wrote:
>> Splitting out these micro changes from #7231
>>
>> - Clean up and simplify setup and code
>> - Add variants with different inputs with varying lengths and encoding
>> weights, but also relevant mixes of each so that we both cover interesting
>> corner cases but also verify that performance behave when there's a
>> multitude of input shapes. Both simple and mixed variants are interesting
>> diagnostic tools.
>> - Drop all charsets from the default run configuration except UTF-8.
>> Motivation: Defaults should give good coverage but try to keep runtimes at
>> bay. Additionally if the charset tested can't encode the higher codepoints
>> used in these micros the results might be misleading. If you for example
>> test using ISO-8859-1 the UTF-16 bytes in StringDecode.decodeUTF16 will have
>> all been replaced by `?`, so the test is effectively the same as testing
>> ASCII-only.
>
> Claes Redestad has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Brent review
test/micro/org/openjdk/bench/java/lang/StringDecode.java line 72:
> 70: public void setup() {
> 71: charset = Charset.forName(charsetName);
> 72: asciiString = LOREM.substring(0, 32).getBytes(charset);
This is problematic IMO in that it's missing short strings such as "Claes".
Average Java strings are about 32 bytes long AFAICR, and people writing
(vectorized) ijntrinsics have a nasty habit of optimizing for long strings, to
the detriment of typical-length ones.
Whether we like it or not, people will optimize for benchmarks, so it's
important that benchmark data is realistic. The shortest here is 15 bytes, as
far as I can see. I'd certainly include a short string of just a few bytes so
that intrinsics don't cause regressions in important cases.
-
PR: https://git.openjdk.java.net/jdk/pull/7516
Re: RFR: 8282047: Enhance StringDecode/Encode microbenchmarks [v2]
> Splitting out these micro changes from #7231 > > - Clean up and simplify setup and code > - Add variants with different inputs with varying lengths and encoding > weights, but also relevant mixes of each so that we both cover interesting > corner cases but also verify that performance behave when there's a multitude > of input shapes. Both simple and mixed variants are interesting diagnostic > tools. > - Drop all charsets from the default run configuration except UTF-8. > Motivation: Defaults should give good coverage but try to keep runtimes at > bay. Additionally if the charset tested can't encode the higher codepoints > used in these micros the results might be misleading. If you for example test > using ISO-8859-1 the UTF-16 bytes in StringDecode.decodeUTF16 will have all > been replaced by `?`, so the test is effectively the same as testing > ASCII-only. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Brent review - Changes: - all: https://git.openjdk.java.net/jdk/pull/7516/files - new: https://git.openjdk.java.net/jdk/pull/7516/files/54e62eda..0dd04498 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7516=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7516=00-01 Stats: 7 lines in 2 files changed: 5 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7516.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7516/head:pull/7516 PR: https://git.openjdk.java.net/jdk/pull/7516
Re: RFR: 8282047: Enhance StringDecode/Encode microbenchmarks
On Thu, 24 Feb 2022 19:01:32 GMT, Brent Christian wrote:
>> Splitting out these micro changes from #7231
>>
>> - Clean up and simplify setup and code
>> - Add variants with different inputs with varying lengths and encoding
>> weights, but also relevant mixes of each so that we both cover interesting
>> corner cases but also verify that performance behave when there's a
>> multitude of input shapes. Both simple and mixed variants are interesting
>> diagnostic tools.
>> - Drop all charsets from the default run configuration except UTF-8.
>> Motivation: Defaults should give good coverage but try to keep runtimes at
>> bay. Additionally if the charset tested can't encode the higher codepoints
>> used in these micros the results might be misleading. If you for example
>> test using ISO-8859-1 the UTF-16 bytes in StringDecode.decodeUTF16 will have
>> all been replaced by `?`, so the test is effectively the same as testing
>> ASCII-only.
>
> test/micro/org/openjdk/bench/java/lang/StringDecode.java line 93:
>
>> 91: public void decodeAsciiLong(Blackhole bh) throws Exception {
>> 92: bh.consume(new String(longAsciiString, charset));
>> 93: bh.consume(new String(longAsciiString, 0, 1024 + 31, charset));
>
> I imagine the 1024+31 addition gets compiled down, and is not executed during
> the test, right?
Yes, adding two integer literals will be constant folded already by javac:
public static void main(String...args) {
int foo = 1024 + 31;
}
javap -v output:
``` stack=1, locals=2, args_size=1
0: sipush1055
3: istore_1
4: return
-
PR: https://git.openjdk.java.net/jdk/pull/7516
Re: RFR: 8282023: PropertiesStoreTest and StoreReproducibilityTest jtreg failures due to en_CA locale [v5]
> Can I please get a review of this test only change which fixes the issue > noted in https://bugs.openjdk.java.net/browse/JDK-8282023? > > As noted in that JBS issue these tests fail when the default locale under > which those tests are run isn't `en_US`. Both these tests were introduced as > part of https://bugs.openjdk.java.net/browse/JDK-8231640. At a high level, > these test do the following: > - Use Properties.store(...) APIs to write out a properties file. This > internally ends up writing a date comment via a call to > `java.util.Date.toString()` method. > - The test then runs a few checks to make sure that the written out `Date` is > an expected one. To run these checks it uses the > `java.time.format.DateTimeFormatter` to parse the date comment out of the > properties file. > > All this works fine when the default locale is (for example) `en_US`. > However, when the default locale is (for example `en_CA` or even `hi_IN`) the > tests fail with an exception in the latter step above while parsing the date > comment using the `DateTimeFormatter` instance. The exception looks like: > > > Using locale: he for Properties#store(OutputStream) test > test PropertiesStoreTest.testStoreOutputStreamDateComment(he): failure > java.lang.AssertionError: Unexpected date comment: Mon Feb 21 19:10:31 IST > 2022 > at org.testng.Assert.fail(Assert.java:87) > at PropertiesStoreTest.testDateComment(PropertiesStoreTest.java:255) > at > PropertiesStoreTest.testStoreOutputStreamDateComment(PropertiesStoreTest.java:223) > at > java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104) > at java.base/java.lang.reflect.Method.invoke(Method.java:577) > at > org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:132) > at org.testng.internal.TestInvoker.invokeMethod(TestInvoker.java:599) > at > org.testng.internal.TestInvoker.invokeTestMethod(TestInvoker.java:174) > at org.testng.internal.MethodRunner.runInSequence(MethodRunner.java:46) > at > org.testng.internal.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:822) > at > org.testng.internal.TestInvoker.invokeTestMethods(TestInvoker.java:147) > at > org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:146) > at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:128) > at java.base/java.util.ArrayList.forEach(ArrayList.java:1511) > at org.testng.TestRunner.privateRun(TestRunner.java:764) > at org.testng.TestRunner.run(TestRunner.java:585) > at org.testng.SuiteRunner.runTest(SuiteRunner.java:384) > at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:378) > at org.testng.SuiteRunner.privateRun(SuiteRunner.java:337) > at org.testng.SuiteRunner.run(SuiteRunner.java:286) > at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:53) > at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:96) > at org.testng.TestNG.runSuitesSequentially(TestNG.java:1218) > at org.testng.TestNG.runSuitesLocally(TestNG.java:1140) > at org.testng.TestNG.runSuites(TestNG.java:1069) > at org.testng.TestNG.run(TestNG.java:1037) > at > com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:94) > at > com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:54) > at > java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104) > at java.base/java.lang.reflect.Method.invoke(Method.java:577) > at > com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127) > at java.base/java.lang.Thread.run(Thread.java:828) > Caused by: java.time.format.DateTimeParseException: Text 'Mon Feb 21 19:10:31 > IST 2022' could not be parsed at index 0 > at > java.base/java.time.format.DateTimeFormatter.parseResolved0(DateTimeFormatter.java:2106) > at > java.base/java.time.format.DateTimeFormatter.parse(DateTimeFormatter.java:1934) > at PropertiesStoreTest.testDateComment(PropertiesStoreTest.java:253) > ... 30 more > > (in the exception above a locale with language `he` is being used) > > The root cause of this failure lies (only) within the tests - the > `DateTimeFormatter` used in the tests is locale sensitive and uses the > current (`Locale.getDefault()`) locale by default for parsing the date text. > This parsing fails because, although `Date.toString()` javadoc states nothing > about locales, it does mention the exact characters that will be used to > write out the date comment. In other words, this date comment written out is > locale insensitive and as such when parsing using `DateTimeFormatter` a > neutral locale is appropriate on the `DateTimeFormatter` instance. This PR > thus changes the tests to use `Locale.ROOT` while parsing this date comment. >
Re: RFR: 8282219: jdk/java/lang/ProcessBuilder/Basic.java fails on AIX [v4]
On Thu, 24 Feb 2022 18:51:08 GMT, Roger Riggs wrote: >> Ichiroh Takiguchi has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Add null check > > The piece I was missing is that the HotSpot AIX specific code, computes and > sets LIBPATH based on > the path to the java executable. This computed LIBPATH value is set in the > environment unless > it is overridden by an existing LIBPATH in the environment. > > The test cases that failed, do not supply a value for LIBPATH so the launcher > provided value is inserted > and thereafter reported as the environment from the child process. > > Since both of these tests are testing a different condition not related to > LIBPATH, > the presence/accuracy of LIBPATH is not the point of the test. > > The current solution to remove `test.nativepath` works and is ok by me. > > (The alternative used by #7581, to include LIBPATH in the environment when > launching might be slightly more robust to future changes.) > > Thanks for the followup. Hello @RogerRiggs , @backwaterred . Could you give me your suggestion, please ? - PR: https://git.openjdk.java.net/jdk/pull/7574
