Re: RFR: 8292955: Collections.checkedMap Map.merge does not properly check key and value [v3]
On Thu, 7 Mar 2024 05:33:16 GMT, Korov wrote: >> When the specified key did not associated with a value, should check the >> `key` and `value` type. > > Korov has updated the pull request incrementally with one additional commit > since the last revision: > > Use testNG builtin functionalities and modify the test function name Looks good to me, maybe @stuart-marks can take a look too? - Marked as reviewed by liach (Author). PR Review: https://git.openjdk.org/jdk/pull/18141#pullrequestreview-1935750299
Re: RFR: 8292955: Collections.checkedMap Map.merge does not properly check key and value [v3]
On Thu, 7 Mar 2024 05:33:16 GMT, Korov wrote: >> When the specified key did not associated with a value, should check the >> `key` and `value` type. > > Korov has updated the pull request incrementally with one additional commit > since the last revision: > > Use testNG builtin functionalities and modify the test function name Hi @liach , Can you review my newly submitted code? - PR Comment: https://git.openjdk.org/jdk/pull/18141#issuecomment-1996550991
RFR: 8328066: WhiteBoxResizeTest failure on linux-x86: Could not reserve enough space for 2097152KB object heap
Can I please get a review of this test-only change which proposes to address https://bugs.openjdk.org/browse/JDK-8328066? The test launches a JVM with 2G heap (`-Xmx2G`) and as noted in that issue, the failure was observed on linux-86 instance on a GitHub jobs run. The commit in this PR proposes to add relevant `@requires` so that the test is only run on 64 bit VM and expects the `os.maxMemory` to be > 2G. The change has been tested on a linux-x86 run in GitHub actions job, plus even on local and CI 64 bit VM environments. No failures have been noticed. - Commit messages: - 8328066: WhiteBoxResizeTest failure on linux-x86: Could not reserve enough space for 2097152KB object heap Changes: https://git.openjdk.org/jdk/pull/18290/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18290&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8328066 Stats: 4 lines in 1 file changed: 3 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18290.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18290/head:pull/18290 PR: https://git.openjdk.org/jdk/pull/18290
Re: RFR: 8327791: Optimization for new BigDecimal(String) [v13]
On Wed, 13 Mar 2024 16:02:29 GMT, Chen Liang wrote: >> Shaojin Wen has updated the pull request incrementally with one additional >> commit since the last revision: >> >> bug fix for CharArraySequence > > src/java.base/share/classes/java/math/BigDecimal.java line 561: > >> 559: index += offset; >> 560: if (index >= length) >> 561: throw new IndexOutOfBoundsException(); > > This logic is wrong: if offset is 3 and length is 2, aab*bc*c would be valid, > but your code will IOOBE on `charAt(0)` because `index += offset` will be 3, > 3 > 2. > > You should use `Objects.checkIndex(index, length)` instead. Oh, what a stupid mistake :) Using Objects.checkIndex will have a redundant check for index < 0, which does not need to be done in this scenario. Objects.checkIndex will cause a slight performance degradation. The performance numbers under MacBookPro M1 Max are as follows: Benchmark Mode Cnt Score Error Units BigDecimals.testConstructorWithSmallCharArray avgt 15 19.167 ? 0.070 ns/op Benchmark Mode Cnt Score Error Units # Objects.checkIndex BigDecimals.testConstructorWithSmallCharArray avgt 15 20.591 ? 0.241 ns/op - PR Review Comment: https://git.openjdk.org/jdk/pull/18177#discussion_r1524049182
Re: RFR: 8327791: Optimization for new BigDecimal(String) [v14]
> The current BigDecimal(String) constructor calls String#toCharArray, which
> has a memory allocation.
>
>
> public BigDecimal(String val) {
> this(val.toCharArray(), 0, val.length()); // allocate char[]
> }
>
>
> When the length is greater than 18, create a char[]
>
>
> boolean isCompact = (len <= MAX_COMPACT_DIGITS); // 18
> if (!isCompact) {
> // ...
> } else {
> char[] coeff = new char[len]; // allocate char[]
> // ...
> }
>
>
> This PR eliminates the two memory allocations mentioned above, resulting in
> an approximate 60% increase in performance..
Shaojin Wen has updated the pull request incrementally with one additional
commit since the last revision:
bug fix for CharArraySequence#charAt
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18177/files
- new: https://git.openjdk.org/jdk/pull/18177/files/f3084ba5..69be44db
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=18177&range=13
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=18177&range=12-13
Stats: 8 lines in 2 files changed: 3 ins; 4 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/18177.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18177/head:pull/18177
PR: https://git.openjdk.org/jdk/pull/18177
Re: RFR: 8327242: Document supported CLDR versions in the javadoc [v4]
On Tue, 12 Mar 2024 18:47:37 GMT, Naoto Sato wrote: > Will let the doc people know. It is at least good that the latest doc for > JDK21 is correct. Now the previous releases' supported locales docs have been updated. - PR Comment: https://git.openjdk.org/jdk/pull/18243#issuecomment-1996071016
Re: RFR: 8328037: Test java/util/Formatter/Padding.java has unnecessary high heap requirement after JDK-8326718 [v3]
> 4f336085d1098e7fba7b58f0a73c028179a2a13d > ([JDK-8326718](https://bugs.openjdk.org/browse/JDK-8326718)) added a few > cases to test java/util/Formatter/Padding.java with huge Strings as > arguments. Since all possible argument combinations for the test are stored > in one array, nothing can be garbage collected while the test is running and > the heap requirement is blown up. > > In one of our test pipelines we run tier1 tests with VMs that default to 384M > of heap and this is not sufficient any longer. > > I'm improving this by splitting the one large @ParameterizedTest into > multiple ones. With that, I could run the test successfully in a test VM with > 96M of heap, e.g. by modifying `@run junit Padding` to `@run junit/othervm > -Xmx96m Padding` Christoph Langer has updated the pull request incrementally with one additional commit since the last revision: Review suggestions - Changes: - all: https://git.openjdk.org/jdk/pull/18264/files - new: https://git.openjdk.org/jdk/pull/18264/files/be84e5e8..ca3ec0d5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18264&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18264&range=01-02 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/18264.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18264/head:pull/18264 PR: https://git.openjdk.org/jdk/pull/18264
Re: RFR: 8328037: Test java/util/Formatter/Padding.java has unnecessary high heap requirement after JDK-8326718 [v2]
On Wed, 13 Mar 2024 14:23:01 GMT, Raffaello Giulietti wrote: > Can you please add the bug id to `@bug` and correct the typo, as suggested > [here](https://github.com/openjdk/jdk/pull/18264#issuecomment-1994382507)? Done. I kept the tenMillion... handling. - PR Comment: https://git.openjdk.org/jdk/pull/18264#issuecomment-1995807615
Re: RFR: 8327180: Failed: java/io/ObjectStreamClass/ObjectStreamClassCaching.java#G1 [v3]
On Wed, 13 Mar 2024 20:10:25 GMT, Roger Riggs wrote: >> The intermittent failure of ObjectStreamClassCaching is due to an incorrect >> assumption about GC behavior and a race condition. >> >> Removed test based on incorrect assumptions about simultaneous clearing of >> WeakReferences. >> Added a run of the test using ZGC, (previously omitted) > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Use the correct ZGenerational collector Marked as reviewed by iris (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18284#pullrequestreview-1935176824
Re: RFR: 8327180: Failed: java/io/ObjectStreamClass/ObjectStreamClassCaching.java#G1 [v3]
On Wed, 13 Mar 2024 20:10:25 GMT, Roger Riggs wrote: >> The intermittent failure of ObjectStreamClassCaching is due to an incorrect >> assumption about GC behavior and a race condition. >> >> Removed test based on incorrect assumptions about simultaneous clearing of >> WeakReferences. >> Added a run of the test using ZGC, (previously omitted) > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Use the correct ZGenerational collector Looks good. Make sure that the test has run after the -XX:+ZGenerational flag was added. - Marked as reviewed by stefank (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18284#pullrequestreview-1935146818
Integrated: 8327242: Document supported CLDR versions in the javadoc
On Tue, 12 Mar 2024 16:26:48 GMT, Naoto Sato wrote: > Adding a table that maps JDK versions and corresponding CLDR releases as an > implNote. A draft CSR has also been created. This pull request has now been integrated. Changeset: 7f6b7ebb Author:Naoto Sato URL: https://git.openjdk.org/jdk/commit/7f6b7ebbcc49d8023e669568c38cd301bb795983 Stats: 56 lines in 1 file changed: 48 ins; 0 del; 8 mod 8327242: Document supported CLDR versions in the javadoc Reviewed-by: joehw, iris, jlu - PR: https://git.openjdk.org/jdk/pull/18243
Re: RFR: 8325621: Improve jspawnhelper version checks [v4]
On Wed, 13 Mar 2024 17:11:25 GMT, Chad Rakoczy wrote: >> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621) >> >> Updates jspawnhelper to check that JDK version and jspawnhelper version are >> the same. Updates test to include check for version. Also tested manually by >> replacing jspawnhelper with incorrect version to confirm that check works. > > Chad Rakoczy has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 10 commits: > > - Merge branch 'master' into JDK-8325621-2 > - Update style and improve error messages > - Print to stdout instead of stderr > - Compare version using VERSION_STRING > - Code cleanup > - Remove string.h include > - Update test > - Pass version as integer instead of string > - Read in version using int instead of string > - 8325621: Improve jspawnhelper version checks Build changes look good now. - Marked as reviewed by ihse (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18204#pullrequestreview-1935108939
Re: RFR: 8327180: Failed: java/io/ObjectStreamClass/ObjectStreamClassCaching.java#G1 [v3]
> The intermittent failure of ObjectStreamClassCaching is due to an incorrect > assumption about GC behavior and a race condition. > > Removed test based on incorrect assumptions about simultaneous clearing of > WeakReferences. > Added a run of the test using ZGC, (previously omitted) Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Use the correct ZGenerational collector - Changes: - all: https://git.openjdk.org/jdk/pull/18284/files - new: https://git.openjdk.org/jdk/pull/18284/files/b94fe6d5..55960c41 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18284&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18284&range=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18284.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18284/head:pull/18284 PR: https://git.openjdk.org/jdk/pull/18284
Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v6]
On Tue, 5 Mar 2024 19:56:58 GMT, Weijun Wang wrote: >> This code change adds an alternative implementation of user-based >> authorization `Subject` APIs that doesn't depend on Security Manager APIs. >> Depending on if the Security Manager is allowed, the methods store the >> current subject differently. See the spec change in the `Subject.java` file >> for details. When the Security Manager APIs are finally removed in a future >> release, this new implementation will be only implementation for these >> methods. >> >> One major change in the new implementation is that `Subject.getSubject` >> always throws an `UnsupportedOperationException` since it has an >> `AccessControlContext` argument but the current subject is no longer >> associated with an `AccessControlContext` object. >> >> Now it's the time to migrate from the `getSubject` and `doAs` methods to >> `current` and `callAs`. If the user application is simply calling >> `getSubject(AccessController.getContext())`, then switching to `current()` >> would work. If the `AccessControlContext` argument is retrieved from an >> earlier `getContext()` call and the associated subject might be different >> from that of the current `AccessControlContext`, then instead of storing the >> previous `AccessControlContext` object and passing it into `getSubject` to >> get the "previous" subject, the application should store the `current()` >> return value directly. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > revert changes to MBeanServerFileAccessController.java test/jdk/javax/security/auth/Subject/CallAsWithScopedValue.java line 29: > 27: * @enablePreview > 28: * @summary Implement Subject.current and Subject.callAs using scoped > values. > 29: * Need @enablePreview to use StructuredTaskScope. jtreg throws a `ParseException` because I think it tries to interpret it as an `@enablePreview` action. Suggest enclosing it in double quotes. - PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r152383
Re: RFR: 8327180: Failed: java/io/ObjectStreamClass/ObjectStreamClassCaching.java#G1 [v2]
On Wed, 13 Mar 2024 19:41:25 GMT, Roger Riggs wrote: >> The intermittent failure of ObjectStreamClassCaching is due to an incorrect >> assumption about GC behavior and a race condition. >> >> Removed test based on incorrect assumptions about simultaneous clearing of >> WeakReferences. >> Added a run of the test using ZGC, (previously omitted) > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Remove obsolete note; no longer disabled Changes requested by stefank (Reviewer). test/jdk/java/io/ObjectStreamClass/ObjectStreamClassCaching.java line 57: > 55: * @library /test/lib/ > 56: * @summary ObjectStreamClass caches keep ClassLoaders alive (ZGC) > 57: * @run testng/othervm -Xmx64m -XX:+UseZGC ObjectStreamClassCaching This test is meant to run with Generational ZGC (according to the requires line). It needs to run with `-XX:+UseZGC -XX:+ZGenerational` to enable Generational ZGC. If you run with `-XX:+UseZGC` you get the older, non-generational ZGC. - PR Review: https://git.openjdk.org/jdk/pull/18284#pullrequestreview-1935079550 PR Review Comment: https://git.openjdk.org/jdk/pull/18284#discussion_r1523833917
Re: RFR: 8325621: Improve jspawnhelper version checks [v4]
On Wed, 13 Mar 2024 17:11:25 GMT, Chad Rakoczy wrote: >> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621) >> >> Updates jspawnhelper to check that JDK version and jspawnhelper version are >> the same. Updates test to include check for version. Also tested manually by >> replacing jspawnhelper with incorrect version to confirm that check works. > > Chad Rakoczy has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 10 commits: > > - Merge branch 'master' into JDK-8325621-2 > - Update style and improve error messages > - Print to stdout instead of stderr > - Compare version using VERSION_STRING > - Code cleanup > - Remove string.h include > - Update test > - Pass version as integer instead of string > - Read in version using int instead of string > - 8325621: Improve jspawnhelper version checks Marked as reviewed by rriggs (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18204#pullrequestreview-1935071268
Re: RFR: 8327180: Failed: java/io/ObjectStreamClass/ObjectStreamClassCaching.java#G1 [v2]
> The intermittent failure of ObjectStreamClassCaching is due to an incorrect > assumption about GC behavior and a race condition. > > Removed test based on incorrect assumptions about simultaneous clearing of > WeakReferences. > Added a run of the test using ZGC, (previously omitted) Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Remove obsolete note; no longer disabled - Changes: - all: https://git.openjdk.org/jdk/pull/18284/files - new: https://git.openjdk.org/jdk/pull/18284/files/de516a24..b94fe6d5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18284&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18284&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18284.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18284/head:pull/18284 PR: https://git.openjdk.org/jdk/pull/18284
Re: RFR: 8327180: Failed: java/io/ObjectStreamClass/ObjectStreamClassCaching.java#G1
On Wed, 13 Mar 2024 18:01:21 GMT, Roger Riggs wrote: > The intermittent failure of ObjectStreamClassCaching is due to an incorrect > assumption about GC behavior and a race condition. > > Removed test based on incorrect assumptions about simultaneous clearing of > WeakReferences. > Added a run of the test using ZGC, (previously omitted) Marked as reviewed by iris (Reviewer). test/jdk/java/io/ObjectStreamClass/ObjectStreamClassCaching.java line 53: > 51: /* > 52: * Disabled for ZGC Generational. > 53: * TODO: Find correct appropriate solution to the flakiness of this test. Shouldn't you also remove the comment at line 52 since this test is no longer disabled? - PR Review: https://git.openjdk.org/jdk/pull/18284#pullrequestreview-1934946970 PR Review Comment: https://git.openjdk.org/jdk/pull/18284#discussion_r1523749769
RFR: 8327180: Failed: java/io/ObjectStreamClass/ObjectStreamClassCaching.java#G1
The intermittent failure of ObjectStreamClassCaching is due to an incorrect assumption about GC behavior and a race condition. Removed test based on incorrect assumptions about simultaneous clearing of WeakReferences. Added a run of the test using ZGC, (previously omitted) - Commit messages: - 8327180: Failed: java/io/ObjectStreamClass/ObjectStreamClassCaching.java#G1 failed with CONF=release -XX:ReservedCodeCacheSize=8m Changes: https://git.openjdk.org/jdk/pull/18284/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18284&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8327180 Stats: 36 lines in 1 file changed: 5 ins; 28 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/18284.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18284/head:pull/18284 PR: https://git.openjdk.org/jdk/pull/18284
Re: RFR: 8327242: Document supported CLDR versions in the javadoc [v4]
On Wed, 13 Mar 2024 16:06:26 GMT, Naoto Sato wrote: >> Adding a table that maps JDK versions and corresponding CLDR releases as an >> implNote. A draft CSR has also been created. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Further refinement Marked as reviewed by iris (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18243#pullrequestreview-1934843256
Re: RFR: 8325621: Improve jspawnhelper version checks [v4]
On Wed, 13 Mar 2024 17:11:25 GMT, Chad Rakoczy wrote: >> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621) >> >> Updates jspawnhelper to check that JDK version and jspawnhelper version are >> the same. Updates test to include check for version. Also tested manually by >> replacing jspawnhelper with incorrect version to confirm that check works. > > Chad Rakoczy has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 10 commits: > > - Merge branch 'master' into JDK-8325621-2 > - Update style and improve error messages > - Print to stdout instead of stderr > - Compare version using VERSION_STRING > - Code cleanup > - Remove string.h include > - Update test > - Pass version as integer instead of string > - Read in version using int instead of string > - 8325621: Improve jspawnhelper version checks I am good with this version, thanks! - Marked as reviewed by shade (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18204#pullrequestreview-1934831130
Re: RFR: 8325621: Improve jspawnhelper version checks [v4]
> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621) > > Updates jspawnhelper to check that JDK version and jspawnhelper version are > the same. Updates test to include check for version. Also tested manually by > replacing jspawnhelper with incorrect version to confirm that check works. Chad Rakoczy has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 10 commits: - Merge branch 'master' into JDK-8325621-2 - Update style and improve error messages - Print to stdout instead of stderr - Compare version using VERSION_STRING - Code cleanup - Remove string.h include - Update test - Pass version as integer instead of string - Read in version using int instead of string - 8325621: Improve jspawnhelper version checks - Changes: https://git.openjdk.org/jdk/pull/18204/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18204&range=03 Stats: 59 lines in 5 files changed: 47 ins; 4 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/18204.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18204/head:pull/18204 PR: https://git.openjdk.org/jdk/pull/18204
Re: RFR: 8327242: Document supported CLDR versions in the javadoc [v4]
> Adding a table that maps JDK versions and corresponding CLDR releases as an > implNote. A draft CSR has also been created. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Further refinement - Changes: - all: https://git.openjdk.org/jdk/pull/18243/files - new: https://git.openjdk.org/jdk/pull/18243/files/21e425ab..e1a5783a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18243&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18243&range=02-03 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18243.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18243/head:pull/18243 PR: https://git.openjdk.org/jdk/pull/18243
Re: RFR: 8327791: Optimization for new BigDecimal(String) [v13]
On Wed, 13 Mar 2024 15:46:30 GMT, Shaojin Wen wrote:
>> The current BigDecimal(String) constructor calls String#toCharArray, which
>> has a memory allocation.
>>
>>
>> public BigDecimal(String val) {
>> this(val.toCharArray(), 0, val.length()); // allocate char[]
>> }
>>
>>
>> When the length is greater than 18, create a char[]
>>
>>
>> boolean isCompact = (len <= MAX_COMPACT_DIGITS); // 18
>> if (!isCompact) {
>> // ...
>> } else {
>> char[] coeff = new char[len]; // allocate char[]
>> // ...
>> }
>>
>>
>> This PR eliminates the two memory allocations mentioned above, resulting in
>> an approximate 60% increase in performance..
>
> Shaojin Wen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> bug fix for CharArraySequence
src/java.base/share/classes/java/math/BigDecimal.java line 561:
> 559: index += offset;
> 560: if (index >= length)
> 561: throw new IndexOutOfBoundsException();
This logic is wrong: if offset is 3 and length is 2, aab*bc*c would be valid,
but your code will IOOBE on `charAt(0)` because `index += offset` will be 3, 3
> 2.
You should use `Objects.checkIndex(index, length)` instead.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18177#discussion_r1523540101
Re: RFR: 8327791: Optimization for new BigDecimal(String) [v12]
On Wed, 13 Mar 2024 13:51:27 GMT, Claes Redestad wrote:
> Relying on the upper bounds check of `charAt` doesn't work well with the
> `CharArraySequence` whose `charAt` deliberately does not throw an IIOBE if
> the array is longer than the provided length, ie, it'll look at chars beyond
> the provided range. The examples I tested still end up as a NFE, but it's
> clear from the cause that we're running past the length:
>
> ```
> jshell> new BigDecimal(new char[] { '-', '1', 'e'}, 0, 1);
> | Exception java.lang.NumberFormatException
> |at BigDecimal. (BigDecimal.java:754)
> |at BigDecimal. (BigDecimal.java:543)
> |at BigDecimal. (BigDecimal.java:518)
> |at (#4:1)
> | Caused by: java.lang.ArrayIndexOutOfBoundsException: Index 3 out of bounds
> for length 3
> |at BigDecimal$CharArraySequence.charAt (BigDecimal.java:559)
> |at BigDecimal.parseExp (BigDecimal.java:772)
> |at BigDecimal. (BigDecimal.java:619)
> |...
> ```
>
> Baseline/expected:
>
> ```
> jshell> new BigDecimal(new char[] { '-', '1', 'e'}, 0, 1);
> | Exception java.lang.NumberFormatException: No digits found.
> |at BigDecimal. (BigDecimal.java:635)
> |at BigDecimal. (BigDecimal.java:518)
> |at (#1:1)
> ```
>
> Having a check on `len > 0` is more robust - and I'd be surprised if avoiding
> a redundant check on the loop entry is affecting performance?
If the likely error/boundary conditions change, those changed conditions should
be added to the regression tests.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18177#discussion_r1523509168
Re: RFR: 8327791: Optimization for new BigDecimal(String) [v13]
> The current BigDecimal(String) constructor calls String#toCharArray, which
> has a memory allocation.
>
>
> public BigDecimal(String val) {
> this(val.toCharArray(), 0, val.length()); // allocate char[]
> }
>
>
> When the length is greater than 18, create a char[]
>
>
> boolean isCompact = (len <= MAX_COMPACT_DIGITS); // 18
> if (!isCompact) {
> // ...
> } else {
> char[] coeff = new char[len]; // allocate char[]
> // ...
> }
>
>
> This PR eliminates the two memory allocations mentioned above, resulting in
> an approximate 60% increase in performance..
Shaojin Wen has updated the pull request incrementally with one additional
commit since the last revision:
bug fix for CharArraySequence
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18177/files
- new: https://git.openjdk.org/jdk/pull/18177/files/be2119c4..f3084ba5
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=18177&range=12
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=18177&range=11-12
Stats: 29 lines in 2 files changed: 28 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/18177.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18177/head:pull/18177
PR: https://git.openjdk.org/jdk/pull/18177
Re: RFR: 8328037: Test java/util/Formatter/Padding.java has unnecessary high heap requirement after JDK-8326718 [v2]
On Wed, 13 Mar 2024 14:10:29 GMT, Christoph Langer wrote: >> 4f336085d1098e7fba7b58f0a73c028179a2a13d >> ([JDK-8326718](https://bugs.openjdk.org/browse/JDK-8326718)) added a few >> cases to test java/util/Formatter/Padding.java with huge Strings as >> arguments. Since all possible argument combinations for the test are stored >> in one array, nothing can be garbage collected while the test is running and >> the heap requirement is blown up. >> >> In one of our test pipelines we run tier1 tests with VMs that default to >> 384M of heap and this is not sufficient any longer. >> >> I'm improving this by splitting the one large @ParameterizedTest into >> multiple ones. With that, I could run the test successfully in a test VM >> with 96M of heap, e.g. by modifying `@run junit Padding` to `@run >> junit/othervm -Xmx96m Padding` > > Christoph Langer has updated the pull request incrementally with one > additional commit since the last revision: > > Generate large strings in parameter generator methods I was thinking more something like var tenMillionBlanks = tenMillionBlanks(); and similarly for `tenMillionsZeros`, thus maintaining the `private static` (but not `final` ;-) ) methods as in your previous commit. But if you are happy with the last commit, it's OK as well. Can you please add the bug id to `@bug` and correct the typo, as suggested [here](https://github.com/openjdk/jdk/pull/18264#issuecomment-1994382507)? Otherwise looks fine. - PR Comment: https://git.openjdk.org/jdk/pull/18264#issuecomment-1994522269
Re: RFR: 8328037: Test java/util/Formatter/Padding.java has unnecessary high heap requirement after JDK-8326718 [v2]
> 4f336085d1098e7fba7b58f0a73c028179a2a13d > ([JDK-8326718](https://bugs.openjdk.org/browse/JDK-8326718)) added a few > cases to test java/util/Formatter/Padding.java with huge Strings as > arguments. Since all possible argument combinations for the test are stored > in one array, nothing can be garbage collected while the test is running and > the heap requirement is blown up. > > In one of our test pipelines we run tier1 tests with VMs that default to 384M > of heap and this is not sufficient any longer. > > I'm improving this by splitting the one large @ParameterizedTest into > multiple ones. With that, I could run the test successfully in a test VM with > 96M of heap, e.g. by modifying `@run junit Padding` to `@run junit/othervm > -Xmx96m Padding` Christoph Langer has updated the pull request incrementally with one additional commit since the last revision: Generate large strings in parameter generator methods - Changes: - all: https://git.openjdk.org/jdk/pull/18264/files - new: https://git.openjdk.org/jdk/pull/18264/files/8e3e9509..be84e5e8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18264&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18264&range=00-01 Stats: 53 lines in 1 file changed: 9 ins; 8 del; 36 mod Patch: https://git.openjdk.org/jdk/pull/18264.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18264/head:pull/18264 PR: https://git.openjdk.org/jdk/pull/18264
Re: RFR: 8328037: Test java/util/Formatter/Padding.java has unnecessary high heap requirement after JDK-8326718
On Wed, 13 Mar 2024 09:42:34 GMT, Raffaello Giulietti wrote: > What about factoring out the 4 invocations of `tenMillionBlanks()` in each > source method in a local var? OK, I inlined the generation of the ten million character strings into the parameter generator methods. I looked a bit at the test runtime and it seems like it doesn't make a lot of difference in a test JVM with larger heap but for smaller test VMs it seems to improve things. - PR Comment: https://git.openjdk.org/jdk/pull/18264#issuecomment-1994497012
Re: RFR: 8327791: Optimization for new BigDecimal(String) [v12]
On Wed, 13 Mar 2024 13:12:54 GMT, Shaojin Wen wrote:
>> If the input is "+" or "-" an exception will be thrown on line 583
>>
>> boolean isneg = c == '-'; // leading minus means negative
>> if (isneg || c == '+') {
>> c = val.charAt(++offset);
>> len--;
>> }
>
> if the input is "" an exception will be throw on line 579
>
> int offset = 0;
> char c = val.charAt(offset);
Relying on the upper bounds check of `charAt` doesn't work well with the
`CharArraySequence` whose `charAt` deliberately does not throw an IIOBE if the
array is longer than the provided length, ie, it'll look at chars beyond the
provided range. The examples I tested still end up as a NFE, but it's clear
from the cause that we're running past the length:
jshell> new BigDecimal(new char[] { '-', '1', 'e'}, 0, 1);
| Exception java.lang.NumberFormatException
|at BigDecimal. (BigDecimal.java:754)
|at BigDecimal. (BigDecimal.java:543)
|at BigDecimal. (BigDecimal.java:518)
|at (#4:1)
| Caused by: java.lang.ArrayIndexOutOfBoundsException: Index 3 out of bounds
for length 3
|at BigDecimal$CharArraySequence.charAt (BigDecimal.java:559)
|at BigDecimal.parseExp (BigDecimal.java:772)
|at BigDecimal. (BigDecimal.java:619)
|...
Baseline/expected:
jshell> new BigDecimal(new char[] { '-', '1', 'e'}, 0, 1);
| Exception java.lang.NumberFormatException: No digits found.
|at BigDecimal. (BigDecimal.java:635)
|at BigDecimal. (BigDecimal.java:518)
|at (#1:1)
Having a check on `len > 0` is more robust - and I'd be surprised if avoiding a
redundant check on the loop entry is affecting performance?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18177#discussion_r1523301252
Re: RFR: 8328074: Add jcheck whitespace checking for assembly files [v4]
On Wed, 13 Mar 2024 11:59:36 GMT, Magnus Ihse Bursie wrote: >> As part of the ongoing effort to enable jcheck whitespace checking to all >> text files, it is now time to address assembly (.S) files. The hotspot >> assembly files were fixed as part of the hotspot mapfile removal, so only a >> single incorrect jdk library remains. >> >> The main issue with `libjsvml` was inconsistent line starts, sometimes using >> tabs. I used the `expand` unix command line tool to replace these with >> spaces. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Make all ALIGN/.align aligned Marked as reviewed by erikj (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18268#pullrequestreview-1934219694
Re: RFR: 8327998: Enable java/lang/ProcessBuilder/JspawnhelperProtocol.java on Mac
On Tue, 12 Mar 2024 21:52:31 GMT, Elif Aslan wrote: > This change enables to run JspawnhelperProtocol.java on MacOS. > > In addition to GHA , the test has been run on macos and linux. > > > Test report is stored in > build/macosx-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java > > == > Test summary > == >TEST TOTAL PASS FAIL ERROR >jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java > 1 1 0 0 > == > TEST SUCCESS > > Finished building target 'test' in configuration > 'macosx-x86_64-server-fastdebug' > > > > > > Test report is stored in > build/linux-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java > > == > Test summary > == >TEST TOTAL PASS FAIL ERROR >jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java > 1 1 0 0 > == > TEST SUCCESS > > Stopping javac server > [ec2-user@ip-172-16-0-10 jdk]$ make test CONF=linux-x86_64-server-fastdebug > TEST=test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java LGTM Just a reminder to keep that the PR description is copied into every email sent so it should be concise. Extended descriptions and additional details should be added in a separate comment. - Marked as reviewed by rriggs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18253#pullrequestreview-1934219262
Re: RFR: 8328037: Test java/util/Formatter/Padding.java has unnecessary high heap requirement after JDK-8326718
On Wed, 13 Mar 2024 07:53:30 GMT, Christoph Langer wrote:
> 4f336085d1098e7fba7b58f0a73c028179a2a13d
> ([JDK-8326718](https://bugs.openjdk.org/browse/JDK-8326718)) added a few
> cases to test java/util/Formatter/Padding.java with huge Strings as
> arguments. Since all possible argument combinations for the test are stored
> in one array, nothing can be garbage collected while the test is running and
> the heap requirement is blown up.
>
> In one of our test pipelines we run tier1 tests with VMs that default to 384M
> of heap and this is not sufficient any longer.
>
> I'm improving this by splitting the one large @ParameterizedTest into
> multiple ones. With that, I could run the test successfully in a test VM with
> 96M of heap, e.g. by modifying `@run junit Padding` to `@run junit/othervm
> -Xmx96m Padding`
L.26-27 to
* @bug 4906370 8299677 8326718 8328037
* @summary Tests to exercise padding on int and double values,
test/jdk/java/util/Formatter/Padding.java line 43:
> 41: public class Padding {
> 42:
> 43: private static final String tenMillionZeros() {
Suggestion:
private static String tenMillionZeros() {
test/jdk/java/util/Formatter/Padding.java line 46:
> 44: return "0".repeat(10_000_000);
> 45: }
> 46: private static final String tenMillionBlanks() {
Suggestion:
private static String tenMillionBlanks() {
-
PR Comment: https://git.openjdk.org/jdk/pull/18264#issuecomment-1994382507
PR Review Comment: https://git.openjdk.org/jdk/pull/18264#discussion_r1523241759
PR Review Comment: https://git.openjdk.org/jdk/pull/18264#discussion_r1523241920
Re: RFR: 8327791: Optimization for new BigDecimal(String) [v12]
On Wed, 13 Mar 2024 12:12:20 GMT, Claes Redestad wrote:
>> Shaojin Wen has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fix benchmark
>
> src/java.base/share/classes/java/math/BigDecimal.java line 596:
>
>> 594: // First compact case, we need not to preserve the
>> character
>> 595: // and we can just compute the value in place.
>> 596: for (; ; c = val.charAt(++offset)) {
>
> This looks gnarly, and it's unclear if it's correct for invalid inputs like
> `""`, `"-"` and `"+"` since you're not testing for `len > 0` before going
> into the loop. Can you make sure there are tests covering such inputs and try
> to simplify this?
If the input is "+" or "-" an exception will be thrown on line 583
boolean isneg = c == '-'; // leading minus means negative
if (isneg || c == '+') {
c = val.charAt(++offset);
len--;
}
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18177#discussion_r1523235244
Re: RFR: 8327791: Optimization for new BigDecimal(String) [v12]
On Wed, 13 Mar 2024 13:11:48 GMT, Shaojin Wen wrote:
>> src/java.base/share/classes/java/math/BigDecimal.java line 596:
>>
>>> 594: // First compact case, we need not to preserve the
>>> character
>>> 595: // and we can just compute the value in place.
>>> 596: for (; ; c = val.charAt(++offset)) {
>>
>> This looks gnarly, and it's unclear if it's correct for invalid inputs like
>> `""`, `"-"` and `"+"` since you're not testing for `len > 0` before going
>> into the loop. Can you make sure there are tests covering such inputs and
>> try to simplify this?
>
> If the input is "+" or "-" an exception will be thrown on line 583
>
> boolean isneg = c == '-'; // leading minus means negative
> if (isneg || c == '+') {
> c = val.charAt(++offset);
> len--;
> }
if the input is "" an exception will be throw on line 579
int offset = 0;
char c = val.charAt(offset);
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18177#discussion_r1523237018
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v4]
On Tue, 27 Feb 2024 11:19:59 GMT, Magnus Ihse Bursie wrote: >> The idea of setting up general "toolchains" in the native build was good, >> but it turned out that we really only need a single toolchain, with a single >> twist: if it should use CC or CPP to link. This is better described by a >> specific argument to SetupNativeCompilation, LANG := C++ or LANG := C (the >> default). >> >> There is a single exception to this rule, and that is if we want to compile >> for the build platform rather than the target platform. (This is only done >> for adlc) To keep expressing this difference, introduce TARGET_TYPE := BUILD >> (or TARGET_TYPE := TARGET, the default). >> >> The final odd-case was the hack for building hsdis/bin on mingw. This can be >> resolved using direct variables to SetupNativeCompilation, instead of first >> creating a toolchain. >> >> Doing this refactoring will simplify the SetupNativeCompilation code, and >> make it much clearer if we use the C or C++ linker. > > Magnus Ihse Bursie has updated the pull request with a new target base due to > a merge or a rebase. The pull request now contains five commits: > > - Merge branch 'master' into remove-toolchain-define > - Rename LANG to LINK_TYPE > - Reword "lib" comment to fit in better > - Merge branch 'master' into remove-toolchain-define > - 8326583: Remove over-generalized DefineNativeToolchain solution I have opened https://bugs.openjdk.org/browse/JDK-8328079 to track the ccache regression. - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1994283777
RFR: 8327994: Update code gen in CallGeneratorHelper
Update the code gen code in CallGeneratorHelper to reflect the latest state of the libTest(Downcall/Upcall)(Stack).c and shared.h files. - The previous code wanted users to pipe stdout into a file. But, since we have 5 files that need to be created, and the names of those files is important too, I've changed the script to write to those files directly instead. - I moved the definition of `S_` to libVarArgs.c where it's used, as it's not actually generated by CallGeneratorHelper. - GitHub doesn't render the changes to some of the files, but those files only contain either white space changes (some missing spaces after `,`), and copyright header updates. - Commit messages: - add more run instructions - simplify - use export.h - factor out some shared code - fix test file generation - remove dead code from CallGeneratorHelper Changes: https://git.openjdk.org/jdk/pull/18269/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18269&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8327994 Stats: 36078 lines in 6 files changed: 62 ins; 40 del; 35976 mod Patch: https://git.openjdk.org/jdk/pull/18269.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18269/head:pull/18269 PR: https://git.openjdk.org/jdk/pull/18269
Re: RFR: 8327791: Optimization for new BigDecimal(String) [v12]
On Wed, 13 Mar 2024 09:52:45 GMT, Shaojin Wen wrote:
>> The current BigDecimal(String) constructor calls String#toCharArray, which
>> has a memory allocation.
>>
>>
>> public BigDecimal(String val) {
>> this(val.toCharArray(), 0, val.length()); // allocate char[]
>> }
>>
>>
>> When the length is greater than 18, create a char[]
>>
>>
>> boolean isCompact = (len <= MAX_COMPACT_DIGITS); // 18
>> if (!isCompact) {
>> // ...
>> } else {
>> char[] coeff = new char[len]; // allocate char[]
>> // ...
>> }
>>
>>
>> This PR eliminates the two memory allocations mentioned above, resulting in
>> an approximate 60% increase in performance..
>
> Shaojin Wen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fix benchmark
src/java.base/share/classes/java/math/BigDecimal.java line 596:
> 594: // First compact case, we need not to preserve the
> character
> 595: // and we can just compute the value in place.
> 596: for (; ; c = val.charAt(++offset)) {
This looks gnarly, and it's unclear if it's correct for invalid inputs like
`""`, `"-"` and `"+"` since you're not testing for `len > 0` before going into
the loop. Can you make sure there are tests covering such inputs and try to
simplify this?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18177#discussion_r1523115595
Re: RFR: 8328074: Add jcheck whitespace checking for assembly files [v3]
On Wed, 13 Mar 2024 11:45:33 GMT, Magnus Ihse Bursie wrote: >> As part of the ongoing effort to enable jcheck whitespace checking to all >> text files, it is now time to address assembly (.S) files. The hotspot >> assembly files were fixed as part of the hotspot mapfile removal, so only a >> single incorrect jdk library remains. >> >> The main issue with `libjsvml` was inconsistent line starts, sometimes using >> tabs. I used the `expand` unix command line tool to replace these with >> spaces. > > Magnus Ihse Bursie has refreshed the contents of this pull request, and > previous commits have been removed. Incremental views are not available. The > pull request now contains two commits: > > - Enable jcheck whitespace checks for .S files > - Run expand on libjsvml I tagged this `core-libs` as that is what @sviswa7 did for changes to libjsvml in https://github.com/openjdk/jdk/pull/8508. I hope this is correct. (We should really add rules to Skara for this library.) - PR Comment: https://git.openjdk.org/jdk/pull/18268#issuecomment-1994211386
Re: RFR: 8328074: Add jcheck whitespace checking for assembly files [v3]
On Wed, 13 Mar 2024 11:26:14 GMT, Julian Waters wrote: >> Magnus Ihse Bursie has refreshed the contents of this pull request, and >> previous commits have been removed. Incremental views are not available. The >> pull request now contains two commits: >> >> - Enable jcheck whitespace checks for .S files >> - Run expand on libjsvml > > src/jdk.incubator.vector/linux/native/libjsvml/jsvml_d_acos_linux_x86.S line > 37: > >> 35: .text >> 36: # mark_begin; >> 37:.align16,0x90 > > .align seems to ironically not be aligned with the rest of the directives Bad indentation like this is not something jcheck can detect or will complain about. However, I agree that it looks horrible. What's more, this seem to be problematic in multiple locations -- many, but not all, instances of `.align` (and `ALIGN` on Windows) are unaligned. I guess it is due to copy/paste error. I'm sort of reluctant to fix this in this PR, but then again, it just looks too bad. - PR Review Comment: https://git.openjdk.org/jdk/pull/18268#discussion_r1523085132
Re: RFR: 8328074: Add jcheck whitespace checking for assembly files [v4]
> As part of the ongoing effort to enable jcheck whitespace checking to all > text files, it is now time to address assembly (.S) files. The hotspot > assembly files were fixed as part of the hotspot mapfile removal, so only a > single incorrect jdk library remains. > > The main issue with `libjsvml` was inconsistent line starts, sometimes using > tabs. I used the `expand` unix command line tool to replace these with spaces. Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision: Make all ALIGN/.align aligned - Changes: - all: https://git.openjdk.org/jdk/pull/18268/files - new: https://git.openjdk.org/jdk/pull/18268/files/ce1d4c9f..4e729cb6 Webrevs: - full: Webrev is not available because diff is too large - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18268&range=02-03 Stats: 625 lines in 71 files changed: 0 ins; 0 del; 625 mod Patch: https://git.openjdk.org/jdk/pull/18268.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18268/head:pull/18268 PR: https://git.openjdk.org/jdk/pull/18268
Re: RFR: 8325621: Improve jspawnhelper version checks [v3]
On Wed, 13 Mar 2024 09:49:13 GMT, Aleksey Shipilev wrote: >> make/modules/java.base/Launcher.gmk line 81: >> >>> 79: SRC := $(TOPDIR)/src/$(MODULE)/unix/native/jspawnhelper, \ >>> 80: OPTIMIZATION := LOW, \ >>> 81: CFLAGS := $(CFLAGS_JDKEXE) \ >> >> There is no need to introduce a break after `$(CFLAGS_JDKEXE)`, for thing >> like flags we try to fill the line. >> Also, the indentation rules are that a broken line should be indented with >> four spaces. See e.g. the CFLAGS line below in CoreLibraries.gmk. > > My fault, I suggested Chad to do this. So what's the preferred formatting > here? Like BUILD_JEXEC block above does it? > > > CFLAGS := $(CFLAGS_JDKEXE) $(VERSION_CFLAGS) \ > -I$(TOPDIR)/src/$(MODULE)/unix/native/libjava, \ Yeah, that looks good. I was thinking just appending it at the end like: CFLAGS := $(CFLAGS_JDKEXE) -I$(TOPDIR)/src/$(MODULE)/unix/native/libjava \ $(VERSION_CFLAGS), \ but your version definitely looks better! - PR Review Comment: https://git.openjdk.org/jdk/pull/18204#discussion_r1522963660
Re: RFR: 8325621: Improve jspawnhelper version checks [v3]
On Tue, 12 Mar 2024 19:22:25 GMT, Chad Rakoczy wrote: >> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621) >> >> Updates jspawnhelper to check that JDK version and jspawnhelper version are >> the same. Updates test to include check for version. Also tested manually by >> replacing jspawnhelper with incorrect version to confirm that check works. > > Chad Rakoczy has updated the pull request incrementally with two additional > commits since the last revision: > > - Print to stdout instead of stderr > - Compare version using VERSION_STRING I was thinking what would happen for the first time we upgrade jspawnhelper like this. There would be no helpful message then, the `jspawnhelper` would just exit on argument count check. So I suggest we polish the failure messages a bit: always report version at `shutItDown`, and report more verbose failure messages too. See: [jspawnhelper-messages-1.patch](https://github.com/openjdk/jdk/files/14586196/jspawnhelper-messages-1.patch) Older JDK invoking new jspawnhelper would then fail like: % build/macosx-aarch64-server-fastdebug/images/jdk/lib/jspawnhelper 1:1:1 Incorrect number of arguments: 2 jspawnhelper version 23-internal-adhoc.shipilev.shipilev-jdk This command is not for general use and should only be run as the result of a call to ProcessBuilder.start() or Runtime.exec() in a java application - PR Comment: https://git.openjdk.org/jdk/pull/18204#issuecomment-1994051562
Re: RFR: 8327701: Remove the xlc toolchain [v4]
On Wed, 13 Mar 2024 09:50:20 GMT, Joachim Kern wrote: > e.g. We should change the HOTSPOT_TOOLCHAIN_TYPE=xlc to aix, because it is > not toolchain, but OS related. As a consequence the globalDefinitions_xlc.hpp > will become globalDefinitions_aix.hpp No, it's not that simple. First, the `globalDefinitions_` files are included on a per-toolchain basis, not OS basis. Secondly, I think you will want to have the stuff in globalDefinitions_gcc.h. In fact, if you compare globalDefinitions_gcc.h and globalDefinitions_xlc.h side-by-side, you see that they are much more similar than you'd perhaps naively expect. The remaining differences will probably be better expressed as #ifdef AIX inside globalDefinitions_gcc.h, I assume. (But of course, in the end this is up to the hotspot team.) I recommend doing such a side-by-side check yourself to get an understanding of the actual differences. - PR Comment: https://git.openjdk.org/jdk/pull/18172#issuecomment-1994049434
Re: RFR: 8327701: Remove the xlc toolchain [v4]
On Tue, 12 Mar 2024 15:27:29 GMT, Magnus Ihse Bursie wrote: >> As of [JDK-8325880](https://bugs.openjdk.org/browse/JDK-8325880), building >> the JDK requires version 17 of IBM Open XL C/C++ (xlc). This is in effect >> clang by another name, and it uses the clang toolchain in the JDK build. >> Thus the old xlc toolchain is no longer supported, and should be removed. > > Magnus Ihse Bursie has updated the pull request incrementally with two > additional commits since the last revision: > > - Replace CC_VERSION_OUTPUT with CC_VERSION_STRING > - We need CC_VERSION_OUTPUT before we can check it e.g. We should change the HOTSPOT_TOOLCHAIN_TYPE=xlc to aix, because it is not toolchain, but OS related. As a consequence the globalDefinitions_xlc.hpp will become globalDefinitions_aix.hpp - PR Comment: https://git.openjdk.org/jdk/pull/18172#issuecomment-1993979991
Re: RFR: 8327791: Optimization for new BigDecimal(String) [v12]
> The current BigDecimal(String) constructor calls String#toCharArray, which
> has a memory allocation.
>
>
> public BigDecimal(String val) {
> this(val.toCharArray(), 0, val.length()); // allocate char[]
> }
>
>
> When the length is greater than 18, create a char[]
>
>
> boolean isCompact = (len <= MAX_COMPACT_DIGITS); // 18
> if (!isCompact) {
> // ...
> } else {
> char[] coeff = new char[len]; // allocate char[]
> // ...
> }
>
>
> This PR eliminates the two memory allocations mentioned above, resulting in
> an approximate 60% increase in performance..
Shaojin Wen has updated the pull request incrementally with one additional
commit since the last revision:
fix benchmark
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18177/files
- new: https://git.openjdk.org/jdk/pull/18177/files/fcf4c818..be2119c4
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=18177&range=11
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=18177&range=10-11
Stats: 14 lines in 1 file changed: 8 ins; 5 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/18177.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18177/head:pull/18177
PR: https://git.openjdk.org/jdk/pull/18177
Re: RFR: 8325621: Improve jspawnhelper version checks [v3]
On Tue, 12 Mar 2024 23:44:45 GMT, Magnus Ihse Bursie wrote: >> Chad Rakoczy has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Print to stdout instead of stderr >> - Compare version using VERSION_STRING > > make/modules/java.base/Launcher.gmk line 81: > >> 79: SRC := $(TOPDIR)/src/$(MODULE)/unix/native/jspawnhelper, \ >> 80: OPTIMIZATION := LOW, \ >> 81: CFLAGS := $(CFLAGS_JDKEXE) \ > > There is no need to introduce a break after `$(CFLAGS_JDKEXE)`, for thing > like flags we try to fill the line. > Also, the indentation rules are that a broken line should be indented with > four spaces. See e.g. the CFLAGS line below in CoreLibraries.gmk. My fault, I suggested Chad to do this. So what's the preferred formatting here? Like BUILD_JEXEC block above does it? CFLAGS := $(CFLAGS_JDKEXE) $(VERSION_CFLAGS) \ -I$(TOPDIR)/src/$(MODULE)/unix/native/libjava, \ - PR Review Comment: https://git.openjdk.org/jdk/pull/18204#discussion_r1522886607
Re: RFR: 8328037: Test java/util/Formatter/Padding.java has unnecessary high heap requirement after JDK-8326718
On Wed, 13 Mar 2024 07:53:30 GMT, Christoph Langer wrote: > 4f336085d1098e7fba7b58f0a73c028179a2a13d > ([JDK-8326718](https://bugs.openjdk.org/browse/JDK-8326718)) added a few > cases to test java/util/Formatter/Padding.java with huge Strings as > arguments. Since all possible argument combinations for the test are stored > in one array, nothing can be garbage collected while the test is running and > the heap requirement is blown up. > > In one of our test pipelines we run tier1 tests with VMs that default to 384M > of heap and this is not sufficient any longer. > > I'm improving this by splitting the one large @ParameterizedTest into > multiple ones. With that, I could run the test successfully in a test VM with > 96M of heap, e.g. by modifying `@run junit Padding` to `@run junit/othervm > -Xmx96m Padding` What about factoring out the 4 invocations of `tenMillionBlanks()` in each source method in a local var? - PR Comment: https://git.openjdk.org/jdk/pull/18264#issuecomment-1993965369
Re: RFR: 8327998: Enable java/lang/ProcessBuilder/JspawnhelperProtocol.java on Mac
On Tue, 12 Mar 2024 21:52:31 GMT, Elif Aslan wrote: > This change enables to run JspawnhelperProtocol.java on MacOS. > > In addition to GHA , the test has been run on macos and linux. > > > Test report is stored in > build/macosx-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java > > == > Test summary > == >TEST TOTAL PASS FAIL ERROR >jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java > 1 1 0 0 > == > TEST SUCCESS > > Finished building target 'test' in configuration > 'macosx-x86_64-server-fastdebug' > > > > > > Test report is stored in > build/linux-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java > > == > Test summary > == >TEST TOTAL PASS FAIL ERROR >jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java > 1 1 0 0 > == > TEST SUCCESS > > Stopping javac server > [ec2-user@ip-172-16-0-10 jdk]$ make test CONF=linux-x86_64-server-fastdebug > TEST=test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java Looks fine, thanks! - Marked as reviewed by shade (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18253#pullrequestreview-1933545908
Re: RFR: 8327701: Remove the xlc toolchain [v4]
On Wed, 13 Mar 2024 08:50:10 GMT, Matthias Baesken wrote: >> Magnus Ihse Bursie has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - Replace CC_VERSION_OUTPUT with CC_VERSION_STRING >> - We need CC_VERSION_OUTPUT before we can check it > > Seems `HOTSPOT_TOOLCHAIN_TYPE=xlc ` and `TARGET_COMPILER_xlc` macros stay, > is this intended ? > (not saying this is a wrong thing to do, but I believed first that all the > xlc toolchain references are replaced by clang?) @MBaesken Yes, I discussed this in length above: https://github.com/openjdk/jdk/pull/18172#issuecomment-1985939418. In short, changing that would mean changing behavior, and that is not something I want to do in a removal refactoring. Also, it is up to you guys to make it work correctly. But I really believe you should address this. Let me know if you need help with the build aspects. - PR Comment: https://git.openjdk.org/jdk/pull/18172#issuecomment-1993862922
Re: RFR: 8327701: Remove the xlc toolchain [v4]
On Tue, 12 Mar 2024 15:27:29 GMT, Magnus Ihse Bursie wrote: >> As of [JDK-8325880](https://bugs.openjdk.org/browse/JDK-8325880), building >> the JDK requires version 17 of IBM Open XL C/C++ (xlc). This is in effect >> clang by another name, and it uses the clang toolchain in the JDK build. >> Thus the old xlc toolchain is no longer supported, and should be removed. > > Magnus Ihse Bursie has updated the pull request incrementally with two > additional commits since the last revision: > > - Replace CC_VERSION_OUTPUT with CC_VERSION_STRING > - We need CC_VERSION_OUTPUT before we can check it Seems `HOTSPOT_TOOLCHAIN_TYPE=xlc ` and `TARGET_COMPILER_xlc` macros stay, is this intended ? (not saying this is a wrong thing to do, but I believed first that all the xlc toolchain references are replaced by clang?) - PR Comment: https://git.openjdk.org/jdk/pull/18172#issuecomment-1993844670
Integrated: 8327701: Remove the xlc toolchain
On Fri, 8 Mar 2024 15:29:58 GMT, Magnus Ihse Bursie wrote: > As of [JDK-8325880](https://bugs.openjdk.org/browse/JDK-8325880), building > the JDK requires version 17 of IBM Open XL C/C++ (xlc). This is in effect > clang by another name, and it uses the clang toolchain in the JDK build. Thus > the old xlc toolchain is no longer supported, and should be removed. This pull request has now been integrated. Changeset: 107cb536 Author:Magnus Ihse Bursie URL: https://git.openjdk.org/jdk/commit/107cb536e75509ad63b245d20772eb2c3f73d595 Stats: 327 lines in 19 files changed: 24 ins; 266 del; 37 mod 8327701: Remove the xlc toolchain Reviewed-by: jwaters, erikj - PR: https://git.openjdk.org/jdk/pull/18172
Re: RFR: 8327701: Remove the xlc toolchain [v2]
On Tue, 12 Mar 2024 16:02:41 GMT, Matthias Baesken wrote: > > Please re-test. > > Hi Magnus, thanks for the adjustments. I reenabled your patch in our > build/test queue . Builds and test results on AIX (product and fastdebug) are fine with the latest version of the PR . - PR Comment: https://git.openjdk.org/jdk/pull/18172#issuecomment-1993808324
Integrated: 8327460: Compile tests with the same visibility rules as product code
On Wed, 6 Mar 2024 11:33:30 GMT, Magnus Ihse Bursie wrote: > Currently, our symbol visibility handling for tests are sloppy; we only > handle it properly on Windows. We need to bring it up to the same levels as > product code. This is a prerequisite for > [JDK-8327045](https://bugs.openjdk.org/browse/JDK-8327045), which in turn is > a building block for Hermetic Java. This pull request has now been integrated. Changeset: cc9a8aba Author:Magnus Ihse Bursie URL: https://git.openjdk.org/jdk/commit/cc9a8aba67f4e240c8de2d1ae15d1b80bfa446a0 Stats: 304 lines in 39 files changed: 69 ins; 159 del; 76 mod 8327460: Compile tests with the same visibility rules as product code Reviewed-by: erikj, jvernee, dholmes, alanb - PR: https://git.openjdk.org/jdk/pull/18135
RFR: 8328037: Test java/util/Formatter/Padding.java has unnecessary high heap requirement after JDK-8326718
4f336085d1098e7fba7b58f0a73c028179a2a13d ([JDK-8326718](https://bugs.openjdk.org/browse/JDK-8326718)) added a few cases to test java/util/Formatter/Padding.java with huge Strings as arguments. Since all possible argument combinations for the test are stored in one array, nothing can be garbage collected while the test is running and the heap requirement is blown up. In one of our test pipelines we run tier1 tests with VMs that default to 384M of heap and this is not sufficient any longer. I'm improving this by splitting the one large @ParameterizedTest into multiple ones. With that, I could run the test successfully in a test VM with 96M of heap, e.g. by modifying `@run junit Padding` to `@run junit/othervm -Xmx96m Padding` - Commit messages: - JDK-8328037 Changes: https://git.openjdk.org/jdk/pull/18264/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18264&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8328037 Stats: 473 lines in 1 file changed: 168 ins; 95 del; 210 mod Patch: https://git.openjdk.org/jdk/pull/18264.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18264/head:pull/18264 PR: https://git.openjdk.org/jdk/pull/18264
Re: RFR: 8327998: Enable java/lang/ProcessBuilder/JspawnhelperProtocol.java on Mac
On Tue, 12 Mar 2024 21:52:31 GMT, Elif Aslan wrote: > This change enables to run JspawnhelperProtocol.java on MacOS. > > In addition to GHA , the test has been run on macos and linux. > > > Test report is stored in > build/macosx-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java > > == > Test summary > == >TEST TOTAL PASS FAIL ERROR >jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java > 1 1 0 0 > == > TEST SUCCESS > > Finished building target 'test' in configuration > 'macosx-x86_64-server-fastdebug' > > > > > > Test report is stored in > build/linux-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java > > == > Test summary > == >TEST TOTAL PASS FAIL ERROR >jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java > 1 1 0 0 > == > TEST SUCCESS > > Stopping javac server > [ec2-user@ip-172-16-0-10 jdk]$ make test CONF=linux-x86_64-server-fastdebug > TEST=test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java LGTM. > /approval request This change enables to run JspawnhelperProtocol.java on > MacOS. In addition to GHA , the test has been run on macos and linux. The `approval` command can only be used at a backport PR. Just a reminder. - Marked as reviewed by gli (Committer). PR Review: https://git.openjdk.org/jdk/pull/18253#pullrequestreview-1933358504 PR Comment: https://git.openjdk.org/jdk/pull/18253#issuecomment-1993744969
Re: RFR: 8327460: Compile tests with the same visibility rules as product code [v2]
On Tue, 12 Mar 2024 13:55:11 GMT, Magnus Ihse Bursie wrote: >> test/hotspot/jtreg/runtime/ErrorHandling/libTestDwarfHelper.h line 24: >> >>> 22: */ >>> 23: >>> 24: #include >> >> Seems unneeded. > > So what I did here was change: > > #include "jni.h" > #include > > > into > > #include > > #include "export.h" > #include "jni.h" > > > The reordering was strictly not needed, but putting user includes in front of > system ones looked like bad coding to me, and put me in a difficult spot in > where to add the new `#include "export.h"` -- next to the current user > include even if I thought that was wrong, or have the system includes > "sandwitched" between two user includes. > > Or do you mean that it seems unneeded to include `jni.h` at all? Yes, I > agree, but it was there before, and I don't want to make any other changes to > the tests. This change is scary enough as it is :-). If you want, I can file > a follow-up to remove this include instead. Yes I meant the include is actually unnecessary, but fine to not make that change here. - PR Review Comment: https://git.openjdk.org/jdk/pull/18135#discussion_r1522659393
