Re: RFR: 8314491: Linux: jexec launched via PATH fails to find java [v6]
> 8314491: Linux: jexec launched via PATH fails to find java Vladimir Petko has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains eight additional commits since the last revision: - Merge branch 'master' into 8314491-jexec-resolve-symlink - declare error in-place - remove unused imports - Review comment: use /proc/self/exe as the backup option - Merge branch 'master' into 8314491-jexec-resolve-symlink - correct copyright statement - Use /proc/self/exec to identify path to the executable. - Create failing test for jexec in PATH issue - Changes: - all: https://git.openjdk.org/jdk/pull/15343/files - new: https://git.openjdk.org/jdk/pull/15343/files/c974e011..5354ceba Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15343&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15343&range=04-05 Stats: 4629 lines in 273 files changed: 3398 ins; 519 del; 712 mod Patch: https://git.openjdk.org/jdk/pull/15343.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15343/head:pull/15343 PR: https://git.openjdk.org/jdk/pull/15343
Re: RFR: 8314949: linux PPC64 Big Endian: Implementation of Foreign Function & Memory API
On Thu, 24 Aug 2023 23:33:23 GMT, Maurizio Cimadamore
wrote:
>> I've found a way to solve the remaining FFI problem on linux PPC64 Big
>> Endian. Large structs (>8 Bytes) which are passed in registers or on stack
>> require shifting the Bytes in the last slot if the size is not a multiple of
>> 8. This PR adds the required functionality to the Java code.
>>
>> Please review and provide feedback. There may be better ways to implement
>> it. I just found one which works and makes the tests pass:
>>
>> Test summary
>> ==
>>TEST TOTAL PASS FAIL ERROR
>>
>>jtreg:test/jdk/java/foreign 8888 0 0
>>
>>
>>
>> Note: This PR should be considered as preparation work for AIX which also
>> uses ABIv1.
>
> src/java.base/share/classes/jdk/internal/foreign/abi/ppc64/ABIv1CallArranger.java
> line 33:
>
>> 31: * PPC64 CallArranger specialized for ABI v1.
>> 32: */
>> 33: public class ABIv1CallArranger extends CallArranger {
>
> Wouldn't it be more natural for CallArranger to have an abstract method (or
> even a kind() accessor for the different kinds of ABI supported) and then
> have these specialized subclasses return the correct kind? It seems to me
> that setting the `useXYZAbi` flag using an instanceof test is a little dirty.
I had something like that, but another reviewer didn't like it, either.
Originally, I had thought that the v1 and v2 CallArrangers would get more
content, but they're still empty. Would it be better to remove these special
CallArrangers and distinguish in the base class?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15417#discussion_r1305300539
Re: RFR: 8314094: java/lang/ProcessHandle/InfoTest.java fails on Windows when run as user with Administrator privileges
On Wed, 16 Aug 2023 18:33:01 GMT, Roger Riggs wrote:
>> On Windows, the test java/lang/ProcessHandle/InfoTest.java can fail when run
>> as user that is member of the Administrators group. In that case new files
>> are not owned by the user but instead by BUILTIN\ADMINISTRATORS. This breaks
>> the assumptions of the test's whoami check. My suggestion is to cater for
>> this case and don't fail the test but write a warning message to stdout that
>> a whoami check is not correctly possible.
>
> test/jdk/java/lang/ProcessHandle/InfoTest.java line 304:
>
>> 302: if (Platform.isWindows() &&
>> "BUILTIN\\Administrators".equals(whoami)) {
>> 303: System.out.println("Test seems to be run as
>> Administrator. " +
>> 304: "Check for user correctness is not
>> possible.");
>
> Is there an alternative way to determine the expected username?
> Perhaps by running a windows command or extracting it from the environment
> (System.getEnv("XX"))?
I think you might use System.getProperty("user.name"). But I am not sure about
domain names of users on Windows.
I am also not sure why the user name is currently determined by creating a file
- there might be a reason for this that is not obvious to me.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15222#discussion_r1305312497
Re: RFR: 8314949: linux PPC64 Big Endian: Implementation of Foreign Function & Memory API
On Thu, 24 Aug 2023 23:58:35 GMT, Maurizio Cimadamore wrote: >> Maybe I'm starting to see it - it's not a special rule, as much as it is a >> consequence of the endianness. E.g. if you have a struct that is 64 + 32 >> bytes, you can store the first 64 bytes as a long. Then, there's an issue as >> we have to fill another long, but we have only 32 bits of value. Is it the >> problem that if we just copy the value into the long word "as is" it will be >> stored in the "wrong" 32 bits? So the shift takes care of that, I guess? > > If my assumption above is correct, then maybe another way to solve the > problem, would be to, instead of adding a new shift binding, to generalize > the VM store binding we have to allow writing a smaller value into a bigger > storage, with an offset. Correct? The ABI says: "An aggregate or union smaller than one doubleword in size is padded so that it appears in the least significant bits of the doubleword. All others are padded, if necessary, at their tail." [https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi.html#PARAM-PASS]. I have written examples which pass 9 and 15 Bytes. In the first case, we need to get 0x0001020304050607 in the first argument and 0x08XX into the second argument (X is "don't care"). Shift amount is 7. In the second case, we need to get 0x0001020304050607 in the first argument and 0x08090a0b0c0d0eXX into the second argument. Shift amount is 1. In other words, we need shift amounts between 1 and 7. Stack slots and registers are always 64 bit on PPC64. - PR Review Comment: https://git.openjdk.org/jdk/pull/15417#discussion_r1305313810
Re: RFR: 8314949: linux PPC64 Big Endian: Implementation of Foreign Function & Memory API
On Fri, 25 Aug 2023 00:10:11 GMT, Maurizio Cimadamore
wrote:
>> I've found a way to solve the remaining FFI problem on linux PPC64 Big
>> Endian. Large structs (>8 Bytes) which are passed in registers or on stack
>> require shifting the Bytes in the last slot if the size is not a multiple of
>> 8. This PR adds the required functionality to the Java code.
>>
>> Please review and provide feedback. There may be better ways to implement
>> it. I just found one which works and makes the tests pass:
>>
>> Test summary
>> ==
>>TEST TOTAL PASS FAIL ERROR
>>
>>jtreg:test/jdk/java/foreign 8888 0 0
>>
>>
>>
>> Note: This PR should be considered as preparation work for AIX which also
>> uses ABIv1.
>
> src/java.base/share/classes/jdk/internal/foreign/abi/Binding.java line 717:
>
>> 715: public void interpret(Deque stack, StoreFunc storeFunc,
>> 716: LoadFunc loadFunc, SegmentAllocator
>> allocator) {
>> 717: if (shiftAmount > 0) {
>
> Why do we assume we can only deal with ints or longs?
I have inserted casts into `public Binding.Builder shiftLeft(int shiftAmount,
Class type)` (similar to other bindings). The VM handles integral types
smaller than `int` like `int` and uses 4 Bytes for arithmetic operations.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15417#discussion_r1305321446
Re: RFR: 8314949: linux PPC64 Big Endian: Implementation of Foreign Function & Memory API
On Thu, 24 Aug 2023 23:38:42 GMT, Maurizio Cimadamore wrote: >> I've found a way to solve the remaining FFI problem on linux PPC64 Big >> Endian. Large structs (>8 Bytes) which are passed in registers or on stack >> require shifting the Bytes in the last slot if the size is not a multiple of >> 8. This PR adds the required functionality to the Java code. >> >> Please review and provide feedback. There may be better ways to implement >> it. I just found one which works and makes the tests pass: >> >> Test summary >> == >>TEST TOTAL PASS FAIL ERROR >> >>jtreg:test/jdk/java/foreign 8888 0 0 >> >> >> >> Note: This PR should be considered as preparation work for AIX which also >> uses ABIv1. > > Overall these changes look good - as commented I'd like to learn a bit more > of the underlying ABI, to get a sense of whether adding a new binding is ok. > But overall it's great to see support for a big-endian ABI - apart from the > linker, I am pleased to see that you did not encounter too many issues in the > memory-side of the FFM API. @mcimadamore: Thanks for your feedback! Jorn and I had resolved the other issues already when we have worked on the linux little endian part. It already contains some ABIv1 code. Note that we already have one big endian platform: s390. But that one doesn't pass structs >8 Bytes in registers. - PR Comment: https://git.openjdk.org/jdk/pull/15417#issuecomment-1692938709
Re: RFR: 8263135: unique_ptr should not be used for types that are not pointers
On Fri, 25 Aug 2023 06:30:13 GMT, Alexey Semenyuk wrote: > The intention was to make jpackage native code work with compilers without > native std::unique_ptr. I understand, however it appears MSVC does not use the code inside that file - PR Comment: https://git.openjdk.org/jdk/pull/2858#issuecomment-1692968268
Re: RFR: 8314949: linux PPC64 Big Endian: Implementation of Foreign Function & Memory API
On Fri, 25 Aug 2023 07:36:47 GMT, Martin Doerr wrote:
>> src/java.base/share/classes/jdk/internal/foreign/abi/ppc64/ABIv1CallArranger.java
>> line 33:
>>
>>> 31: * PPC64 CallArranger specialized for ABI v1.
>>> 32: */
>>> 33: public class ABIv1CallArranger extends CallArranger {
>>
>> Wouldn't it be more natural for CallArranger to have an abstract method (or
>> even a kind() accessor for the different kinds of ABI supported) and then
>> have these specialized subclasses return the correct kind? It seems to me
>> that setting the `useXYZAbi` flag using an instanceof test is a little dirty.
>
> I had something like that, but another reviewer didn't like it, either.
> Originally, I had thought that the v1 and v2 CallArrangers would get more
> content, but they're still empty. Would it be better to remove these special
> CallArrangers and distinguish in the base class?
It seems to me that what you are doing is similar to what was done for aarch64,
which was dealt with using very simple subclasses:
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/linux/LinuxAArch64CallArranger.java
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/macos/MacOsAArch64CallArranger.java
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/windows/WindowsAArch64CallArranger.java
In your case there's less difference, but I think we should follow the same
idiom for both.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15417#discussion_r1305396592
Re: RFR: 8314949: linux PPC64 Big Endian: Implementation of Foreign Function & Memory API
On Fri, 25 Aug 2023 07:48:19 GMT, Martin Doerr wrote: >> If my assumption above is correct, then maybe another way to solve the >> problem, would be to, instead of adding a new shift binding, to generalize >> the VM store binding we have to allow writing a smaller value into a bigger >> storage, with an offset. Correct? > > The ABI says: "An aggregate or union smaller than one doubleword in size is > padded so that it appears in the least significant bits of the doubleword. > All others are padded, if necessary, at their tail." > [https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi.html#PARAM-PASS]. > I have written examples which pass 9 and 15 Bytes. > In the first case, we need to get 0x0001020304050607 in the first argument > and 0x08XX into the second argument (X is "don't care"). Shift > amount is 7. > In the second case, we need to get 0x0001020304050607 in the first argument > and 0x08090a0b0c0d0eXX into the second argument. Shift amount is 1. > In other words, we need shift amounts between 1 and 7. Stack slots and > registers are always 64 bit on PPC64. Got it - I found these representations: https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi-1.7.html#BYTEORDER Very helpful. So you have e.g. a `short` value (loaded from somewhere) and you have to store it on a double-word. Now, if you just stored it at offset 0, you will write the bits 0-15, which are the "most" significant bits in big-endian representation. So, it's backwards. I believe FFM will take care of endianness, so that the bytes 0-7 and 8-15 will be "swapped" when writing into the double-word (right?) but their base offset (0) is still off, as they should really start at offset 48. Hence the shift. - PR Review Comment: https://git.openjdk.org/jdk/pull/15417#discussion_r1305420316
Re: RFR: 8311220: Optimization for StringLatin UpperLower [v3]
On Thu, 6 Jul 2023 05:20:14 GMT, 温绍锦 wrote: >> # Benchmark Result >> >> >> sh make/devkit/createJMHBundle.sh >> bash configure --with-jmh=build/jmh/jars >> make test TEST="micro:java.lang.StringUpperLower.*" >> >> >> >> ## 1. >> [aliyun_ecs_c8i.xlarge](https://help.aliyun.com/document_detail/25378.html#c8i) >> * cpu : intel xeon sapphire rapids (x64) >> >> ``` diff >> -Benchmark Mode Cnt Score Error Units (baseline) >> -StringUpperLower.lowerToLower avgt 15 27.180 ± 0.017 ns/op >> -StringUpperLower.lowerToUpper avgt 15 47.196 ± 0.066 ns/op >> -StringUpperLower.mixedToLower avgt 15 32.307 ± 0.072 ns/op >> -StringUpperLower.mixedToUpper avgt 15 44.005 ± 0.414 ns/op >> -StringUpperLower.upperToLower avgt 15 32.310 ± 0.033 ns/op >> -StringUpperLower.upperToUpper avgt 15 42.053 ± 0.341 ns/op >> >> +Benchmark Mode Cnt Score Error Units (Update 01) >> +StringUpperLower.lowerToLower avgt 15 16.964 ± 0.021 ns/op (+60.96%) >> +StringUpperLower.lowerToUpper avgt 15 46.491 ± 0.036 ns/op (+1.51%) >> +StringUpperLower.mixedToLower avgt 15 35.947 ± 0.254 ns/op (-10.12%) >> +StringUpperLower.mixedToUpper avgt 15 41.976 ± 0.326 ns/op (+4.83%) >> +StringUpperLower.upperToLower avgt 15 33.466 ± 4.036 ns/op (-3.45%) >> +StringUpperLower.upperToUpper avgt 15 17.446 ± 1.036 ns/op (+141.04%) >> >> +Benchmark Mode Cnt Score Error Units (Update 00) >> +StringUpperLower.lowerToLower avgt 15 16.976 ± 0.043 ns/op (+60.160) >> +StringUpperLower.lowerToUpper avgt 15 46.373 ± 0.086 ns/op (+1.77%) >> +StringUpperLower.mixedToLower avgt 15 32.018 ± 0.061 ns/op (+0.9%) >> +StringUpperLower.mixedToUpper avgt 15 42.019 ± 0.473 ns/op (+4.72%) >> +StringUpperLower.upperToLower avgt 15 32.052 ± 0.051 ns/op (+0.8%) >> +StringUpperLower.upperToUpper avgt 15 16.978 ± 0.190 ns/op (+147.69%) >> >> >> ## 2. >> [aliyun_ecs_c8a.xlarge](https://help.aliyun.com/document_detail/25378.html#c8a) >> * cpu : amd epc genoa (x64) >> >> ``` diff >> -Benchmark Mode Cnt Score Error Units (baseline) >> -StringUpperLower.lowerToLower avgt 15 22.164 ± 0.021 ns/op >> -StringUpperLower.lowerToUpper avgt 15 46.113 ± 0.047 ns/op >> -StringUpperLower.mixedToLower avgt 15 28.501 ± 0.037 ns/op >> -StringUpperLower.mixedToUpper avgt 15 38.782 ± 0.038 ns/op >> -StringUpperLower.upperToLower avgt 15 28.625 ± 0.162 ns/op >> -StringUpperLower.upperToUpper avgt 15 27.960 ± 0.038 ns/op >> >> +B... > > 温绍锦 has updated the pull request incrementally with one additional commit > since the last revision: > > add method CharacterDataLatin1#isLowerCaseEx @AlanBateman can you help me to review this PR? - PR Comment: https://git.openjdk.org/jdk/pull/14751#issuecomment-1693056828
Re: RFR: 8314063 : The socket is not closed in Connection::createSocket when the handshake failed for LDAP connection [v14]
On Thu, 24 Aug 2023 23:08:43 GMT, Weibing Xiao wrote: >> Please refer to JDK-8314063. >> >> The failure scenario is due to the setting of connection timeout. It is >> either too small or not an optimal value for the system. When the client >> tries to connect to the server with LDAPs protocol. It requires the >> handshake after the socket is created and connected, but it fails due to >> connection timeout and leaves the socket open. It is not closed properly due >> to the exception handling in the JDK code. >> >> The change is adding a try/catch block and closing the socket in the catch >> block, and the format of the code got changed consequently. > > Weibing Xiao has updated the pull request incrementally with one additional > commit since the last revision: > > update the code Thanks for addressing all comments, Weibing. The changes look good to me. I will sponsor this change. - Marked as reviewed by aefimov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15294#pullrequestreview-1595318497
Re: RFR: 8314949: linux PPC64 Big Endian: Implementation of Foreign Function & Memory API
On Fri, 25 Aug 2023 07:54:51 GMT, Martin Doerr wrote:
>> src/java.base/share/classes/jdk/internal/foreign/abi/Binding.java line 717:
>>
>>> 715: public void interpret(Deque stack, StoreFunc storeFunc,
>>> 716: LoadFunc loadFunc, SegmentAllocator
>>> allocator) {
>>> 717: if (shiftAmount > 0) {
>>
>> Why do we assume we can only deal with ints or longs?
>
> I have inserted casts into `public Binding.Builder shiftLeft(int shiftAmount,
> Class type)` (similar to other bindings). The VM handles integral types
> smaller than `int` like `int` and uses 4 Bytes for arithmetic operations.
Ah I see that now - it's done the binding "builder".
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15417#discussion_r1305426640
RFR: 8315004: Runtime.halt() debug logging
I want to add a log output similar to JDK-8301627 to Runtime.halt(). To avoid double logging of Runtime.exit(), add a flag to indicate whether logging was done, and fix it so that logging is done only once. Could someone please review this fix? - Commit messages: - Merge branch 'openjdk:master' into LogRuntimeHalt - Merge branch 'openjdk:master' into LogRuntimeHalt - Add log output on Runtime.halt() - Add log output on Runtime.halt() Changes: https://git.openjdk.org/jdk/pull/15426/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=15426&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8315004 Stats: 21 lines in 2 files changed: 7 ins; 0 del; 14 mod Patch: https://git.openjdk.org/jdk/pull/15426.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15426/head:pull/15426 PR: https://git.openjdk.org/jdk/pull/15426
Re: RFR: 8315004: Runtime.halt() debug logging
On Fri, 25 Aug 2023 09:37:47 GMT, Masanori Yano wrote: > I want to add a log output similar to JDK-8301627 to Runtime.halt(). > To avoid double logging of Runtime.exit(), add a flag to indicate whether > logging was done, and fix it so that logging is done only once. > Could someone please review this fix? I think you may have missed the comment in the JBS issue. Logging means running potentially arbitrary code, doing this at Runtime.halt time is problematic. I thought the conclusion from the work on Runtime.exit was not to log in Runtime.halt? - PR Comment: https://git.openjdk.org/jdk/pull/15426#issuecomment-1693095419
Re: RFR: 8314940: Use of NIO in JDKs Metrics implementation causes issues in GraalVM
On Thu, 24 Aug 2023 14:39:18 GMT, Severin Gehwolf wrote: > I've now realized that the bug had an incorrect statement in the description. > The cycle happens due to the `Runtime.getRuntime().maxMemory()` > implementation in GraalVM to use JDK `Metrics`, since the `ByteBuffer` [code > relies on the `Runtime.getRuntime().maxMemory()` > API](https://github.com/openjdk/jdk/blob/76b9011c9ecb8c0c713a58d034f281ba70d65d4e/src/java.base/share/classes/jdk/internal/misc/VM.java#L261). > The GraalVM impl to use the JDK Metrics seems a reasonable thing to do, no? > > With that said, it's seems a rather uncontroversial change with very limited > scope. Do you see anything problematic in this patch? Happy to revise if you > think there are some no-no's :) Recursive initialization issues can tricky and often it comes down to deciding where to break the cycle. In this case, it seems a bit fragile to do it in CgroupUtil as it should be allowed to use any of the file system APIs to access cgroups or proc files. Part of me wonders if this would be better handled in their implementation of jdk.internal.misc.VM or Runtime.maxMemory but maybe you've been there already? - PR Comment: https://git.openjdk.org/jdk/pull/15416#issuecomment-1693114562
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError [v2]
On Thu, 24 Aug 2023 10:54:19 GMT, Sean Coffey wrote:
>> Recursive initialization calls possible during loading of LoggerFinder
>> service.
>>
>> This fix detects the recursive call and returns a temporary LoggerFinder
>> that is backed by a lazy logger. Automated test case developed to simulate
>> loading of an external LoggerFinder service while also having other threads
>> poke System.getLogger during this framework initialization.
>
> Sean Coffey has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Improve test coverage
> - Incorporate review comments from Daniel
src/java.base/share/classes/jdk/internal/logger/BootstrapLogger.java line 988:
> 986: private static void ensureClassInitialized(Class c) {
> 987: try {
> 988: MethodHandles.lookup().ensureInitialized(c);
Hello Sean, should we check if there are any implications, like on startup
performance, of using `MethodHandles` in this `BootstrapLogger`?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1305485295
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError [v2]
On Wed, 23 Aug 2023 17:10:42 GMT, Daniel Fuchs wrote:
>> Sean Coffey has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Improve test coverage
>> - Incorporate review comments from Daniel
>
> src/java.base/share/classes/jdk/internal/logger/LazyLoggers.java line 425:
>
>> 423: */
>> 424: public static final Logger getLogger(String name, Module module) {
>> 425: BootstrapLogger.detectBackend();
>
> Suggestion:
>
> // triggers detection of the backend
> BootstrapLogger.detectBackend();
Hello Daniel, Sean, I couldn't understand the need for this method. The changes
to `BootstrapLogger` in this PR removes the initialization of `DetectBackend`
class while holding a lock on `BootstrapLogger` class in the
`BootstrapLogger.useLazyLoggers` method. Wouldn't that be enough?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1305493743
Re: RFR: 8314949: linux PPC64 Big Endian: Implementation of Foreign Function & Memory API [v2]
> I've found a way to solve the remaining FFI problem on linux PPC64 Big > Endian. Large structs (>8 Bytes) which are passed in registers or on stack > require shifting the Bytes in the last slot if the size is not a multiple of > 8. This PR adds the required functionality to the Java code. > > Please review and provide feedback. There may be better ways to implement it. > I just found one which works and makes the tests pass: > > Test summary > == >TEST TOTAL PASS FAIL ERROR > >jtreg:test/jdk/java/foreign 8888 0 0 > > > > Note: This PR should be considered as preparation work for AIX which also > uses ABIv1. Martin Doerr has updated the pull request incrementally with one additional commit since the last revision: Implement ABI version selection by virtual method instead of instanceof check. - Changes: - all: https://git.openjdk.org/jdk/pull/15417/files - new: https://git.openjdk.org/jdk/pull/15417/files/5d7b0e1d..50144b14 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15417&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15417&range=00-01 Stats: 16 lines in 3 files changed: 13 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/15417.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15417/head:pull/15417 PR: https://git.openjdk.org/jdk/pull/15417
Re: RFR: 8314949: linux PPC64 Big Endian: Implementation of Foreign Function & Memory API [v2]
On Fri, 25 Aug 2023 09:01:43 GMT, Maurizio Cimadamore wrote: >> I had something like that, but another reviewer didn't like it, either. >> Originally, I had thought that the v1 and v2 CallArrangers would get more >> content, but they're still empty. Would it be better to remove these special >> CallArrangers and distinguish in the base class? > > It seems to me that what you are doing is similar to what was done for > aarch64, which was dealt with using very simple subclasses: > https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/linux/LinuxAArch64CallArranger.java > https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/macos/MacOsAArch64CallArranger.java > https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/windows/WindowsAArch64CallArranger.java > > In your case there's less difference, but I think we should follow the same > idiom for both. Makes sense. I've changed it with the 2nd commit. - PR Review Comment: https://git.openjdk.org/jdk/pull/15417#discussion_r1305495063
Re: RFR: 8314236: Overflow in Collections.rotate [v7]
> `Collections.rotate` method contains a bug. This method throws
> IndexOutOfBoundsException on arrays larger than $2^{30}$ elements. The way to
> reproduce:
>
> final int size = (1 << 30) + 1;
> final List list = new ArrayList<>(size);
> for (int i = 0; i < size; ++i)
> list.add((byte) 0);
> Collections.rotate(list, size - 1);
>
> Output:
> ```Exception in thread "main" java.lang.IndexOutOfBoundsException: Index
> -2147483648 out of bounds for length 1073741825```
>
> In that case private method `Collections.rotate1` will be called. And the
> line:
> `i += distance;`
> will cause overflow. I fixed this method and wrote a test for it.
>
> I've signed the Oracle Contributor Agreement, but I don't have permission to
> raise a bug in the JDK Bug System.
>
> Kindly ask you to raise a bug.
Nikita Sakharin has updated the pull request incrementally with 141 additional
commits since the last revision:
- 8314990: Generational ZGC: Strong OopStorage stats reported as weak roots
Reviewed-by: stefank, eosterlund
- 8312749: Generational ZGC: Tests crash with assert(index == 0 ||
is_power_of_2(index))
Co-authored-by: Stefan Karlsson
Co-authored-by: Erik Österlund
Reviewed-by: thartmann, ayang, kvn
- 8314951: VM build without C2 still fails after JDK-8313530
Reviewed-by: dnsimon, kvn
- 8219567: Name of first parameter of RandomAccessFile(String,String) is
inconsistent
Reviewed-by: jlu, vtewari, rriggs, jpai
- 8314850: SharedRuntime::handle_wrong_method() gets called too often when
resolving Continuation.enter
Reviewed-by: rpressler, aph
- 8314759: VirtualThread.parkNanos timeout adjustment when pinned should be
replaced
Reviewed-by: aturbanov, shade, dfuchs
- 8306040: HttpResponseInputStream.available() returns 1 on empty stream
Reviewed-by: dfuchs
- 8314656: GHA: No need for Debian ports keyring installation after JDK-8313701
Reviewed-by: fyang
- 8314554: Debian/Ubuntu should not link OpenJDK with --as-needed link option
Reviewed-by: erikj
- 8314883: Java_java_util_prefs_FileSystemPreferences_lockFile0 write result
errno in missing case
Reviewed-by: jpai, shade, vtewari
- ... and 131 more: https://git.openjdk.org/jdk/compare/69672701...b49bf280
-
Changes:
- all: https://git.openjdk.org/jdk/pull/15270/files
- new: https://git.openjdk.org/jdk/pull/15270/files/69672701..b49bf280
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=15270&range=06
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=15270&range=05-06
Stats: 14502 lines in 618 files changed: 9508 ins; 1142 del; 3852 mod
Patch: https://git.openjdk.org/jdk/pull/15270.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/15270/head:pull/15270
PR: https://git.openjdk.org/jdk/pull/15270
Re: RFR: 8314949: linux PPC64 Big Endian: Implementation of Foreign Function & Memory API [v3]
> I've found a way to solve the remaining FFI problem on linux PPC64 Big > Endian. Large structs (>8 Bytes) which are passed in registers or on stack > require shifting the Bytes in the last slot if the size is not a multiple of > 8. This PR adds the required functionality to the Java code. > > Please review and provide feedback. There may be better ways to implement it. > I just found one which works and makes the tests pass: > > Test summary > == >TEST TOTAL PASS FAIL ERROR > >jtreg:test/jdk/java/foreign 8888 0 0 > > > > Note: This PR should be considered as preparation work for AIX which also > uses ABIv1. Martin Doerr has updated the pull request incrementally with one additional commit since the last revision: Remove unnecessary imports. - Changes: - all: https://git.openjdk.org/jdk/pull/15417/files - new: https://git.openjdk.org/jdk/pull/15417/files/50144b14..430fa018 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15417&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15417&range=01-02 Stats: 5 lines in 3 files changed: 0 ins; 5 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/15417.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15417/head:pull/15417 PR: https://git.openjdk.org/jdk/pull/15417
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError [v2]
On Wed, 23 Aug 2023 17:16:15 GMT, Daniel Fuchs wrote: > We could create a singleton instance of TemporaryLoggerFinder in the > TemporaryLoggerFinder class and return that. I think the disadvantage is that this singleton instance will never be GCed (till `LoggerFinderLoader` class itself is unloaded) unlike just using a new instance at the call site. Having said that, not being GCed doesn't make a huge difference given what this singleton instance does or holds on to. - PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1305512530
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError [v2]
On Thu, 24 Aug 2023 10:54:19 GMT, Sean Coffey wrote:
>> Recursive initialization calls possible during loading of LoggerFinder
>> service.
>>
>> This fix detects the recursive call and returns a temporary LoggerFinder
>> that is backed by a lazy logger. Automated test case developed to simulate
>> loading of an external LoggerFinder service while also having other threads
>> poke System.getLogger during this framework initialization.
>
> Sean Coffey has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Improve test coverage
> - Incorporate review comments from Daniel
test/jdk/java/lang/System/LoggerFinder/SignedLoggerFinderTest/SignedLoggerFinderTest.java
line 109:
> 107:
> Boolean.parseBoolean(System.getProperty("mutliThreadLoad", "false"));
> 108: boolean withCustomLoggerFinder =
> 109:
> Boolean.parseBoolean(System.getProperty("withCustomLoggerFinder", "false"));
Nit - these two calls can be replaced with
`Boolean.getBoolean("")`. If you however want to use this current
form, that's fine too.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1305525527
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError [v2]
On Thu, 24 Aug 2023 10:54:19 GMT, Sean Coffey wrote:
>> Recursive initialization calls possible during loading of LoggerFinder
>> service.
>>
>> This fix detects the recursive call and returns a temporary LoggerFinder
>> that is backed by a lazy logger. Automated test case developed to simulate
>> loading of an external LoggerFinder service while also having other threads
>> poke System.getLogger during this framework initialization.
>
> Sean Coffey has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Improve test coverage
> - Incorporate review comments from Daniel
test/jdk/java/lang/System/LoggerFinder/SignedLoggerFinderTest/SignedLoggerFinderTest.java
line 122:
> 120: Thread.sleep(sleep);
> 121: } catch (InterruptedException e) {
> 122: throw new RuntimeException(e);
Given that this will end up being thrown from a `Thread`, this will end up
being an uncaught exception and handled by a `UncaughtExceptionHandler` (I
don't remember if/what jtreg sets it to). The default `ThreadGroup`
UncaughtExceptionHandler, just logs to System.err such exceptions
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/ThreadGroup.java#L697.
Just noting it here, in case you want to do this differently if you want this
exception to fail the test.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1305534996
Re: RFR: 8288899: java/util/concurrent/ExecutorService/CloseTest.java failed with "InterruptedException: sleep interrupted" [v15]
> Addresses Jdk 8288899 : java/util/concurrent/ExecutorService/CloseTest.java > failed with "InterruptedException: sleep interrupted" and related issues. > > This is a major ForkJoin update (and hard to review -- sorry) that finally > addresses incompatibilities between ExecutorService and ForkJoinPool (which > claims to implement it), with the goal of avoiding continuing bug reports and > incompatibilities. Doing this required reworking internal control to use > phaser/seqlock-style versioning schemes (affecting nearly every method) that > ensure consistent data structures and actions without requiring global > synchronization or locking on every task execution that would massively > degrade performance. The previous lack of a solution to this was the main > reason for these incompatibilities. Doug Lea has updated the pull request incrementally with one additional commit since the last revision: Isolate unexpected interrupt status issues - Changes: - all: https://git.openjdk.org/jdk/pull/14301/files - new: https://git.openjdk.org/jdk/pull/14301/files/9d1b55a7..9a329b6c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14301&range=14 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14301&range=13-14 Stats: 119 lines in 3 files changed: 112 ins; 1 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/14301.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14301/head:pull/14301 PR: https://git.openjdk.org/jdk/pull/14301
Withdrawn: 8311084: Add typeSymbol() API for applicable constant pool entries
On Thu, 29 Jun 2023 09:59:30 GMT, Chen Liang wrote: > 5 Constant Pool entries, namely ConstantDynamicEntry, InvokeDynamicEntry, > FieldRefEntry, MethodRefEntry, and InterfaceMethodRefEntry should have a > typeSymbol() API to return the nominal/symbolic descriptor (ClassDesc or > MethodTypeDesc). > > This API is not added to NameAndTypeEntry itself, for a NameAndTypeEntry only > knows if its type should be a field or method type from the other entries > that refer to it. > > This is one of the issues discussed in this mailing list thread: > https://mail.openjdk.org/pipermail/classfile-api-dev/2023-June/000381.html This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/14706
Integrated: 8314063 : The socket is not closed in Connection::createSocket when the handshake failed for LDAP connection
On Tue, 15 Aug 2023 17:30:54 GMT, Weibing Xiao wrote: > Please refer to JDK-8314063. > > The failure scenario is due to the setting of connection timeout. It is > either too small or not an optimal value for the system. When the client > tries to connect to the server with LDAPs protocol. It requires the handshake > after the socket is created and connected, but it fails due to connection > timeout and leaves the socket open. It is not closed properly due to the > exception handling in the JDK code. > > The change is adding a try/catch block and closing the socket in the catch > block, and the format of the code got changed consequently. This pull request has now been integrated. Changeset: f2383b3c Author:Weibing Xiao Committer: Aleksei Efimov URL: https://git.openjdk.org/jdk/commit/f2383b3cbd1096f0b38e89a3d876da2217511f11 Stats: 357 lines in 3 files changed: 290 ins; 42 del; 25 mod 8314063: The socket is not closed in Connection::createSocket when the handshake failed for LDAP connection Reviewed-by: aefimov, msheppar - PR: https://git.openjdk.org/jdk/pull/15294
Withdrawn: 8307149: MethodHandles.arrayConstructor can be cached
On Thu, 4 May 2023 23:29:17 GMT, Chen Liang wrote: > This patch migrates `MethodHandles::arrayConstructor`, added in Java 9 as a > hotfix to the incorrect constructor found on arrays via Lookup, to share the > array access caching features. The result is that calling > `MethodHandles.arrayConstructor` to create method handles is much faster. > > Oracle JDK 20: > > Benchmark Mode Cnt Score > Error Units > MethodHandlesArrayConstructor.createWithAnewarray avgt 15 2.739 ± > 0.058 ns/op > MethodHandlesArrayConstructor.createWithMethodHandle avgt 15 3.313 ± > 0.041 ns/op > MethodHandlesArrayConstructor.methodHandleCreationavgt 15 61.874 ± > 0.397 ns/op > > > This patch: > > Benchmark Mode Cnt Score > Error Units > MethodHandlesArrayConstructor.createWithAnewarray avgt 15 3.067 ± > 0.026 ns/op > MethodHandlesArrayConstructor.createWithMethodHandle avgt 15 3.699 ± > 0.042 ns/op > MethodHandlesArrayConstructor.methodHandleCreationavgt 15 1.447 ± > 0.004 ns/op This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/13819
Re: RFR: 8314483: Optionally override copyright header in generated source [v2]
On Fri, 25 Aug 2023 06:45:25 GMT, Andrey Turbanov wrote: >> Erik Joelsson has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Suggestion from dholmes > > make/jdk/src/classes/build/tools/cldrconverter/CLDRConverter.java line 236: > >> 234: case "-jdk-header-template": >> 235: jdkHeaderTemplate = new String( >> 236: Files.readAllBytes(Paths.get(args[++i])), > > Can `java.nio.file.Files#readString(java.nio.file.Path)` be used instead? Ah yes, I was looking for that method and didn't find it, but I now realize I was looking in the jdk 8 api. Punished for not setting up an IDE for editing these files. Fixed now, thanks! - PR Review Comment: https://git.openjdk.org/jdk/pull/15346#discussion_r1305638090
Re: RFR: 8314483: Optionally override copyright header in generated source [v3]
> In the JDK build we have various build tools that generate source code from > data files. For most of these tools, the source files are based on template > files, which already have copyright headers, but for some, the complete > source file is generated by the tool, which is providing the copyright header > programatically. For the latter, we would like to implement an override > mechanism in each tool so that we can change the copyright header from a > custom makefile. Erik Joelsson has updated the pull request incrementally with one additional commit since the last revision: Use Files.readString - Changes: - all: https://git.openjdk.org/jdk/pull/15346/files - new: https://git.openjdk.org/jdk/pull/15346/files/a53cf2f2..8ab213a0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15346&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15346&range=01-02 Stats: 7 lines in 2 files changed: 0 ins; 5 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/15346.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15346/head:pull/15346 PR: https://git.openjdk.org/jdk/pull/15346
Re: RFR: 8314236: Overflow in Collections.rotate [v8]
> `Collections.rotate` method contains a bug. This method throws
> IndexOutOfBoundsException on arrays larger than $2^{30}$ elements. The way to
> reproduce:
>
> final int size = (1 << 30) + 1;
> final List list = new ArrayList<>(size);
> for (int i = 0; i < size; ++i)
> list.add((byte) 0);
> Collections.rotate(list, size - 1);
>
> Output:
> ```Exception in thread "main" java.lang.IndexOutOfBoundsException: Index
> -2147483648 out of bounds for length 1073741825```
>
> In that case private method `Collections.rotate1` will be called. And the
> line:
> `i += distance;`
> will cause overflow. I fixed this method and wrote a test for it.
>
> I've signed the Oracle Contributor Agreement, but I don't have permission to
> raise a bug in the JDK Bug System.
>
> Kindly ask you to raise a bug.
Nikita Sakharin has updated the pull request with a new target base due to a
merge or a rebase. The incremental webrev excludes the unrelated changes
brought in by the merge/rebase. The pull request contains one additional commit
since the last revision:
fix overflow in Collections.rotate and write tests
-
Changes:
- all: https://git.openjdk.org/jdk/pull/15270/files
- new: https://git.openjdk.org/jdk/pull/15270/files/b49bf280..a7d7722b
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=15270&range=07
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=15270&range=06-07
Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod
Patch: https://git.openjdk.org/jdk/pull/15270.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/15270/head:pull/15270
PR: https://git.openjdk.org/jdk/pull/15270
Re: RFR: 8309130: x86_64 AVX512 intrinsics for Arrays.sort methods (int, long, float and double arrays) [v29]
On Fri, 25 Aug 2023 01:57:41 GMT, Srinivas Vamsi Parasa wrote: >> The goal is to develop faster sort routines for x86_64 CPUs by taking >> advantage of AVX512 instructions. This enhancement provides an order of >> magnitude speedup for Arrays.sort() using int, long, float and double arrays. >> >> This PR shows upto ~7x improvement for 32-bit datatypes (int, float) and >> upto ~4.5x improvement for 64-bit datatypes (long, double) as shown in the >> performance data below. >> >> >> **Arrays.sort performance data using JMH benchmarks for arrays with random >> data** >> >> |Arrays.sort benchmark | Array Size | Baseline >> (us/op)| AVX512 Sort (us/op) | Speedup | >> |--- | --- | --- | --- | --- >> | >> |ArraysSort.doubleSort | 10 | 0.034 | 0.035 >> | 1.0 | >> |ArraysSort.doubleSort | 25 | 0.116 | 0.089 >> | 1.3 | >> |ArraysSort.doubleSort | 50 | 0.282 | 0.291 >> | 1.0 | >> |ArraysSort.doubleSort | 75 | 0.474 | 0.358 >> | 1.3 | >> |ArraysSort.doubleSort | 100 | 0.654 | 0.623 >> | 1.0 | >> |ArraysSort.doubleSort | 1000| 9.274 | 6.331 >> | 1.5 | >> |ArraysSort.doubleSort | 1 | 323.339 | 71.228 >> | **4.5** | >> |ArraysSort.doubleSort | 10 | 4471.871| >> 1002.748| **4.5** | >> |ArraysSort.doubleSort | 100 | 51660.742 | >> 12921.295 | **4.0** | >> |ArraysSort.floatSort| 10 | 0.045 | 0.046 >> | 1.0 | >> |ArraysSort.floatSort| 25 | 0.103 | 0.084 >> | 1.2 | >> |ArraysSort.floatSort| 50 | 0.285 | 0.33 >> | 0.9 | >> |ArraysSort.floatSort| 75 | 0.492 | 0.346 >> | 1.4 | >> |ArraysSort.floatSort| 100 | 0.597 | 0.326 >> | 1.8 | >> |ArraysSort.floatSort| 1000| 9.811 | 5.294 >> | 1.9 | >> |ArraysSort.floatSort| 1 | 323.955 | 50.547 >> | **6.4** | >> |ArraysSort.floatSort| 10 | 4326.38 | 731.152 >> | **5.9** | >> |ArraysSort.floatSort| 100 | 52413.88| >> 8409.193| **6.2** | >> |ArraysSort.intSort | 10 | 0.033 | 0.033 >> | 1.0 | >> |ArraysSort.intSort | 25 | 0.086 | 0.051 >> | 1.7 | >> |ArraysSort.intSort | 50 | 0.236 | 0.151 >> | 1.6 | >> |ArraysSort.intSort | 75 | 0.416 | 0.332 >> | 1.3 | >> |ArraysSort.intSort | 100 | 0.63| 0.521 >> | 1.2 | >> |ArraysSort.intSort | 1000| 10.518 | 4.698 >> | 2.2 | >> |ArraysSort.intSort | 1 | 309.659 | 42.518 >> | **7.3** | >> |ArraysSort.intSort | 10 | 4130.917| >> 573.956 | **7.2** | >> |ArraysSort.intSort | 100 | 49876.307 | >> 6712.812| **7.4** | >> |ArraysSort.longSort | 10 | 0.036 | 0.037 >> | 1.0 | >> |ArraysSort.longSort | 25 | 0.094 | 0.08 >> | 1.2 | >> |ArraysSort.longSort | 50 | 0.218 | 0.227 >> | 1.0 | >> |ArraysSort.longSort | 75 | 0.466 | 0.402 >> | 1.2 | >> |ArraysSort.longSort | 100 | 0.76| 0.58 >> | 1.3 | >> |ArraysSort.longSort | 1000| 10.449 | 6 > > Srinivas Vamsi Parasa has updated the pull request incrementally with one > additional commit since the last revision: > > Remove unnecessary import in Arrays.java make/modules/java.base/Lib.gmk line 239: > 237: > > 238: > 239: ifeq ($(call isTargetOs, linux)+$(call isTargetCpu, > x86_64)+$(INCLUDE_COMPILER2), true+true+true) Is there a reason for this to only be supported on Linux? - PR Review Comment: https://git.openjdk.org/jdk/pull/14227#discussion_r1305652232
Re: RFR: 8314940: Use of NIO in JDKs Metrics implementation causes issues in GraalVM
On Fri, 25 Aug 2023 10:04:28 GMT, Alan Bateman wrote: > In this case, it seems a bit fragile to do it in CgroupUtil as it should be > allowed to use any of the file system APIs to access cgroups or proc files. In theory, yes. It should be able to use any file system APIs. Practically, it doesn't make a whole lot of difference :-) Going by the last few years this area of the code hasn't had many updates. > Part of me wonders if this would be better handled in their implementation of > jdk.internal.misc.VM or Runtime.maxMemory but maybe you've been there already? Implementing `Runtime.maxMemory` (determine the configured max heap size), usually needs some notion of 'Physical Memory', since the heap size is being based on 'Physical Memory' if not explicitly set. As with HotSpot, that notion of 'Physical Memory' might depend on whether or not you're running in a container (usually with a single process in that container) or not. The GraalVM implementation uses the JDK cgroups code to figure out the 'Physical Memory' in the container case. I don't think there is a way to implement `Runtime.maxMemory` without knowing `Physical Memory'. This isn't a problem in OpenJDK (yet), since HotSpot has its own implementation in its runtime written in C. That has its own set of problems, since we need to update both implementations when bugs come in. In a way, this boils down to ByteBuffers using max heap size for it's default direct memory size determination. I'm not sure doing something else in GraalVM for determining the default direct memory size would be any better. All things considered, breaking the cycle in OpenJDK seems reasonable to me. Would this convince you to accept this patch? - PR Comment: https://git.openjdk.org/jdk/pull/15416#issuecomment-1693364297
Re: RFR: 8314236: Overflow in Collections.rotate [v4]
On Thu, 24 Aug 2023 02:36:52 GMT, Stuart Marks wrote: >> Nikita Sakharin has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8314236: rewrite test > > @nikita-sakharin > > Thanks for the updates. With the "Mock List" implementation we can run the > test in-JVM and we can avoid allocating several GB of memory. Great! > > The implementation logic in the `rotate1` method looks correct. > > Now that an individual test case is much less expensive, it becomes feasible > to add multiple test cases. In particular, for this kind of testing of > arithmetic errors, I like to test a variety of edge cases. The one test case > you have is for size=`(1 << 30) - 1` and distance=`(1 << 30)`. It would be > good to have a few other cases where the existing code fails and where the > modified code should pass. I was able to come up with a few examples quickly: > > size distance > Integer.MAX_VALUE2 > Integer.MAX_VALUEInteger.MIN_VALUE > Integer.MAX_VALUEInteger.MAX_VALUE - 1 > > Please add these cases, and any others that you think might be interesting. @stuart-marks Thank you for your review. Test was failing due to [JDK-8313701](https://bugs.openjdk.org/browse/JDK-8313701). So, I pulled changes from the `master` and squash my commits. I increased test coverage with respect to your advice. All tests passed successfully. Awaiting the next step of your review. - PR Comment: https://git.openjdk.org/jdk/pull/15270#issuecomment-1693385541
Re: RFR: 8268829: Provide an optimized way to walk the stack with Class object only [v5]
On Thu, 24 Aug 2023 18:44:14 GMT, Mandy Chung wrote:
>> 8268829: Provide an optimized way to walk the stack with Class object only
>>
>> `StackWalker::walk` creates one `StackFrame` per frame and the current
>> implementation
>> allocates one `StackFrameInfo` and one `MemberName` objects per frame. Some
>> frameworks
>> like logging may only interest in the Class object but not the method name
>> nor the BCI,
>> for example, filters out its implementation classes to find the caller
>> class. It's
>> similar to `StackWalker::getCallerClass` but allows a predicate to filter
>> out the element.
>>
>> This PR proposes to add `StackWalker.Kind` enum to specify the information
>> that a stack walker
>> collects. If no method information is needed, a `StackWalker` of
>> `CLASS_INFO` can be used
>> instead and such stack walker will save the overhead (1) to extract the
>> method information
>> and (2) the memory used for the stack walking. In addition, this can also
>> fix
>>
>> - [8311500](https://bugs.openjdk.org/browse/JDK-8311500):
>> StackWalker.getCallerClass() throws UOE if invoked reflectively
>>
>> New factory methods to take a parameter to specify the kind of stack walker
>> to be created are defined.
>> This provides a simple way for existing code, for example logging
>> frameworks, to take advantage of
>> this enhancement with the least change as it can keep the existing function
>> for traversing
>> `StackFrame`s.
>>
>> For example: to find the first caller filtering a known list of
>> implementation class,
>> existing code can call `StackWalker::getInstance(CLASS_INFO, ...)` to create
>> a stack walker instance:
>>
>>
>> StackWalker walker = StackWalker.getInstance(Kind.CLASS_INFO,
>> Option.RETAIN_CLASS_REFERENCE);
>> Optional> callerClass = walker.walk(s ->
>> s.map(StackFrame::getDeclaringClass)
>> .filter(interestingClasses::contains)
>> .findFirst());
>>
>>
>> If method information is accessed on the `StackFrame`s produced by this
>> stack walker such as
>> `StackFrame::getMethodName`, then `UnsupportedOperationException` will be
>> thrown.
>>
>> Alternatives Considered
>> One alternative is to provide a new API:
>> ` T walkClass(Function, ? extends T> function)`
>>
>> In this case, the caller would need to pass a function that takes a stream
>> of `Class` object instead of `StackFrame`. Existing code would have to
>> modify calls to the `walk` method to `walkClass` and the function body.
>>
>> Another alternative is to add a new `NO_METHOD_INFO` option. Similar to the
>> proposed API,
>>...
>
> Mandy Chung has updated the pull request incrementally with one additional
> commit since the last revision:
>
> review feedback and javadoc clean up
src/hotspot/share/classfile/javaClasses.cpp line 2990:
> 2988: // direct calls for private and/or final non-static methods.
> 2989: flags |= java_lang_invoke_MemberName::MN_IS_METHOD;
> 2990: }
Both the is_static and the else block set
java_lang_invoke_MemberName::MN_IS_METHOD. Do we need to differentiate between
the two cases or can they be collapsed?
src/java.base/share/classes/java/lang/ClassFrameInfo.java line 48:
> 46: }
> 47: boolean isHidden() {
> 48: return
> SharedSecrets.getJavaLangInvokeAccess().isHiddenMember(flags &
> MEMBER_INFO_FLAGS);
Is it better to cache the JLIA in a static final similar to what StackFrameInfo
does?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1305670313
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1305670016
Re: RFR: 8288899: java/util/concurrent/ExecutorService/CloseTest.java failed with "InterruptedException: sleep interrupted" [v16]
> Addresses Jdk 8288899 : java/util/concurrent/ExecutorService/CloseTest.java > failed with "InterruptedException: sleep interrupted" and related issues. > > This is a major ForkJoin update (and hard to review -- sorry) that finally > addresses incompatibilities between ExecutorService and ForkJoinPool (which > claims to implement it), with the goal of avoiding continuing bug reports and > incompatibilities. Doing this required reworking internal control to use > phaser/seqlock-style versioning schemes (affecting nearly every method) that > ensure consistent data structures and actions without requiring global > synchronization or locking on every task execution that would massively > degrade performance. The previous lack of a solution to this was the main > reason for these incompatibilities. Doug Lea has updated the pull request incrementally with one additional commit since the last revision: Conditionalize new tests - Changes: - all: https://git.openjdk.org/jdk/pull/14301/files - new: https://git.openjdk.org/jdk/pull/14301/files/9a329b6c..5a4131f7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14301&range=15 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14301&range=14-15 Stats: 38 lines in 1 file changed: 4 ins; 0 del; 34 mod Patch: https://git.openjdk.org/jdk/pull/14301.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14301/head:pull/14301 PR: https://git.openjdk.org/jdk/pull/14301
CFV: New Core Libraries Group Member: Daniel Fuchs
I hereby nominate Daniel Fuchs to Membership in the Core Libraries Group Daniel has been contributing to the OpenJDK Core Libraries at Oracle since 2012. He is leading the networking team and has made nearly 400 contributions to OpenJDK since JDK 6. Votes are due by September 8th, 2023. Only current Members of the Core Libraries Group [1] are eligible to vote on this nomination. Votes must be cast in the open by replying to this mailing list. For Lazy Consensus voting instructions, see [2]. Roger Riggs [1] https://openjdk.org/census#core-libs [2] https://openjdk.org/groups/#member-vote
CFV: New Core Libraries Group Member: Lance Andersen
I hereby nominate Lance Andersen to Membership in the Core Libraries Group Lance has been contributing to the OpenJDK at Oracle since 2007. He has been the JDBC spec lead since 2005 and an OpenJDK committer since day 1. He has extensive experience with Java EE, Databases and SQL and is improving the integrity and performance of Zip and JAR implementations. Votes are due by September 8th, 2023. Only current Members of the Core Libraries Group [1] are eligible to vote on this nomination. Votes must be cast in the open by replying to this mailing list. For Lazy Consensus voting instructions, see [2]. Roger Riggs [1] https://openjdk.org/census#core-libs [2] https://openjdk.org/groups/#member-vote||
CFV: New Core Libraries Group Member: Michael McMahon
I hereby nominate Michael McMahon to Membership in the Core Libraries Group Michael has been contributing to the OpenJDK Core Libraries since 2008, originally at Sun Microsystems, now Oracle. He has deep networking experience and has made more than 260 contributions to OpenJDK. Votes are due by September 8th, 2023. Only current Members of the Core Libraries Group [1] are eligible to vote on this nomination. Votes must be cast in the open by replying to this mailing list For Lazy Consensus voting instructions, see [2]. Roger Riggs [1] https://openjdk.org/census#core-libs [2] https://openjdk.org/groups/#member-vote
Re: CFV: New Core Libraries Group Member: Daniel Fuchs
Vote: yes
Re: CFV: New Core Libraries Group Member: Lance Andersen
Vote: yes
Re: CFV: New Core Libraries Group Member: Michael McMahon
Vote: yes
Re: CFV: New Core Libraries Group Member: Daniel Fuchs
Vote: yes -Joe On 8/25/23 8:23 AM, Roger Riggs wrote: I hereby nominate Daniel Fuchs to Membership in the Core Libraries Group
Re: CFV: New Core Libraries Group Member: Michael McMahon
Vote: yes -Joe On 8/25/23 8:24 AM, Roger Riggs wrote: I hereby nominate Michael McMahon to Membership in the Core Libraries Group
Re: CFV: New Core Libraries Group Member: Lance Andersen
Vote: yes -Joe On 8/25/23 8:23 AM, Roger Riggs wrote: I hereby nominate Lance Andersen to Membership in the Core Libraries Group
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError [v2]
On Thu, 24 Aug 2023 10:54:19 GMT, Sean Coffey wrote: >> Recursive initialization calls possible during loading of LoggerFinder >> service. >> >> This fix detects the recursive call and returns a temporary LoggerFinder >> that is backed by a lazy logger. Automated test case developed to simulate >> loading of an external LoggerFinder service while also having other threads >> poke System.getLogger during this framework initialization. > > Sean Coffey has updated the pull request incrementally with two additional > commits since the last revision: > > - Improve test coverage > - Incorporate review comments from Daniel Thanks for your comments @jai - I'll incorporate your test comments soon. - PR Comment: https://git.openjdk.org/jdk/pull/15404#issuecomment-1693595951
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError [v3]
> Recursive initialization calls possible during loading of LoggerFinder > service. > > This fix detects the recursive call and returns a temporary LoggerFinder that > is backed by a lazy logger. Automated test case developed to simulate loading > of an external LoggerFinder service while also having other threads poke > System.getLogger during this framework initialization. Sean Coffey has updated the pull request incrementally with one additional commit since the last revision: fix up bootstrap loggers, patch contribution from Daniel - Changes: - all: https://git.openjdk.org/jdk/pull/15404/files - new: https://git.openjdk.org/jdk/pull/15404/files/976fdb27..3b35c440 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15404&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15404&range=01-02 Stats: 70 lines in 3 files changed: 23 ins; 32 del; 15 mod Patch: https://git.openjdk.org/jdk/pull/15404.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15404/head:pull/15404 PR: https://git.openjdk.org/jdk/pull/15404
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError [v2]
On Fri, 25 Aug 2023 10:24:23 GMT, Jaikiran Pai wrote:
>> Sean Coffey has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Improve test coverage
>> - Incorporate review comments from Daniel
>
> src/java.base/share/classes/jdk/internal/logger/BootstrapLogger.java line 988:
>
>> 986: private static void ensureClassInitialized(Class c) {
>> 987: try {
>> 988: MethodHandles.lookup().ensureInitialized(c);
>
> Hello Sean, should we check if there are any implications, like on startup
> performance, of using `MethodHandles` in this `BootstrapLogger`?
Thanks for the comments Jai. Latest patch just pushed resolved this - in any
case, it looks like the MethodHandles class is loaded very early in the module
system (even before the application code)
[0.026s][info][class,load] java.util.HexFormat source: shared objects file
[0.026s][info][class,load] java.util.concurrent.atomic.AtomicInteger source:
shared objects file
[0.026s][info][class,load] jdk.internal.module.ModuleBootstrap source: shared
objects file
[0.026s][info][class,load] java.lang.invoke.MethodHandles source: shared
objects file
[0.026s][info][class,load] java.lang.invoke.MemberName$Factory source: shared
objects file
[0.026s][info][class,load] java.lang.invoke.MethodHandles$Lookup source: shared
objects file
[0.027s][info][class,load] java.lang.StrictMath source: shared objects file
[0.032s][info][class,load] java.security.SecureClassLoader$DebugHolder source:
shared objects file
[0.032s][info][class,load] sun.security.util.Debug source: shared objects file
[0.033s][info][class,load] SignedLoggerFinderTest source:
file:/MYREPO/jdk/open/test/jdk/JTwork/classes/0/java/lang/System/LoggerFinder/SignedLoggerFinderTest/SignedLoggerFinderTest.d/
[0.033s][info][class,load] java.lang.NamedPackage source: shared objects file
[0.033s][info][class,load] jdk.internal.misc.MainMethodFinder source: shared
objects file
[0.033s][info][class,load] jdk.internal.misc.PreviewFeatures source: shared
objects file
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1305854273
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError [v3]
On Fri, 25 Aug 2023 10:33:49 GMT, Jaikiran Pai wrote:
>> src/java.base/share/classes/jdk/internal/logger/LazyLoggers.java line 425:
>>
>>> 423: */
>>> 424: public static final Logger getLogger(String name, Module module) {
>>> 425: BootstrapLogger.detectBackend();
>>
>> Suggestion:
>>
>> // triggers detection of the backend
>> BootstrapLogger.detectBackend();
>
> Hello Daniel, Sean, I couldn't understand the need for this method. The
> changes to `BootstrapLogger` in this PR removes the initialization of
> `DetectBackend` class while holding a lock on `BootstrapLogger` class in the
> `BootstrapLogger.useLazyLoggers` method. Wouldn't that be enough?
a deadlock is still possible Jai with forcing class initialization here. The
new auto test confirms.
here's the interesting calling stack :
"Thread-0" #31 [2241170] prio=5 os_prio=0 cpu=32.77ms elapsed=13.69s
tid=0x7fb0b019bf10 nid=2241170 waiting on condition [0x7fb01ac29000]
java.lang.Thread.State: RUNNABLE
at
jdk.internal.logger.BootstrapLogger.useLazyLoggers(java.base@22-internal/BootstrapLogger.java:952)
- waiting on the Class initialization monitor for
jdk.internal.logger.BootstrapLogger$DetectBackend
at
jdk.internal.logger.LazyLoggers.getLazyLogger(java.base@22-internal/LazyLoggers.java:462)
at
jdk.internal.logger.LazyLoggers.getLogger(java.base@22-internal/LazyLoggers.java:437)
at java.lang.System.getLogger(java.base@22-internal/System.java:1822)
at
jdk.internal.event.EventHelper.isLoggingSecurity(java.base@22-internal/EventHelper.java:148)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1305856639
Thanks for re-play
Hi there! I’ve e nominate my self to be a change provoker of this community! I’ve propose to stop kissing everyones butts only because of the history, go this evening to your favorite bar get drunk, talk to people about your beliefs and in the morning or next day assemble all your craziest hidden thoughts that you have documented inGoogle Keep about OOP in Java in form of proposal to discuss and share it soon with the community. Because the people in Core who do not believe in OOP will not improve it. All the tries interpreting of ‘_fast_invokevfinal’ approaches will not make significant change between Java 10 and 30 ambitions. You free to remove me from here, I appreciate you read it. Reboot your self. I’ve already have learned from you something. I appreciate it. ISelo means in Ukrainian - I’m village man who can ask stupid questions but sometimes they could be essential. Please vote, for your self! Kind regards and sorry for disturbing your. Young and stupid, but I believe ;) iselo
Re: CFV: New Core Libraries Group Member: Daniel Fuchs
Vote: yes Mandy
Re: CFV: New Core Libraries Group Member: Lance Andersen
Vote: Yes Mandy
Re: CFV: New Core Libraries Group Member: Michael McMahon
Vote: yes Mandy
Re: CFV: New Core Libraries Group Member: Michael McMahon
Vote: Yes On 8/25/23 11:24 AM, Roger Riggs wrote: I hereby nominate Michael McMahon to Membership in the Core Libraries Group
Re: CFV: New Core Libraries Group Member: Lance Andersen
Vote: Yes On 8/25/23 11:23 AM, Roger Riggs wrote: I hereby nominate Lance Andersen to Membership in the Core Libraries Group
Re: CFV: New Core Libraries Group Member: Daniel Fuchs
Vote: Yes On 8/25/23 11:23 AM, Roger Riggs wrote: I hereby nominate Daniel Fuchs to Membership in the Core Libraries Group
Re: RFR: 8268829: Provide an optimized way to walk the stack with Class object only [v5]
On Fri, 25 Aug 2023 13:35:04 GMT, Dan Heidinga wrote:
>> Mandy Chung has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> review feedback and javadoc clean up
>
> src/hotspot/share/classfile/javaClasses.cpp line 2990:
>
>> 2988: // direct calls for private and/or final non-static methods.
>> 2989: flags |= java_lang_invoke_MemberName::MN_IS_METHOD;
>> 2990: }
>
> Both the is_static and the else block set
> java_lang_invoke_MemberName::MN_IS_METHOD. Do we need to differentiate
> between the two cases or can they be collapsed?
yes they can be collapsed. I will update it.
> src/java.base/share/classes/java/lang/ClassFrameInfo.java line 48:
>
>> 46: }
>> 47: boolean isHidden() {
>> 48: return
>> SharedSecrets.getJavaLangInvokeAccess().isHiddenMember(flags &
>> MEMBER_INFO_FLAGS);
>
> Is it better to cache the JLIA in a static final similar to what
> StackFrameInfo does?
either way is fine. I can add a static field.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1305941281
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1305942726
Re: RFR: 8309130: x86_64 AVX512 intrinsics for Arrays.sort methods (int, long, float and double arrays) [v22]
On Thu, 24 Aug 2023 06:23:29 GMT, Srinivas Vamsi Parasa wrote: >>> Improvements are nice but it would not pay off if you have big regressions. >>> I can accept 0.9x but 0.4x - 0.8x regressions should be investigated and >>> implementation adjusted to avoid them. >> >> Hi Vladimir, >> >> Thank you for the suggestion! >> Currently, AVX512sort is doing well for Random, Repeated and Shuffle >> patterns of input data. The regressions are observed for Staggered (Wave) >> pattern of input data. >> Will investigate the regressions and adjust the implementations to address >> them. >> >> Thanks, >> Vamsi > >> Improvements are nice but it would not pay off if you have big regressions. >> I can accept 0.9x but 0.4x - 0.8x regressions should be investigated and >> implementation adjusted to avoid them. > > Hello Vladimir (@vnkozlov) , > > As per your suggestion, the implementation was adjusted to address the > regressions caused for STAGGER and REPEATED type of input data patterns. > Please see below the new JMH performance data using the adjusted > implementation. > > In the new implementation, we don't call the AVX512 sort intrinsic at the top > level (`Arrays.sort()`) . Instead, we take a decomposed or a 'building > blocks' approach where we only intrinsify (using AVX512 instructions) the > partitioning and small array sort functions used in the current baseline ( > `DualPivotQuickSort.sort()` ). Since the current baseline has logic to detect > and sort special input patterns like STAGGER, we fallback to the current > baseline instead of using AVX512 partitioning and sorting (which works best > for RANDOM, REPEATED and SHUFFLE patterns). > > Data below shows `Arrays.sort()` performance comparison between the current > **Java baseline (DPQS)** vs. **AVX512 sort** (this PR) using the > `ArraysSort.java` JMH > [benchmark](https://github.com/openjdk/jdk/pull/13568/files#diff-dee51b13bd1872ff455cec2f29255cfd25014a5dd33dda55a2fc68638c3dd4b2) > provided in the PR for [JDK-8266431: Dual-Pivot Quicksort improvements > (Radix sort)](https://github.com/openjdk/jdk/pull/13568/files#top) ( #13568) > > - The following command line was used to run the benchmarks: ` java -jar > $JDK_HOME/build/linux-x86_64-server-release/images/test/micro/benchmarks.jar > -jvmArgs "-XX:CompileThreshold=1 -XX:-TieredCompilation" ArraysSort` > - The scores shown are the average time (us/op), thus lower is better. The > last column towards the right shows the speedup. > > > | Benchmark | Mode| Size| Baseline DPQS > (us/op) | AVX512 partitioning & sort (us/op) | Speedup | > | --- | --- | --- | --- | --- > | --- | > | ArraysSort.Double.testSort | RANDOM | 800 | > 6.7 | 4.8 | 1.39x | > | ArraysSort.Double.testSort | RANDOM | 7000| > 234.1 | 51.5| **4.55x** | > | ArraysSort.Double.testSort | RANDOM | 5 | > 2155.9 | 470.0 | **4.59x** | > | ArraysSort.Double.testSort | RANDOM | 30 | > 15076.3 | 3391.3 | **4.45x** | > | ArraysSort.Double.testSort | RANDOM | 200 | > 116445.5| 27491.7 | **4.24x** | > | ArraysSort.Double.testSort | REPEATED| 800 > | 2.3 | 1.7 | 1.35x | > | ArraysSort.Double.testSort | REPEATED| 7000 > | 23.3| 12.1| **1.92x** | > | ArraysSort.Double.testSort |... @vamsi-parasa I submitted our testing of latest v28 version. It found issue in `ArraysSort.java` new benchmark file. You missed `,`after year in copyright line: * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved. - PR Comment: https://git.openjdk.org/jdk/pull/14227#issuecomment-1693790589
Re: RFR: 8309130: x86_64 AVX512 intrinsics for Arrays.sort methods (int, long, float and double arrays) [v29]
On Fri, 25 Aug 2023 01:57:41 GMT, Srinivas Vamsi Parasa wrote: >> The goal is to develop faster sort routines for x86_64 CPUs by taking >> advantage of AVX512 instructions. This enhancement provides an order of >> magnitude speedup for Arrays.sort() using int, long, float and double arrays. >> >> This PR shows upto ~7x improvement for 32-bit datatypes (int, float) and >> upto ~4.5x improvement for 64-bit datatypes (long, double) as shown in the >> performance data below. >> >> >> **Arrays.sort performance data using JMH benchmarks for arrays with random >> data** >> >> |Arrays.sort benchmark | Array Size | Baseline >> (us/op)| AVX512 Sort (us/op) | Speedup | >> |--- | --- | --- | --- | --- >> | >> |ArraysSort.doubleSort | 10 | 0.034 | 0.035 >> | 1.0 | >> |ArraysSort.doubleSort | 25 | 0.116 | 0.089 >> | 1.3 | >> |ArraysSort.doubleSort | 50 | 0.282 | 0.291 >> | 1.0 | >> |ArraysSort.doubleSort | 75 | 0.474 | 0.358 >> | 1.3 | >> |ArraysSort.doubleSort | 100 | 0.654 | 0.623 >> | 1.0 | >> |ArraysSort.doubleSort | 1000| 9.274 | 6.331 >> | 1.5 | >> |ArraysSort.doubleSort | 1 | 323.339 | 71.228 >> | **4.5** | >> |ArraysSort.doubleSort | 10 | 4471.871| >> 1002.748| **4.5** | >> |ArraysSort.doubleSort | 100 | 51660.742 | >> 12921.295 | **4.0** | >> |ArraysSort.floatSort| 10 | 0.045 | 0.046 >> | 1.0 | >> |ArraysSort.floatSort| 25 | 0.103 | 0.084 >> | 1.2 | >> |ArraysSort.floatSort| 50 | 0.285 | 0.33 >> | 0.9 | >> |ArraysSort.floatSort| 75 | 0.492 | 0.346 >> | 1.4 | >> |ArraysSort.floatSort| 100 | 0.597 | 0.326 >> | 1.8 | >> |ArraysSort.floatSort| 1000| 9.811 | 5.294 >> | 1.9 | >> |ArraysSort.floatSort| 1 | 323.955 | 50.547 >> | **6.4** | >> |ArraysSort.floatSort| 10 | 4326.38 | 731.152 >> | **5.9** | >> |ArraysSort.floatSort| 100 | 52413.88| >> 8409.193| **6.2** | >> |ArraysSort.intSort | 10 | 0.033 | 0.033 >> | 1.0 | >> |ArraysSort.intSort | 25 | 0.086 | 0.051 >> | 1.7 | >> |ArraysSort.intSort | 50 | 0.236 | 0.151 >> | 1.6 | >> |ArraysSort.intSort | 75 | 0.416 | 0.332 >> | 1.3 | >> |ArraysSort.intSort | 100 | 0.63| 0.521 >> | 1.2 | >> |ArraysSort.intSort | 1000| 10.518 | 4.698 >> | 2.2 | >> |ArraysSort.intSort | 1 | 309.659 | 42.518 >> | **7.3** | >> |ArraysSort.intSort | 10 | 4130.917| >> 573.956 | **7.2** | >> |ArraysSort.intSort | 100 | 49876.307 | >> 6712.812| **7.4** | >> |ArraysSort.longSort | 10 | 0.036 | 0.037 >> | 1.0 | >> |ArraysSort.longSort | 25 | 0.094 | 0.08 >> | 1.2 | >> |ArraysSort.longSort | 50 | 0.218 | 0.227 >> | 1.0 | >> |ArraysSort.longSort | 75 | 0.466 | 0.402 >> | 1.2 | >> |ArraysSort.longSort | 100 | 0.76| 0.58 >> | 1.3 | >> |ArraysSort.longSort | 1000| 10.449 | 6 > > Srinivas Vamsi Parasa has updated the pull request incrementally with one > additional commit since the last revision: > > Remove unnecessary import in Arrays.java After I fixed it Tier1 passed and I submitted other tiers. - PR Comment: https://git.openjdk.org/jdk/pull/14227#issuecomment-1693791251
Re: RFR: 8311864: Add ArraysSupport.hashCode(int[] a, fromIndex, length, initialValue)
On Tue, 11 Jul 2023 16:35:51 GMT, Pavel Rappo wrote: > This PR adds an internal method to calculate hash code from the provided > integer array, offset and length into that array, and the initial hash code > value. > > Current options for calculating a hash code for int[] are inflexible. It's > either ArraysSupport.vectorizedHashCode with an offset, length and initial > value, or Arrays.hashCode with just an array. > > For an arbitrary int[], unconditional vectorization might be unwarranted or > puzzling. Unfortunately, it's the only hash code method that operates on an > array subrange or accepts the initial hash code value. > > A more convenient method could be added and then used, for example, here: > > * > https://github.com/openjdk/jdk/blob/0ef03f122866f010ebf50683097e9b92e41cdaad/src/java.base/share/classes/java/util/concurrent/CopyOnWriteArrayList.java#L1076-L1083 > > * > https://github.com/openjdk/jdk/blob/0ef03f122866f010ebf50683097e9b92e41cdaad/src/java.base/share/classes/java/util/ArrayList.java#L669-L680 > > * > https://github.com/openjdk/jdk/blob/0ef03f122866f010ebf50683097e9b92e41cdaad/src/java.base/share/classes/sun/security/pkcs10/PKCS10.java#L356-L362 > > This PR adds such a method and provides tests for it. Additionally, this PR > adds tests for `null` passed to `java.util.Arrays.hashCode` overloads, > behaviour which was specified but untested. To Skara bots: keep this PR alive. - PR Comment: https://git.openjdk.org/jdk/pull/14831#issuecomment-1693802282
Re: RFR: 8309130: x86_64 AVX512 intrinsics for Arrays.sort methods (int, long, float and double arrays) [v22]
On Thu, 24 Aug 2023 06:23:29 GMT, Srinivas Vamsi Parasa wrote: >>> Improvements are nice but it would not pay off if you have big regressions. >>> I can accept 0.9x but 0.4x - 0.8x regressions should be investigated and >>> implementation adjusted to avoid them. >> >> Hi Vladimir, >> >> Thank you for the suggestion! >> Currently, AVX512sort is doing well for Random, Repeated and Shuffle >> patterns of input data. The regressions are observed for Staggered (Wave) >> pattern of input data. >> Will investigate the regressions and adjust the implementations to address >> them. >> >> Thanks, >> Vamsi > >> Improvements are nice but it would not pay off if you have big regressions. >> I can accept 0.9x but 0.4x - 0.8x regressions should be investigated and >> implementation adjusted to avoid them. > > Hello Vladimir (@vnkozlov) , > > As per your suggestion, the implementation was adjusted to address the > regressions caused for STAGGER and REPEATED type of input data patterns. > Please see below the new JMH performance data using the adjusted > implementation. > > In the new implementation, we don't call the AVX512 sort intrinsic at the top > level (`Arrays.sort()`) . Instead, we take a decomposed or a 'building > blocks' approach where we only intrinsify (using AVX512 instructions) the > partitioning and small array sort functions used in the current baseline ( > `DualPivotQuickSort.sort()` ). Since the current baseline has logic to detect > and sort special input patterns like STAGGER, we fallback to the current > baseline instead of using AVX512 partitioning and sorting (which works best > for RANDOM, REPEATED and SHUFFLE patterns). > > Data below shows `Arrays.sort()` performance comparison between the current > **Java baseline (DPQS)** vs. **AVX512 sort** (this PR) using the > `ArraysSort.java` JMH > [benchmark](https://github.com/openjdk/jdk/pull/13568/files#diff-dee51b13bd1872ff455cec2f29255cfd25014a5dd33dda55a2fc68638c3dd4b2) > provided in the PR for [JDK-8266431: Dual-Pivot Quicksort improvements > (Radix sort)](https://github.com/openjdk/jdk/pull/13568/files#top) ( #13568) > > - The following command line was used to run the benchmarks: ` java -jar > $JDK_HOME/build/linux-x86_64-server-release/images/test/micro/benchmarks.jar > -jvmArgs "-XX:CompileThreshold=1 -XX:-TieredCompilation" ArraysSort` > - The scores shown are the average time (us/op), thus lower is better. The > last column towards the right shows the speedup. > > > | Benchmark | Mode| Size| Baseline DPQS > (us/op) | AVX512 partitioning & sort (us/op) | Speedup | > | --- | --- | --- | --- | --- > | --- | > | ArraysSort.Double.testSort | RANDOM | 800 | > 6.7 | 4.8 | 1.39x | > | ArraysSort.Double.testSort | RANDOM | 7000| > 234.1 | 51.5| **4.55x** | > | ArraysSort.Double.testSort | RANDOM | 5 | > 2155.9 | 470.0 | **4.59x** | > | ArraysSort.Double.testSort | RANDOM | 30 | > 15076.3 | 3391.3 | **4.45x** | > | ArraysSort.Double.testSort | RANDOM | 200 | > 116445.5| 27491.7 | **4.24x** | > | ArraysSort.Double.testSort | REPEATED| 800 > | 2.3 | 1.7 | 1.35x | > | ArraysSort.Double.testSort | REPEATED| 7000 > | 23.3| 12.1| **1.92x** | > | ArraysSort.Double.testSort |... > @vamsi-parasa I submitted our testing of latest v28 version. It found issue > in `ArraysSort.java` new benchmark file. You missed `,`after year in > copyright line: > > ``` > * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved. > ``` Thank you, Vladimir! - PR Comment: https://git.openjdk.org/jdk/pull/14227#issuecomment-1693804429
Integrated: 8314483: Optionally override copyright header in generated source
On Fri, 18 Aug 2023 14:22:49 GMT, Erik Joelsson wrote: > In the JDK build we have various build tools that generate source code from > data files. For most of these tools, the source files are based on template > files, which already have copyright headers, but for some, the complete > source file is generated by the tool, which is providing the copyright header > programatically. For the latter, we would like to implement an override > mechanism in each tool so that we can change the copyright header from a > custom makefile. This pull request has now been integrated. Changeset: 837d2e1c Author:Erik Joelsson URL: https://git.openjdk.org/jdk/commit/837d2e1cac7e006506cd4cff03862d7fbcd42140 Stats: 46 lines in 4 files changed: 37 ins; 1 del; 8 mod 8314483: Optionally override copyright header in generated source Reviewed-by: dholmes, iris - PR: https://git.openjdk.org/jdk/pull/15346
Re: RFR: 8268829: Provide an optimized way to walk the stack with Class object only [v5]
On Thu, 24 Aug 2023 18:44:14 GMT, Mandy Chung wrote:
>> 8268829: Provide an optimized way to walk the stack with Class object only
>>
>> `StackWalker::walk` creates one `StackFrame` per frame and the current
>> implementation
>> allocates one `StackFrameInfo` and one `MemberName` objects per frame. Some
>> frameworks
>> like logging may only interest in the Class object but not the method name
>> nor the BCI,
>> for example, filters out its implementation classes to find the caller
>> class. It's
>> similar to `StackWalker::getCallerClass` but allows a predicate to filter
>> out the element.
>>
>> This PR proposes to add `StackWalker.Kind` enum to specify the information
>> that a stack walker
>> collects. If no method information is needed, a `StackWalker` of
>> `CLASS_INFO` can be used
>> instead and such stack walker will save the overhead (1) to extract the
>> method information
>> and (2) the memory used for the stack walking. In addition, this can also
>> fix
>>
>> - [8311500](https://bugs.openjdk.org/browse/JDK-8311500):
>> StackWalker.getCallerClass() throws UOE if invoked reflectively
>>
>> New factory methods to take a parameter to specify the kind of stack walker
>> to be created are defined.
>> This provides a simple way for existing code, for example logging
>> frameworks, to take advantage of
>> this enhancement with the least change as it can keep the existing function
>> for traversing
>> `StackFrame`s.
>>
>> For example: to find the first caller filtering a known list of
>> implementation class,
>> existing code can call `StackWalker::getInstance(CLASS_INFO, ...)` to create
>> a stack walker instance:
>>
>>
>> StackWalker walker = StackWalker.getInstance(Kind.CLASS_INFO,
>> Option.RETAIN_CLASS_REFERENCE);
>> Optional> callerClass = walker.walk(s ->
>> s.map(StackFrame::getDeclaringClass)
>> .filter(interestingClasses::contains)
>> .findFirst());
>>
>>
>> If method information is accessed on the `StackFrame`s produced by this
>> stack walker such as
>> `StackFrame::getMethodName`, then `UnsupportedOperationException` will be
>> thrown.
>>
>> Javadoc & specdiff
>>
>> https://cr.openjdk.org/~mchung/api/java.base/java/lang/StackWalker.html
>> https://cr.openjdk.org/~mchung/jdk22/specdiff/overview-summary.html
>>
>> Alternatives Considered
>> One alternative is to provide a new API:
>> ` T walkClass(Function, ? extends T> function)`
>>
>> In this case, the caller would need to pass a function that takes a stream
>> of `Class` object instead of `StackFrame`. Existing code would hav...
>
> Mandy Chung has updated the pull request incrementally with one additional
> commit since the last revision:
>
> review feedback and javadoc clean up
src/java.base/share/classes/java/lang/StackWalker.java line 55:
> 53: * available but not the {@link StackFrame#getDeclaringClass() Class
> reference}.
> 54: * The {@code Class} reference can be accessed if {@link
> Option#RETAIN_CLASS_REFERENCE}
> 55: * RETAIN_CLASS_REFERENCE} option is set.
Double `}` in the link.
src/java.base/share/classes/java/lang/StackWalker.java line 249:
> 247: *
> 248: * @throws UnsupportedOperationException if the {@code
> StackWalker} is of
> 249: * {@link Kind#CLASS_INFO CLASS_INFO} kind
Other CLASS_INFO methods use the javadoc:
if the StackWalker collects class only information
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1306129921
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1306143327
Re: RFR: 8309130: x86_64 AVX512 intrinsics for Arrays.sort methods (int, long, float and double arrays) [v29]
On Fri, 25 Aug 2023 01:57:41 GMT, Srinivas Vamsi Parasa
wrote:
>> The goal is to develop faster sort routines for x86_64 CPUs by taking
>> advantage of AVX512 instructions. This enhancement provides an order of
>> magnitude speedup for Arrays.sort() using int, long, float and double arrays.
>>
>> This PR shows upto ~7x improvement for 32-bit datatypes (int, float) and
>> upto ~4.5x improvement for 64-bit datatypes (long, double) as shown in the
>> performance data below.
>>
>>
>> **Arrays.sort performance data using JMH benchmarks for arrays with random
>> data**
>>
>> |Arrays.sort benchmark | Array Size | Baseline
>> (us/op)| AVX512 Sort (us/op) | Speedup |
>> |--- | --- | --- | --- | ---
>> |
>> |ArraysSort.doubleSort | 10 | 0.034 | 0.035
>> | 1.0 |
>> |ArraysSort.doubleSort | 25 | 0.116 | 0.089
>> | 1.3 |
>> |ArraysSort.doubleSort | 50 | 0.282 | 0.291
>> | 1.0 |
>> |ArraysSort.doubleSort | 75 | 0.474 | 0.358
>> | 1.3 |
>> |ArraysSort.doubleSort | 100 | 0.654 | 0.623
>> | 1.0 |
>> |ArraysSort.doubleSort | 1000| 9.274 | 6.331
>> | 1.5 |
>> |ArraysSort.doubleSort | 1 | 323.339 | 71.228
>> | **4.5** |
>> |ArraysSort.doubleSort | 10 | 4471.871|
>> 1002.748| **4.5** |
>> |ArraysSort.doubleSort | 100 | 51660.742 |
>> 12921.295 | **4.0** |
>> |ArraysSort.floatSort| 10 | 0.045 | 0.046
>> | 1.0 |
>> |ArraysSort.floatSort| 25 | 0.103 | 0.084
>> | 1.2 |
>> |ArraysSort.floatSort| 50 | 0.285 | 0.33
>> | 0.9 |
>> |ArraysSort.floatSort| 75 | 0.492 | 0.346
>> | 1.4 |
>> |ArraysSort.floatSort| 100 | 0.597 | 0.326
>> | 1.8 |
>> |ArraysSort.floatSort| 1000| 9.811 | 5.294
>> | 1.9 |
>> |ArraysSort.floatSort| 1 | 323.955 | 50.547
>> | **6.4** |
>> |ArraysSort.floatSort| 10 | 4326.38 | 731.152
>> | **5.9** |
>> |ArraysSort.floatSort| 100 | 52413.88|
>> 8409.193| **6.2** |
>> |ArraysSort.intSort | 10 | 0.033 | 0.033
>> | 1.0 |
>> |ArraysSort.intSort | 25 | 0.086 | 0.051
>> | 1.7 |
>> |ArraysSort.intSort | 50 | 0.236 | 0.151
>> | 1.6 |
>> |ArraysSort.intSort | 75 | 0.416 | 0.332
>> | 1.3 |
>> |ArraysSort.intSort | 100 | 0.63| 0.521
>> | 1.2 |
>> |ArraysSort.intSort | 1000| 10.518 | 4.698
>> | 2.2 |
>> |ArraysSort.intSort | 1 | 309.659 | 42.518
>> | **7.3** |
>> |ArraysSort.intSort | 10 | 4130.917|
>> 573.956 | **7.2** |
>> |ArraysSort.intSort | 100 | 49876.307 |
>> 6712.812| **7.4** |
>> |ArraysSort.longSort | 10 | 0.036 | 0.037
>> | 1.0 |
>> |ArraysSort.longSort | 25 | 0.094 | 0.08
>> | 1.2 |
>> |ArraysSort.longSort | 50 | 0.218 | 0.227
>> | 1.0 |
>> |ArraysSort.longSort | 75 | 0.466 | 0.402
>> | 1.2 |
>> |ArraysSort.longSort | 100 | 0.76| 0.58
>> | 1.3 |
>> |ArraysSort.longSort | 1000| 10.449 | 6
>
> Srinivas Vamsi Parasa has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Remove unnecessary import in Arrays.java
src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 4143:
> 4141: log_info(library)("Loaded library %s, handle " INTPTR_FORMAT,
> JNI_LIB_PREFIX "x86_64" JNI_LIB_SUFFIX, p2i(libx86_64));
> 4142:
> 4143: if (UseAVX > 2 && VM_Version::supports_avx512dq()) {
This check should be done before you locate and load library
src/hotspot/share/opto/library_call.cpp line 5218:
> 5216: BasicType bt = elem_type->basic_type();
> 5217: stubAddr = StubRoutines::select_array_partition_function(bt);
> 5218: if (stubAddr == nullptr) return false;
I see now how you check for AVX512 support.
Y
Re: RFR: 8268829: Provide an optimized way to walk the stack with Class object only [v6]
> 8268829: Provide an optimized way to walk the stack with Class object only > > `StackWalker::walk` creates one `StackFrame` per frame and the current > implementation > allocates one `StackFrameInfo` and one `MemberName` objects per frame. Some > frameworks > like logging may only interest in the Class object but not the method name > nor the BCI, > for example, filters out its implementation classes to find the caller class. > It's > similar to `StackWalker::getCallerClass` but allows a predicate to filter out > the element. > > This PR proposes to add `StackWalker.Kind` enum to specify the information > that a stack walker > collects. If no method information is needed, a `StackWalker` of > `CLASS_INFO` can be used > instead and such stack walker will save the overhead (1) to extract the > method information > and (2) the memory used for the stack walking. In addition, this can also > fix > > - [8311500](https://bugs.openjdk.org/browse/JDK-8311500): > StackWalker.getCallerClass() throws UOE if invoked reflectively > > New factory methods to take a parameter to specify the kind of stack walker > to be created are defined. > This provides a simple way for existing code, for example logging frameworks, > to take advantage of > this enhancement with the least change as it can keep the existing function > for traversing > `StackFrame`s. > > For example: to find the first caller filtering a known list of > implementation class, > existing code can call `StackWalker::getInstance(CLASS_INFO, ...)` to create > a stack walker instance: > > > StackWalker walker = StackWalker.getInstance(Kind.CLASS_INFO, > Option.RETAIN_CLASS_REFERENCE); > Optional> callerClass = walker.walk(s -> > s.map(StackFrame::getDeclaringClass) > .filter(interestingClasses::contains) > .findFirst()); > > > If method information is accessed on the `StackFrame`s produced by this stack > walker such as > `StackFrame::getMethodName`, then `UnsupportedOperationException` will be > thrown. > > Javadoc & specdiff > > https://cr.openjdk.org/~mchung/api/java.base/java/lang/StackWalker.html > https://cr.openjdk.org/~mchung/jdk22/specdiff/overview-summary.html > > Alternatives Considered > One alternative is to provide a new API: > ` T walkClass(Function, ? extends T> function)` > > In this case, the caller would need to pass a function that takes a stream > of `Class` object instead of `StackFrame`. Existing code would have to > modify calls to the `walk` method to `walkClass` and the function body. > > Another alte... Mandy Chung has updated the pull request incrementally with two additional commits since the last revision: - fixup javadoc - Review feedback: move JLIA to ClassFrameInfo - Changes: - all: https://git.openjdk.org/jdk/pull/15370/files - new: https://git.openjdk.org/jdk/pull/15370/files/a3a44575..4c5c5d79 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15370&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15370&range=04-05 Stats: 19 lines in 4 files changed: 3 ins; 10 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/15370.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15370/head:pull/15370 PR: https://git.openjdk.org/jdk/pull/15370
Re: RFR: 8309130: x86_64 AVX512 intrinsics for Arrays.sort methods (int, long, float and double arrays) [v29]
On Fri, 25 Aug 2023 18:46:53 GMT, Vladimir Kozlov wrote: >> Srinivas Vamsi Parasa has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Remove unnecessary import in Arrays.java > > After I fixed it Tier1 passed and I submitted other tiers. @vnkozlov The _mm512_set1_* are all C/C++ intrinsics for Intel instructions documented at https://www.intel.com/content/www/us/en/docs/intrinsics-guide/index.html. Both GCC and Microsoft C implements them. https://learn.microsoft.com/en-us/cpp/intrinsics/x64-amd64-intrinsics-list. - PR Comment: https://git.openjdk.org/jdk/pull/14227#issuecomment-1694025189
Withdrawn: 8311175: Move BufWriter::asByteBuffer to BufWriterImpl
On Fri, 30 Jun 2023 14:43:36 GMT, Chen Liang wrote: > As discussed on the mailing list > https://mail.openjdk.org/pipermail/classfile-api-dev/2023-June/000381.html, > BufWriter::asByteBuffer has a behavior not suitable for API and is only used > by internal StackMapGenerator/StackCounter, so it will be converted to an > internal API. > > Somehow the ByteBuffer needs to be sliced, or StackMapGenerator encounters > IOOBE. Not sure what the exact cause was. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/14736
Re: RFR: 8310929: Optimization for Integer.toString [v13]
On Tue, 18 Jul 2023 01:49:17 GMT, 温绍锦 wrote: >> Optimization for: >> Integer.toString >> Long.toString >> StringBuilder#append(int) >> >> # Benchmark Result >> >> >> sh make/devkit/createJMHBundle.sh >> bash configure --with-jmh=build/jmh/jars >> make test TEST="micro:java.lang.Integers.toString*" >> make test TEST="micro:java.lang.Longs.toString*" >> make test TEST="micro:java.lang.StringBuilders.toStringCharWithInt8" >> >> >> ## 1. >> [aliyun_ecs_c8i.xlarge](https://help.aliyun.com/document_detail/25378.html#c8i) >> * cpu : intel xeon sapphire rapids (x64) >> >> ``` diff >> -Benchmark (size) Mode Cnt Score Error Units (baseline) >> -Integers.toStringBig 500 avgt 15 6.825 ± 0.023 us/op >> -Integers.toStringSmall 500 avgt 15 4.823 ± 0.023 us/op >> -Integers.toStringTiny 500 avgt 15 3.878 ± 0.101 us/op >> >> +Benchmark (size) Mode Cnt Score Error Units (PR Update >> 04 f4aa1989) >> +Integers.toStringBig 500 avgt 15 6.002 ± 0.054 us/op (+13.71%) >> +Integers.toStringSmall 500 avgt 15 4.025 ± 0.020 us/op (+19.82%) >> +Integers.toStringTiny 500 avgt 15 3.874 ± 0.067 us/op (+0.10%) >> >> -Benchmark(size) Mode Cnt Score Error Units (baseline) >> -Longs.toStringBig 500 avgt 15 9.224 ± 0.021 us/op >> -Longs.toStringSmall 500 avgt 15 4.621 ± 0.087 us/op >> >> +Benchmark(size) Mode Cnt Score Error Units (PR Update 04 >> f4aa1989) >> +Longs.toStringBig 500 avgt 15 7.483 ± 0.018 us/op (+23.26%) >> +Longs.toStringSmall 500 avgt 15 4.020 ± 0.016 us/op (+14.95%) >> >> -Benchmark Mode Cnt ScoreError Units >> (baseline) >> -StringBuilders.toStringCharWithInt8 avgt 1589.327 ± 0.733 ns/op >> >> +BenchmarkMode Cnt Score Error Units (PR >> Update 04 f4aa1989) >> +StringBuilders.toStringCharWithInt8 avgt 15 36.639 ± 0.422 ns/op >> (+143.80%) >> >> >> >> ## 2. >> [aliyun_ecs_c8a.xlarge](https://help.aliyun.com/document_detail/25378.html#c8a) >> * cpu : amd epc genoa (x64) >> >> ``` diff >> -Benchmark (size) Mode Cnt Score Error Units (baseline) >> -Integers.toStringBig 500 avgt 15 6.753 ± 0.007 us/op >> -Integers.toStringSmall 500 avgt 15 4.470 ± 0.005 us/op >> -Integers.toStringTiny 500 avgt 15 2.764 ± 0.020 us/op >> >> +Benchmark (size) Mode Cnt Score Error Units (PR Update >> 04 f4aa1989) >> +Integers.toStringBig 500 avgt 15 5.036 ± 0.005 us/op (+... > > 温绍锦 has updated the pull request incrementally with one additional commit > since the last revision: > > assert bounds check @AlanBateman can you help me to review this PR? - PR Comment: https://git.openjdk.org/jdk/pull/14699#issuecomment-1694137748
