Re: RFR: 8277954: Replace use of monitors with explicit locks in the JDK LDAP provider implementation [v2]
On Fri, 8 Sep 2023 14:59:38 GMT, Daniel Fuchs wrote: > Look reasonable to me. If tier2 and all jndi tests are still passing, I'm > good with it. Thanks for the review Daniel. `tier1`, `tier2`, `tier3` and JNDI/LDAP JCK tests showed no failures with the change proposed here. - PR Comment: https://git.openjdk.org/jdk/pull/15526#issuecomment-1712323351
Re: RFR: 8277954: Replace use of monitors with explicit locks in the JDK LDAP provider implementation [v2]
> The change proposed in this PR improves the behavior of the JDK JNDI/LDAP > provider when running in a virtual thread. Currently, when an LDAP operation > is performed from a virtual thread context a pinned carrier thread is > detected: > > Thread[#29,ForkJoinPool-1-worker-1,5,CarrierThreads] > java.naming/com.sun.jndi.ldap.Connection.read > reply(Connection.java:444) <== monitors:1 > > java.naming/com.sun.jndi.ldap.LdapClient.ldapBind(LdapClient.java:369) <== > monitors:1 > > java.naming/com.sun.jndi.ldap.LdapClient.authenticate(LdapClient.java:196) > <== monitors:1 > java.naming/com.sun.jndi.ldap.LdapCtx.connect(LdapCtx.java:2896) <== > monitors:1 > > To fix that monitor usages are replaced with `j.u.c` locks. All synchronized > blocks/methods have been replaced with locks even if it is only guarding > memory access - the motivation behind such a decision was to avoid an > analysis of scenarios where a mix of monitors and `j.u.c` locks is used. > > There are three types of mechanical changes done in this PR: > > 1. Classes with `synchronized` blocks or `synchronized` methods have been > updated to include a new `ReentrantLock` field. These new fields are used to > replace `synchronized` blocks/methods. > 1. classes that use notify/wait on object monitor have been updated to use > `ReentrantLock.Condition`s signal/await. > 1. if one class `synchronized` on an instance of another class - the > `ReentrantLock` added in item (1) was made a package-protected to give access > to another class. > > With the proposed changes pinned carrier threads are no longer detected > during execution of LDAP operations. > > Testing: `jdk-tier1` to `jdk-tier3`, other `jndi/ldap` regression and JCK > naming tests show no failures. Aleksei Efimov 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 three additional commits since the last revision: - Merge branch 'master' into JDK-8277954_replace_synchronized_ldap - revert incorrect changes in LdapCtx.addNamingListener, formatting and (c) updates - draft version - Changes: - all: https://git.openjdk.org/jdk/pull/15526/files - new: https://git.openjdk.org/jdk/pull/15526/files/85bbe2a7..9794b094 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15526=01 - incr: https://webrevs.openjdk.org/?repo=jdk=15526=00-01 Stats: 31231 lines in 922 files changed: 18653 ins; 7610 del; 4968 mod Patch: https://git.openjdk.org/jdk/pull/15526.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15526/head:pull/15526 PR: https://git.openjdk.org/jdk/pull/15526
Re: RFR: 8277954: Replace use of monitors with explicit locks in the JDK LDAP provider implementation
On Thu, 31 Aug 2023 22:48:30 GMT, Aleksei Efimov wrote: > The change proposed in this PR improves the behavior of the JDK JNDI/LDAP > provider when running in a virtual thread. Currently, when an LDAP operation > is performed from a virtual thread context a pinned carrier thread is > detected: > > Thread[#29,ForkJoinPool-1-worker-1,5,CarrierThreads] > java.naming/com.sun.jndi.ldap.Connection.read > reply(Connection.java:444) <== monitors:1 > > java.naming/com.sun.jndi.ldap.LdapClient.ldapBind(LdapClient.java:369) <== > monitors:1 > > java.naming/com.sun.jndi.ldap.LdapClient.authenticate(LdapClient.java:196) > <== monitors:1 > java.naming/com.sun.jndi.ldap.LdapCtx.connect(LdapCtx.java:2896) <== > monitors:1 > > To fix that monitor usages are replaced with `j.u.c` locks. All synchronized > blocks/methods have been replaced with locks even if it is only guarding > memory access - the motivation behind such a decision was to avoid an > analysis of scenarios where a mix of monitors and `j.u.c` locks is used. > > There are three types of mechanical changes done in this PR: > > 1. Classes with `synchronized` blocks or `synchronized` methods have been > updated to include a new `ReentrantLock` field. These new fields are used to > replace `synchronized` blocks/methods. > 1. classes that use notify/wait on object monitor have been updated to use > `ReentrantLock.Condition`s signal/await. > 1. if one class `synchronized` on an instance of another class - the > `ReentrantLock` added in item (1) was made a package-protected to give access > to another class. > > With the proposed changes pinned carrier threads are no longer detected > during execution of LDAP operations. > > Testing: `jdk-tier1` to `jdk-tier3`, other `jndi/ldap` regression and JCK > naming tests show no failures. Look reasonable to me. If tier2 and all jndi tests are still passing, I'm good with it. - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15526#pullrequestreview-1617680805
RFR: 8277954: Replace use of monitors with explicit locks in the JDK LDAP provider implementation
The change proposed in this PR improves the behavior of the JDK JNDI/LDAP provider when running in a virtual thread. Currently, when an LDAP operation is performed from a virtual thread context a pinned carrier thread is detected: Thread[#29,ForkJoinPool-1-worker-1,5,CarrierThreads] java.naming/com.sun.jndi.ldap.Connection.read reply(Connection.java:444) <== monitors:1 java.naming/com.sun.jndi.ldap.LdapClient.ldapBind(LdapClient.java:369) <== monitors:1 java.naming/com.sun.jndi.ldap.LdapClient.authenticate(LdapClient.java:196) <== monitors:1 java.naming/com.sun.jndi.ldap.LdapCtx.connect(LdapCtx.java:2896) <== monitors:1 To fix that monitor usages are replaced with `j.u.c` locks. All synchronized blocks/methods have been replaced with locks even if it is only guarding memory access - the motivation behind such a decision was to avoid an analysis of scenarios where a mix of monitors and `j.u.c` locks is used. There are three types of mechanical changes done in this PR: 1. Classes with `synchronized` blocks or `synchronized` methods have been updated to include a new `ReentrantLock` field. These new fields are used to replace `synchronized` blocks/methods. 1. classes that use notify/wait on object monitor have been updated to use `ReentrantLock.Condition`s signal/await. 1. if one class `synchronized` on an instance of another class - the `ReentrantLock` added in item (1) was made a package-protected to give access to another class. With the proposed changes pinned carrier threads are no longer detected during execution of LDAP operations. Testing: `jdk-tier1` to `jdk-tier3`, other `jndi/ldap` regression and JCK naming tests show no failures. - Commit messages: - revert incorrect changes in LdapCtx.addNamingListener, formatting and (c) updates - draft version Changes: https://git.openjdk.org/jdk/pull/15526/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15526=00 Issue: https://bugs.openjdk.org/browse/JDK-8277954 Stats: 970 lines in 11 files changed: 394 ins; 101 del; 475 mod Patch: https://git.openjdk.org/jdk/pull/15526.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15526/head:pull/15526 PR: https://git.openjdk.org/jdk/pull/15526
