Re: [jdk17] RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K [v2]
> More refactoring to limit the scope of `@SuppressWarnings` annotations. > > Sometimes I introduce new methods. Please feel free to suggest method names > you like to use. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: one more - Changes: - all: https://git.openjdk.java.net/jdk17/pull/152/files - new: https://git.openjdk.java.net/jdk17/pull/152/files/d8b384df..774eb9da Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk17=152=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk17=152=00-01 Stats: 2 lines in 1 file changed: 1 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jdk17/pull/152.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/152/head:pull/152 PR: https://git.openjdk.java.net/jdk17/pull/152
Re: [jdk17] RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K
On Fri, 25 Jun 2021 20:04:37 GMT, Weijun Wang wrote: > More refactoring to limit the scope of `@SuppressWarnings` annotations. > > Sometimes I introduce new methods. Please feel free to suggest method names > you like to use. LGTM. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk17/pull/152
[jdk17] Integrated: 8269302: serviceability/dcmd/framework/InvalidCommandTest.java still fails after JDK-8268433
On Fri, 25 Jun 2021 18:11:27 GMT, Alex Menkov wrote: > Please review this simple test fix for jdk17. > > The cycle should run until connection is established > (connection.isConnected() returns true) or error occurred (error != null) > This wrong condition causes test error if ListenerThread.getConnection() > reaches "synchronized (this)" section earlier than ListenerThread.run() This pull request has now been integrated. Changeset: 1404e4bf Author:Alex Menkov URL: https://git.openjdk.java.net/jdk17/commit/1404e4bf44e28cadda3949f9e398e664cb98a5e2 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8269302: serviceability/dcmd/framework/InvalidCommandTest.java still fails after JDK-8268433 Reviewed-by: kevinw, dcubed - PR: https://git.openjdk.java.net/jdk17/pull/150
Re: [jdk17] RFR: 8269302: serviceability/dcmd/framework/InvalidCommandTest.java still fails after JDK-8268433
On Fri, 25 Jun 2021 20:26:16 GMT, Daniel D. Daugherty wrote: > Thumbs up. > > This looks like a trivial fix to me. I don't see any mention of > testing. It looks like the two tests mentioned in the bug both > run in Tier1 on multiple platforms. Run 3 tests in test/hotspot/jtreg/serviceability/dcmd/framework 200 times (win and win-debug). No failures. - PR: https://git.openjdk.java.net/jdk17/pull/150
Re: RFR: 8269268: JDWP: Properly fix thread lookup assert in findThread() [v2]
On Thu, 24 Jun 2021 16:51:59 GMT, Chris Plummer wrote: >> Re-enable the assert that was disabled (with some overhead) by >> [JDK-8265683](https://bugs.openjdk.java.net/browse/JDK-8265683). Explanation >> is in the CR and also in comments included with the changes. >> >> I tested by running `vmTestbase/nsk/jdb/suspend/suspend001/suspend001.java` >> and `vmTestbase/nsk/jdb/wherei/wherei001/wherei001.java` 100's of times, and >> did not see any failures. I also verified the original issue was still >> reproducible by temporarily not setting `gdata->handlingVMDeath = JNI_TRUE`, >> which did trigger the assert as expected. > > Chris Plummer has updated the pull request incrementally with one additional > commit since the last revision: > > Rename flag and make some minor adjustments. Marked as reviewed by amenkov (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4580
Re: [jdk17] RFR: 8269302: serviceability/dcmd/framework/InvalidCommandTest.java still fails after JDK-8268433
On Fri, 25 Jun 2021 18:11:27 GMT, Alex Menkov wrote: > Please review this simple test fix for jdk17. > > The cycle should run until connection is established > (connection.isConnected() returns true) or error occurred (error != null) > This wrong condition causes test error if ListenerThread.getConnection() > reaches "synchronized (this)" section earlier than ListenerThread.run() Thumbs up. This looks like a trivial fix to me. I don't see any mention of testing. It looks like the two tests mentioned in the bug both run in Tier1 on multiple platforms. - Marked as reviewed by dcubed (Reviewer). PR: https://git.openjdk.java.net/jdk17/pull/150
Re: [jdk17] RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K
On Fri, 25 Jun 2021 20:04:37 GMT, Weijun Wang wrote: > More refactoring to limit the scope of `@SuppressWarnings` annotations. > > Sometimes I introduce new methods. Please feel free to suggest method names > you like to use. Changes look good Max - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk17/pull/152
Re: [jdk17] RFR: 8269302: serviceability/dcmd/framework/InvalidCommandTest.java still fails after JDK-8268433
On Fri, 25 Jun 2021 18:11:27 GMT, Alex Menkov wrote: > Please review this simple test fix for jdk17. > > The cycle should run until connection is established > (connection.isConnected() returns true) or error occurred (error != null) > This wrong condition causes test error if ListenerThread.getConnection() > reaches "synchronized (this)" section earlier than ListenerThread.run() Marked as reviewed by kevinw (Committer). - PR: https://git.openjdk.java.net/jdk17/pull/150
[jdk17] RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K
More refactoring to limit the scope of `@SuppressWarnings` annotations. Sometimes I introduce new methods. Please feel free to suggest method names you like to use. - Commit messages: - 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K Changes: https://git.openjdk.java.net/jdk17/pull/152/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk17=152=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8269409 Stats: 289 lines in 21 files changed: 161 ins; 64 del; 64 mod Patch: https://git.openjdk.java.net/jdk17/pull/152.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/152/head:pull/152 PR: https://git.openjdk.java.net/jdk17/pull/152
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v6]
On 6/21/2021 2:02 PM, Paul Sandoz wrote: On Mon, 21 Jun 2021 05:17:09 GMT, Yi Yang wrote: After JDK-8265518(#3615), it's possible to replace all variants of checkIndex by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in the whole JDK codebase. Yi Yang has updated the pull request incrementally with one additional commit since the last revision: more replacement 2 All the updates to the check* methods look ok (requires some careful looking!). I cannot recall what others said about the change in exception messages. @jddarcy any advice here? Generally, the JDK does not have the text of exception message as a supported interface meant to be relied on by users. This doesn't stop developers from occasionally parsing those messages, but that is usually a sign of a missing API which we try to rectify more directly. HTH, -Joe
[jdk17] RFR: 8269302: serviceability/dcmd/framework/InvalidCommandTest.java still fails after JDK-8268433
Please review this simple test fix for jdk17. The cycle should run until connection is established (connection.isConnected() returns true) or error occurred (error != null) This wrong condition causes test error if ListenerThread.getConnection() reaches "synchronized (this)" section earlier than ListenerThread.run() - Commit messages: - Fixed cycle condition in ListenerThread.getConnection Changes: https://git.openjdk.java.net/jdk17/pull/150/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk17=150=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8269302 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk17/pull/150.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/150/head:pull/150 PR: https://git.openjdk.java.net/jdk17/pull/150
Withdrawn: 8269302: serviceability/dcmd/framework/InvalidCommandTest.java still fails after JDK-8268433
On Fri, 25 Jun 2021 04:25:08 GMT, Alex Menkov wrote: > Please review this trivial fix in the cycle condition. > > The cycle should run until connection is established > (connection.isConnected() returns true) or error occurred (error != null) > This wrong condition causes test error if ListenerThread.getConnection() > reaches "synchronized (this)" section earlier than ListenerThread.run() This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/4593
Re: RFR: 8269302: serviceability/dcmd/framework/InvalidCommandTest.java still fails after JDK-8268433
On Fri, 25 Jun 2021 15:32:09 GMT, Daniel D. Daugherty wrote: > > > @alexmenkov - would you mind withdrawing this PR and creating a new PR > against JDK17? > We're seeing these test failures in the JDK17 CI also. I've bumped the > priority of the bug > from P4 -> P3 since we're seeing Tier3 test failures. Makes sense. Will do - PR: https://git.openjdk.java.net/jdk/pull/4593
Re: RFR: JDK-8268893: jcmd to trim the glibc heap [v3]
On Fri, 25 Jun 2021 06:22:37 GMT, Thomas Stuefe wrote: >> Proposal to add a Linux+glibc-only jcmd to manually induce malloc_trim(3) in >> the VM process. >> >> >> The glibc is somewhat notorious for retaining released C Heap memory: >> calling free(3) returns memory to the glibc, and most libc variants will >> return at least a portion of it back to the Operating System, but the glibc >> often does not. >> >> This depends on the granularity of the allocations and a number of other >> factors, but we found that many small allocations in particular may cause >> the process heap segment (hence RSS) to get bloaty. This can cause the VM to >> not recover from C-heap usage spikes. >> >> The glibc offers an API, "malloc_trim", which can be used to cause the glibc >> to return free'd memory back to the Operating System. >> >> This may cost performance, however, and therefore I hesitate to call >> malloc_trim automatically. That may be an idea for another day. >> >> Instead of an automatic trim I propose to add a jcmd which allows to >> manually trigger a libc heap trim. Such a command would have two purposes: >> - when analyzing cases of high memory footprint, it allows to distinguish >> "real" footprint, e.g. leaks, from a cases where the glibc just holds on to >> memory >> - as a stop gap measure it allows to release pressure from a high footprint >> scenario. >> >> Note that this command also helps with analyzing libc peaks which had >> nothing to do with the VM - e.g. peaks created by customer code which just >> happens to share the same process as the VM. Such memory does not even have >> to show up in NMT. >> >> I propose to introduce this command for Linux only. Other OSes (apart maybe >> AIX) do not seem to have this problem, but Linux is arguably important >> enough in itself to justify a Linux specific jcmd. >> >> CSR for this command: https://bugs.openjdk.java.net/browse/JDK-8269345 >> >> Note that an alternative to a Linux-only jcmd would be a command which would >> trim the C-heap on all platforms, with implementations to be filled out >> later. >> >> = >> >> This patch: >> >> - introduces a new jcmd, "VM.trim_libc_heap", no arguments, which trims the >> glibc heap on glibc platforms. >> - includes a (rather basic) test >> - the command calls malloc_trim(3), and additionally prints out its effect >> (changes caused in virt size, rss and swap space) >> - I refactored some code in os_linux.cpp to factor out scanning >> /proc/self/status to get kernel memory information. >> >> = >> >> Example: >> >> A programm causes a temporary peak in C-heap usage (in this case, triggered >> via Unsafe.allocateMemory), right away frees the memory again, so its not >> leaky. The peak in RSS was ~8G (even though the user allocation was way >> smaller - glibc has a lot of overhead). The effects of this peak linger even >> after returning that memory to the glibc: >> >> >> >> thomas@starfish:~$ jjjcmd AllocCHeap VM.info | grep Resident >> Resident Set Size: 8685896K (peak: 8685896K) (anon: 8648680K, file: 37216K, >> shmem: 0K) >> >> >> >> We execute the new trim command via jcmd: >> >> >> thomas@starfish:~$ jjjcmd AllocCHeap VM.trim_libc_heap >> 18770: >> Attempting trim... >> Done. >> Virtual size before: 28849744k, after: 28849724k, (-20k) >> RSS before: 8685896k, after: 920740k, (-7765156k) >> Swap before: 0k, after: 0k, (0k) >> >> >> It prints out reduction in virtual size, rss and swap. The virtual size did >> not decrease since no mappings had been unmapped by the glibc. However, the >> process heap was shrunk heavily by the glibc, resulting in a large drop in >> RSS (8.5G->900M), freeing >7G of memory: >> >> >> thomas@starfish:~$ jjjcmd AllocCHeap VM.info | grep Resident >> Resident Set Size: 920740K (peak: 8686004K) (anon: 883460K, file: 37280K, >> shmem: 0K) >>^^^ >> >> >> When the VM is started with -Xlog:os, this is also logged: >> >> >> [139,068s][info][os] malloc_trim: >> [139,068s][info][os] Virtual size before: 28849744k, after: 28849724k, (-20k) >> RSS before: 8685896k, after: 920740k, (-7765156k) >> Swap before: 0k, after: 0k, (0k) > > Thomas Stuefe 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 four additional > commits since the last revision: > > - Volker feedback > - Merge > - Feedback Severin; renamed query function > - start Thanks for the cleanups. Looks good to me now. - Marked as reviewed by simonis (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4510
Re: RFR: 8269302: serviceability/dcmd/framework/InvalidCommandTest.java still fails after JDK-8268433
On Fri, 25 Jun 2021 04:25:08 GMT, Alex Menkov wrote: > Please review this trivial fix in the cycle condition. > > The cycle should run until connection is established > (connection.isConnected() returns true) or error occurred (error != null) > This wrong condition causes test error if ListenerThread.getConnection() > reaches "synchronized (this)" section earlier than ListenerThread.run() @alexmenkov - would you mind withdrawing this PR and creating a new PR against JDK17? We're seeing these test failures in the JDK17 CI also. I've bumped the priority of the bug from P4 -> P3 since we're seeing Tier3 test failures. - PR: https://git.openjdk.java.net/jdk/pull/4593
Re: RFR: 8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl implementation [v5]
On Fri, 25 Jun 2021 11:48:52 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my changes for the removal of the legacy >> `PlainSocketImpl` and `PlainDatagramSocketImpl` implementations? >> >> In JDK 13, JEP 353 provided a drop in replacement for the legacy >> `PlainSocketImpl` implementation. Since JDK 13, the `PlainSocketImpl` >> implementation was no longer used but included a mitigation mechanism to >> reduce compatibility risks in the form of a JDK-specific property >> `jdk.net.usePlainSocketImpl` allowing to switch back to the old >> implementation. >> Similarly, in JDK 15, JEP 373 provided a new implementation for >> `DatagramSocket` and `MulticastSocket`, with a JDK-specific property >> `jdk.net.usePlainDatagramSocketImpl` also allowing the user to switch back >> to the old implementation in case of compatibility issue. >> >> As these implementations (and the mechanisms they use to enable them to >> mitigate compatibility issues) have been deemed no longer necessary, they >> now represent a maintenance burden. This patch looks at removing them from >> the JDK. >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > 8253119: Fixed typo Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4574
Re: RFR: 8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl implementation [v5]
On Fri, 25 Jun 2021 11:48:52 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my changes for the removal of the legacy >> `PlainSocketImpl` and `PlainDatagramSocketImpl` implementations? >> >> In JDK 13, JEP 353 provided a drop in replacement for the legacy >> `PlainSocketImpl` implementation. Since JDK 13, the `PlainSocketImpl` >> implementation was no longer used but included a mitigation mechanism to >> reduce compatibility risks in the form of a JDK-specific property >> `jdk.net.usePlainSocketImpl` allowing to switch back to the old >> implementation. >> Similarly, in JDK 15, JEP 373 provided a new implementation for >> `DatagramSocket` and `MulticastSocket`, with a JDK-specific property >> `jdk.net.usePlainDatagramSocketImpl` also allowing the user to switch back >> to the old implementation in case of compatibility issue. >> >> As these implementations (and the mechanisms they use to enable them to >> mitigate compatibility issues) have been deemed no longer necessary, they >> now represent a maintenance burden. This patch looks at removing them from >> the JDK. >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > 8253119: Fixed typo Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4574
Re: RFR: 8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl implementation [v4]
On Fri, 25 Jun 2021 11:37:50 GMT, Daniel Fuchs wrote: >> src/java.base/share/classes/java/net/DatagramSocket.java line 1398: >> >>> 1396: DatagramSocketImpl impl = >>> factory.createDatagramSocketImpl(); >>> 1397: Objects.requireNonNull(impl, >>> 1398: "Implementation returned by installed >>> DatagramSocketFactoryImpl is null"); >> >> I think you mean "DatagramSocketImplFactory" instead of >> "DatagramSocketFactoryImpl" here. > > Yes - good catch! I've been bitten by this before too :-) > https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/net/DatagramSocketImplFactory.html Well spotted. Thanks Alan. Change made in commit 686b436 - PR: https://git.openjdk.java.net/jdk/pull/4574
Re: RFR: 8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl implementation [v5]
> Hi, > > Could someone please review my changes for the removal of the legacy > `PlainSocketImpl` and `PlainDatagramSocketImpl` implementations? > > In JDK 13, JEP 353 provided a drop in replacement for the legacy > `PlainSocketImpl` implementation. Since JDK 13, the `PlainSocketImpl` > implementation was no longer used but included a mitigation mechanism to > reduce compatibility risks in the form of a JDK-specific property > `jdk.net.usePlainSocketImpl` allowing to switch back to the old > implementation. > Similarly, in JDK 15, JEP 373 provided a new implementation for > `DatagramSocket` and `MulticastSocket`, with a JDK-specific property > `jdk.net.usePlainDatagramSocketImpl` also allowing the user to switch back to > the old implementation in case of compatibility issue. > > As these implementations (and the mechanisms they use to enable them to > mitigate compatibility issues) have been deemed no longer necessary, they now > represent a maintenance burden. This patch looks at removing them from the > JDK. > > Kind regards, > Patrick Patrick Concannon has updated the pull request incrementally with one additional commit since the last revision: 8253119: Fixed typo - Changes: - all: https://git.openjdk.java.net/jdk/pull/4574/files - new: https://git.openjdk.java.net/jdk/pull/4574/files/36bcee31..686b4369 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4574=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4574=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4574.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4574/head:pull/4574 PR: https://git.openjdk.java.net/jdk/pull/4574
Re: RFR: 8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl implementation [v4]
On Fri, 25 Jun 2021 10:56:15 GMT, Alan Bateman wrote: >> Patrick Concannon 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 five additional >> commits since the last revision: >> >> - 8253119: Added message to null check in java/net/DatagramSocket >> - Merge remote-tracking branch 'origin/master' into JDK-8253119 >> - 8253119: Removed redundant comments and USE_PLAINDATAGRAMSOCKET from >> java/net/DatagramSocket >> - Merge remote-tracking branch 'origin/master' into JDK-8253119 >> - 8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl >> implementation > > src/java.base/share/classes/java/net/DatagramSocket.java line 1398: > >> 1396: DatagramSocketImpl impl = >> factory.createDatagramSocketImpl(); >> 1397: Objects.requireNonNull(impl, >> 1398: "Implementation returned by installed >> DatagramSocketFactoryImpl is null"); > > I think you mean "DatagramSocketImplFactory" instead of > "DatagramSocketFactoryImpl" here. Yes - good catch! I've been bitten by this before too :-) https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/net/DatagramSocketImplFactory.html - PR: https://git.openjdk.java.net/jdk/pull/4574
Re: RFR: 8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl implementation [v4]
On Fri, 25 Jun 2021 10:25:52 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my changes for the removal of the legacy >> `PlainSocketImpl` and `PlainDatagramSocketImpl` implementations? >> >> In JDK 13, JEP 353 provided a drop in replacement for the legacy >> `PlainSocketImpl` implementation. Since JDK 13, the `PlainSocketImpl` >> implementation was no longer used but included a mitigation mechanism to >> reduce compatibility risks in the form of a JDK-specific property >> `jdk.net.usePlainSocketImpl` allowing to switch back to the old >> implementation. >> Similarly, in JDK 15, JEP 373 provided a new implementation for >> `DatagramSocket` and `MulticastSocket`, with a JDK-specific property >> `jdk.net.usePlainDatagramSocketImpl` also allowing the user to switch back >> to the old implementation in case of compatibility issue. >> >> As these implementations (and the mechanisms they use to enable them to >> mitigate compatibility issues) have been deemed no longer necessary, they >> now represent a maintenance burden. This patch looks at removing them from >> the JDK. >> >> Kind regards, >> Patrick > > Patrick Concannon 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 five additional > commits since the last revision: > > - 8253119: Added message to null check in java/net/DatagramSocket > - Merge remote-tracking branch 'origin/master' into JDK-8253119 > - 8253119: Removed redundant comments and USE_PLAINDATAGRAMSOCKET from > java/net/DatagramSocket > - Merge remote-tracking branch 'origin/master' into JDK-8253119 > - 8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl > implementation src/java.base/share/classes/java/net/DatagramSocket.java line 1398: > 1396: DatagramSocketImpl impl = > factory.createDatagramSocketImpl(); > 1397: Objects.requireNonNull(impl, > 1398: "Implementation returned by installed > DatagramSocketFactoryImpl is null"); I think you mean "DatagramSocketFactoryImpl" rather than "DatagramSocketImplFactory" here. - PR: https://git.openjdk.java.net/jdk/pull/4574
Re: RFR: 8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl implementation [v4]
On Fri, 25 Jun 2021 10:25:52 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my changes for the removal of the legacy >> `PlainSocketImpl` and `PlainDatagramSocketImpl` implementations? >> >> In JDK 13, JEP 353 provided a drop in replacement for the legacy >> `PlainSocketImpl` implementation. Since JDK 13, the `PlainSocketImpl` >> implementation was no longer used but included a mitigation mechanism to >> reduce compatibility risks in the form of a JDK-specific property >> `jdk.net.usePlainSocketImpl` allowing to switch back to the old >> implementation. >> Similarly, in JDK 15, JEP 373 provided a new implementation for >> `DatagramSocket` and `MulticastSocket`, with a JDK-specific property >> `jdk.net.usePlainDatagramSocketImpl` also allowing the user to switch back >> to the old implementation in case of compatibility issue. >> >> As these implementations (and the mechanisms they use to enable them to >> mitigate compatibility issues) have been deemed no longer necessary, they >> now represent a maintenance burden. This patch looks at removing them from >> the JDK. >> >> Kind regards, >> Patrick > > Patrick Concannon 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 five additional > commits since the last revision: > > - 8253119: Added message to null check in java/net/DatagramSocket > - Merge remote-tracking branch 'origin/master' into JDK-8253119 > - 8253119: Removed redundant comments and USE_PLAINDATAGRAMSOCKET from > java/net/DatagramSocket > - Merge remote-tracking branch 'origin/master' into JDK-8253119 > - 8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl > implementation Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4574
Re: RFR: 8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl implementation [v4]
On Thu, 24 Jun 2021 15:06:49 GMT, Daniel Fuchs wrote: >>> I've created an issue to track this: >>> https://bugs.openjdk.java.net/browse/JDK-8269288 >> >> Thanks. So are you keeping the Objects.requireNonNull here? If so then it >> should probably be the 2-arg version so that the message is clear that the >> custom DatagramSocketFactory returned null, otherwise it will look like a DS >> bug. > > @AlanBateman Ah - good idea - something like "implementation returned by > installed DatagramSocketFactoryImpl is null"? > Maybe we could add the name of the concrete DatagramSocketFactoryImpl class > too? OK. I've added a message to the null check now. See 36bcee3 - PR: https://git.openjdk.java.net/jdk/pull/4574
Re: RFR: 8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl implementation [v4]
> Hi, > > Could someone please review my changes for the removal of the legacy > `PlainSocketImpl` and `PlainDatagramSocketImpl` implementations? > > In JDK 13, JEP 353 provided a drop in replacement for the legacy > `PlainSocketImpl` implementation. Since JDK 13, the `PlainSocketImpl` > implementation was no longer used but included a mitigation mechanism to > reduce compatibility risks in the form of a JDK-specific property > `jdk.net.usePlainSocketImpl` allowing to switch back to the old > implementation. > Similarly, in JDK 15, JEP 373 provided a new implementation for > `DatagramSocket` and `MulticastSocket`, with a JDK-specific property > `jdk.net.usePlainDatagramSocketImpl` also allowing the user to switch back to > the old implementation in case of compatibility issue. > > As these implementations (and the mechanisms they use to enable them to > mitigate compatibility issues) have been deemed no longer necessary, they now > represent a maintenance burden. This patch looks at removing them from the > JDK. > > Kind regards, > Patrick Patrick Concannon 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 five additional commits since the last revision: - 8253119: Added message to null check in java/net/DatagramSocket - Merge remote-tracking branch 'origin/master' into JDK-8253119 - 8253119: Removed redundant comments and USE_PLAINDATAGRAMSOCKET from java/net/DatagramSocket - Merge remote-tracking branch 'origin/master' into JDK-8253119 - 8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl implementation - Changes: - all: https://git.openjdk.java.net/jdk/pull/4574/files - new: https://git.openjdk.java.net/jdk/pull/4574/files/49125e7e..36bcee31 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4574=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4574=02-03 Stats: 7050 lines in 209 files changed: 1681 ins; 5063 del; 306 mod Patch: https://git.openjdk.java.net/jdk/pull/4574.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4574/head:pull/4574 PR: https://git.openjdk.java.net/jdk/pull/4574
Re: RFR: 8269268: JDWP: Properly fix thread lookup assert in findThread() [v2]
On Thu, 24 Jun 2021 16:51:59 GMT, Chris Plummer wrote: >> Re-enable the assert that was disabled (with some overhead) by >> [JDK-8265683](https://bugs.openjdk.java.net/browse/JDK-8265683). Explanation >> is in the CR and also in comments included with the changes. >> >> I tested by running `vmTestbase/nsk/jdb/suspend/suspend001/suspend001.java` >> and `vmTestbase/nsk/jdb/wherei/wherei001/wherei001.java` 100's of times, and >> did not see any failures. I also verified the original issue was still >> reproducible by temporarily not setting `gdata->handlingVMDeath = JNI_TRUE`, >> which did trigger the assert as expected. > > Chris Plummer has updated the pull request incrementally with one additional > commit since the last revision: > > Rename flag and make some minor adjustments. Marked as reviewed by kevinw (Committer). - PR: https://git.openjdk.java.net/jdk/pull/4580
Re: RFR: 8269302: serviceability/dcmd/framework/InvalidCommandTest.java still fails after JDK-8268433
On Fri, 25 Jun 2021 04:25:08 GMT, Alex Menkov wrote: > Please review this trivial fix in the cycle condition. > > The cycle should run until connection is established > (connection.isConnected() returns true) or error occurred (error != null) > This wrong condition causes test error if ListenerThread.getConnection() > reaches "synchronized (this)" section earlier than ListenerThread.run() Marked as reviewed by kevinw (Committer). - PR: https://git.openjdk.java.net/jdk/pull/4593
Re: RFR: 8252842: Extend jmap to support parallel heap dump [v27]
On Mon, 7 Jun 2021 07:58:26 GMT, Lin Zang wrote: >> 8252842: Extend jmap to support parallel heap dump > > Lin Zang has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 36 commits: > > - Merge branch 'master' into par-dump > - Merge branch 'master' into par-dump > - fix build issue after rebase and fix one issue > - Merge branch 'master' > - Merge branch 'master' into pd > - update copyright info > - Merge branch 'master' into par-dump > - undo JMap.java > - code refine and typo fix > - Merge branch 'master' into par-dump > - ... and 26 more: > https://git.openjdk.java.net/jdk/compare/908aca29...78c2c763 Dear All， Sorry for not updating this PR for quite a while. I will try to add detailed explaination of the implementation here to make it easier to be reviewed. - PR: https://git.openjdk.java.net/jdk/pull/2261
Re: RFR: JDK-8268893: jcmd to trim the glibc heap [v3]
> Proposal to add a Linux+glibc-only jcmd to manually induce malloc_trim(3) in > the VM process. > > > The glibc is somewhat notorious for retaining released C Heap memory: calling > free(3) returns memory to the glibc, and most libc variants will return at > least a portion of it back to the Operating System, but the glibc often does > not. > > This depends on the granularity of the allocations and a number of other > factors, but we found that many small allocations in particular may cause the > process heap segment (hence RSS) to get bloaty. This can cause the VM to not > recover from C-heap usage spikes. > > The glibc offers an API, "malloc_trim", which can be used to cause the glibc > to return free'd memory back to the Operating System. > > This may cost performance, however, and therefore I hesitate to call > malloc_trim automatically. That may be an idea for another day. > > Instead of an automatic trim I propose to add a jcmd which allows to manually > trigger a libc heap trim. Such a command would have two purposes: > - when analyzing cases of high memory footprint, it allows to distinguish > "real" footprint, e.g. leaks, from a cases where the glibc just holds on to > memory > - as a stop gap measure it allows to release pressure from a high footprint > scenario. > > Note that this command also helps with analyzing libc peaks which had nothing > to do with the VM - e.g. peaks created by customer code which just happens to > share the same process as the VM. Such memory does not even have to show up > in NMT. > > I propose to introduce this command for Linux only. Other OSes (apart maybe > AIX) do not seem to have this problem, but Linux is arguably important enough > in itself to justify a Linux specific jcmd. > > If this finds agreement, I will file a CSR. > > Note that an alternative to a Linux-only jcmd would be a command which would > trim the C-heap on all platforms, with implementations to be filled out later. > > = > > This patch: > > - introduces a new jcmd, "VM.trim_libc_heap", no arguments, which trims the > glibc heap on glibc platforms. > - includes a (rather basic) test > - the command calls malloc_trim(3), and additionally prints out its effect > (changes caused in virt size, rss and swap space) > - I refactored some code in os_linux.cpp to factor out scanning > /proc/self/status to get kernel memory information. > > = > > Example: > > A programm causes a temporary peak in C-heap usage (in this case, triggered > via Unsafe.allocateMemory), right away frees the memory again, so its not > leaky. The peak in RSS was ~8G (even though the user allocation was way > smaller - glibc has a lot of overhead). The effects of this peak linger even > after returning that memory to the glibc: > > > > thomas@starfish:~$ jjjcmd AllocCHeap VM.info | grep Resident > Resident Set Size: 8685896K (peak: 8685896K) (anon: 8648680K, file: 37216K, > shmem: 0K) > > > > We execute the new trim command via jcmd: > > > thomas@starfish:~$ jjjcmd AllocCHeap VM.trim_libc_heap > 18770: > Attempting trim... > Done. > Virtual size before: 28849744k, after: 28849724k, (-20k) > RSS before: 8685896k, after: 920740k, (-7765156k) > Swap before: 0k, after: 0k, (0k) > > > It prints out reduction in virtual size, rss and swap. The virtual size did > not decrease since no mappings had been unmapped by the glibc. However, the > process heap was shrunk heavily by the glibc, resulting in a large drop in > RSS (8.5G->900M), freeing >7G of memory: > > > thomas@starfish:~$ jjjcmd AllocCHeap VM.info | grep Resident > Resident Set Size: 920740K (peak: 8686004K) (anon: 883460K, file: 37280K, > shmem: 0K) >^^^ > > > When the VM is started with -Xlog:os, this is also logged: > > > [139,068s][info][os] malloc_trim: > [139,068s][info][os] Virtual size before: 28849744k, after: 28849724k, (-20k) > RSS before: 8685896k, after: 920740k, (-7765156k) > Swap before: 0k, after: 0k, (0k) Thomas Stuefe 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 four additional commits since the last revision: - Volker feedback - Merge - Feedback Severin; renamed query function - start - Changes: - all: https://git.openjdk.java.net/jdk/pull/4510/files - new: https://git.openjdk.java.net/jdk/pull/4510/files/a48791e4..29807b26 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4510=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4510=01-02 Stats: 28324 lines in 497 files changed: 16225 ins; 10712 del; 1387 mod Patch: https://git.openjdk.java.net/jdk/pull/4510.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4510/head:pull/4510 PR: https://git.openjdk.java.net/jdk/pull/4510
Re: RFR: JDK-8268893: jcmd to trim the glibc heap [v2]
On Mon, 21 Jun 2021 08:25:29 GMT, Volker Simonis wrote:
>> Thomas Stuefe has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Feedback Severin; renamed query function
>
> src/hotspot/os/linux/os_linux.cpp line 2142:
>
>> 2140: bool os::Linux::query_process_memory_info(os::Linux::meminfo_t* info) {
>> 2141: FILE* f = ::fopen("/proc/self/status", "r");
>> 2142: const int num_values = 8;
>
> Refactoring of `os::Linux::print_process_memory_info` looks good. But now
> that you've already created `os::Linux::meminfo_t` anyway I'd suggest to make
> `num_values == sizeof(os::Linux::meminfo_t)/sizeof(ssize_t)`.
Okay, makes sense. I was a bit apprehensive since that relies on the internal
layout of this structure. But OTOH if that number is wrong, the worst that
happens is that we, each time, parse through the whole of /proc/self/status
instead of stopping early when all data are found. Which may happen anyway
since not all kernels support all of these data.
> src/hotspot/share/services/diagnosticCommand.cpp line 99:
>
>> 97: DCmdFactory::register_DCmdFactory(new
>> DCmdFactoryImpl(full_export, true, false));
>> 98: DCmdFactory::register_DCmdFactory(new
>> DCmdFactoryImpl(full_export, true, false));
>> 99: LINUX_ONLY(DCmdFactory::register_DCmdFactory(new
>> DCmdFactoryImpl(full_export, true, false));)
>
> Not sure if the commands are really sorted and if order is important here.
> Otherwise you could add the new command a few lines later where there already
> exists an `#ifdef LINUX` section:
>
>
> #ifdef LINUX
> DCmdFactory::register_DCmdFactory(new
> DCmdFactoryImpl(full_export, true, false));
> DCmdFactory::register_DCmdFactory(new
> DCmdFactoryImpl(full_export, true, false));
> #endif // LINUX
I don't think order matters, and grouping linux specific commands looks better.
-
PR: https://git.openjdk.java.net/jdk/pull/4510
