Re: RFR: 8296472: Remove ObjectLocker around appendToClassPathForInstrumentation call [v3]
On Mon, 7 Nov 2022 20:40:33 GMT, Coleen Phillimore wrote: >> This patch moves the acquisition of the boot class loader lock out of the >> JVM and into the Java function. >> Tested with tier1-4, and jvmti and jdi tests locally. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > really revert the file Thanks David and Alan for the code review and all the help with the CSR and release notes. - PR: https://git.openjdk.org/jdk/pull/11023
Re: RFR: 8296472: Remove ObjectLocker around appendToClassPathForInstrumentation call [v3]
On Mon, 7 Nov 2022 20:40:33 GMT, Coleen Phillimore wrote: >> This patch moves the acquisition of the boot class loader lock out of the >> JVM and into the Java function. >> Tested with tier1-4, and jvmti and jdi tests locally. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > really revert the file Looks good. Thanks. - Marked as reviewed by dholmes (Reviewer). PR: https://git.openjdk.org/jdk/pull/11023
Re: RFR: 8296472: Remove ObjectLocker around appendToClassPathForInstrumentation call [v3]
On Mon, 7 Nov 2022 20:40:33 GMT, Coleen Phillimore wrote: >> This patch moves the acquisition of the boot class loader lock out of the >> JVM and into the Java function. >> Tested with tier1-4, and jvmti and jdi tests locally. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > really revert the file Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.org/jdk/pull/11023
Re: RFR: 8296472: Remove ObjectLocker around appendToClassPathForInstrumentation call [v3]
On Mon, 7 Nov 2022 20:40:33 GMT, Coleen Phillimore wrote: >> This patch moves the acquisition of the boot class loader lock out of the >> JVM and into the Java function. >> Tested with tier1-4, and jvmti and jdi tests locally. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > really revert the file CSR is approved, please review. - PR: https://git.openjdk.org/jdk/pull/11023
Re: RFR: 8296472: Remove ObjectLocker around appendToClassPathForInstrumentation call [v3]
On Mon, 7 Nov 2022 20:40:33 GMT, Coleen Phillimore wrote: >> This patch moves the acquisition of the boot class loader lock out of the >> JVM and into the Java function. >> Tested with tier1-4, and jvmti and jdi tests locally. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > really revert the file https://bugs.openjdk.org/browse/JDK-8297169 Probably needs better wording. - PR: https://git.openjdk.org/jdk/pull/11023
Re: RFR: 8296472: Remove ObjectLocker around appendToClassPathForInstrumentation call [v3]
On Mon, 7 Nov 2022 20:40:33 GMT, Coleen Phillimore wrote: >> This patch moves the acquisition of the boot class loader lock out of the >> JVM and into the Java function. >> Tested with tier1-4, and jvmti and jdi tests locally. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > really revert the file Still think this needs a CSR request - sorry. If it warrants a RN it warrants a CSR request. - PR: https://git.openjdk.org/jdk/pull/11023
Re: RFR: 8296472: Remove ObjectLocker around appendToClassPathForInstrumentation call [v3]
On Mon, 7 Nov 2022 20:40:33 GMT, Coleen Phillimore wrote: >> This patch moves the acquisition of the boot class loader lock out of the >> JVM and into the Java function. >> Tested with tier1-4, and jvmti and jdi tests locally. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > really revert the file Looks good. Thanks, Serguei - Marked as reviewed by sspitsyn (Reviewer). PR: https://git.openjdk.org/jdk/pull/11023
Re: RFR: 8296472: Remove ObjectLocker around appendToClassPathForInstrumentation call [v3]
On Mon, 7 Nov 2022 20:40:33 GMT, Coleen Phillimore wrote: >> This patch moves the acquisition of the boot class loader lock out of the >> JVM and into the Java function. >> Tested with tier1-4, and jvmti and jdi tests locally. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > really revert the file The release note is closed/delivered which I think blocked reviewing this PR. Please review! - PR: https://git.openjdk.org/jdk/pull/11023
Re: RFR: 8296472: Remove ObjectLocker around appendToClassPathForInstrumentation call [v3]
On Mon, 7 Nov 2022 20:40:33 GMT, Coleen Phillimore wrote: >> This patch moves the acquisition of the boot class loader lock out of the >> JVM and into the Java function. >> Tested with tier1-4, and jvmti and jdi tests locally. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > really revert the file What do you think of this release note ? https://bugs.openjdk.org/browse/JDK-8297073 - PR: https://git.openjdk.org/jdk/pull/11023
Re: RFR: 8296472: Remove ObjectLocker around appendToClassPathForInstrumentation call [v3]
On Tue, 8 Nov 2022 23:45:18 GMT, David Holmes wrote: > "in the custom class loader must append to the class path in a thread-safe > manner." Maybe "its search path" rather than "the class path" because the custom class loader can't add to the class path (the application class path continues to be owned by the application class loader). - PR: https://git.openjdk.org/jdk/pull/11023
Re: RFR: 8296472: Remove ObjectLocker around appendToClassPathForInstrumentation call [v3]
On Tue, 8 Nov 2022 22:08:06 GMT, Coleen Phillimore wrote: > in the custom class loader must synchronize appending to the class path I would say: "in the custom class loader must append to the class path in a thread-safe manner." - PR: https://git.openjdk.org/jdk/pull/11023
Re: RFR: 8296472: Remove ObjectLocker around appendToClassPathForInstrumentation call [v3]
On Mon, 7 Nov 2022 20:40:33 GMT, Coleen Phillimore wrote: >> This patch moves the acquisition of the boot class loader lock out of the >> JVM and into the Java function. >> Tested with tier1-4, and jvmti and jdi tests locally. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > really revert the file I could add a release note to say something like: When running a java application with the options "-javaagent:myagent.jar -Djava.system.classloader=MyClassLoader" the myagent.jar is added to the custom system class loader rather then the application class loader. The JVM no longer synchronizes on the custom class loader while calling appendToClassPathForInstrumentation. The appendToClassPathForInstrumentation method in the custom class loader must synchronize appending to the class path. - PR: https://git.openjdk.org/jdk/pull/11023
Re: RFR: 8296472: Remove ObjectLocker around appendToClassPathForInstrumentation call [v3]
On Tue, 8 Nov 2022 17:03:05 GMT, Coleen Phillimore wrote: > Can one create a class loader to override this function, and does one expect > the JVM to synchronize it on the system class loader object (returned by > getSystemClassLoader)? This feels too remote to try to explain in a CSR. > > I'll look to see if we have tests for this case. In most cases, system class loader == the application class loader. It should be very rare to run with -Djava.system.class.loader=... but we know of a few application servers that do use it. The behavior prior to JDK 9 was to always call the app class loader's appendToClassPathForInstrumentation. This changed in JDK 9 (see JDK-8160950) to call the custom system class loader's appendToClassPathForInstrumentation. The changes for JDK-8160950 added test/java/lang/instrument/CustomSystemLoader so running the test/java/lang/instrument tests would be good. I'd forgotten about that change when initially commenting here. There isn't anything in the API docs about synchronization on the class loader object, nothing should be depending on that but it wouldn't do any harm to note the change in a release note. - PR: https://git.openjdk.org/jdk/pull/11023
Re: RFR: 8296472: Remove ObjectLocker around appendToClassPathForInstrumentation call [v3]
On Mon, 7 Nov 2022 20:40:33 GMT, Coleen Phillimore wrote: >> This patch moves the acquisition of the boot class loader lock out of the >> JVM and into the Java function. >> Tested with tier1-4, and jvmti and jdi tests locally. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > really revert the file We call compute_java_loaders with upcall to ClassLoader.getSystemClassLoader() after initPhase3 but before JVMTI live phase: https://github.com/coleenp/jdk/blob/master/src/hotspot/share/runtime/threads.cpp#L731 which is when jvmti calls this: https://github.com/coleenp/jdk/blob/master/src/hotspot/share/prims/jvmtiEnv.cpp#L703 Can one create a class loader to override this function, and does one expect the JVM to synchronize it on the system class loader object (returned by getSystemClassLoader)? This feels too remote to try to explain in a CSR. I'll look to see if we have tests for this case. - PR: https://git.openjdk.org/jdk/pull/11023
Re: RFR: 8296472: Remove ObjectLocker around appendToClassPathForInstrumentation call [v3]
On Mon, 7 Nov 2022 20:40:33 GMT, Coleen Phillimore wrote: >> This patch moves the acquisition of the boot class loader lock out of the >> JVM and into the Java function. >> Tested with tier1-4, and jvmti and jdi tests locally. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > really revert the file I checked the JLPIS agent (= j.l.instrument implementation) with a custom system class loader that doesn't define a appendToClassPathForInstrumentation method and it isn't handled correctly. The right behavior should be for AddToSystemClassLoaderSearch to throw UOE but instead it aborts the VM. So I think we'll create a JBS issue for that. If the custom system class loader does define appendToClassPathForInstrumentation then it will be called, it's just not possible for it to delegate it to the application class loader's appendToClassPathForInstrumentation. It's very possible this will lead to some anomalies as the defining class loader for the classes on the original class path will be the app class loader but any classes added by the agent at runtime (after startup) will likely be the custom system class loader. This is Java agents as opposed to JVMTI agents but it does suggest that the combination of custom system class loader and agents augmenting the class path at runtime is not well tested. So ObjectLocker or not, it is unlikely to be detected by any tests. - PR: https://git.openjdk.org/jdk/pull/11023
Re: RFR: 8296472: Remove ObjectLocker around appendToClassPathForInstrumentation call [v3]
On Tue, 8 Nov 2022 04:23:13 GMT, David Holmes wrote: > Note the loader involved need not be our own default platform loader and we > don't know how a custom system loader might operate. The specification for > this says nothing about thread-safety, nor that the VM will do any locking, > so I think it is okay to remove it - but it is a change in behaviour that > should be documented by a CSR request. In that configuration, the custom system class loader will be created with the app class loader as its parent. It's still the app class loader that loads from the class path and it will be the app class loader's appendToClassPathForInstrumentation method that is called (there's no equivalent for custom system class loaders). I would hope there are tests for this but the discussion makes me wonder what SystemDictionary::java_system_loader() returns, esp. given a custom class loader won't be created until initPhase3. - PR: https://git.openjdk.org/jdk/pull/11023
Re: RFR: 8296472: Remove ObjectLocker around appendToClassPathForInstrumentation call [v3]
On Mon, 7 Nov 2022 20:40:33 GMT, Coleen Phillimore wrote: >> This patch moves the acquisition of the boot class loader lock out of the >> JVM and into the Java function. >> Tested with tier1-4, and jvmti and jdi tests locally. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > really revert the file Note the loader involved need not be our own default platform loader and we don't know how a custom system loader might operate. The specification for this says nothing about thread-safety, nor that the VM will do any locking, so I think it is okay to remove it - but it is a change in behaviour that should be documented by a CSR request. - PR: https://git.openjdk.org/jdk/pull/11023
Re: RFR: 8296472: Remove ObjectLocker around appendToClassPathForInstrumentation call [v3]
On Mon, 7 Nov 2022 20:40:33 GMT, Coleen Phillimore wrote: >> This patch moves the acquisition of the boot class loader lock out of the >> JVM and into the Java function. >> Tested with tier1-4, and jvmti and jdi tests locally. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > really revert the file > We might not need this. appendClasspath is thread safe so it's okay for > several > agents calling appendToSystemClassLoaderSearch at around the same time. > I don't think it needs to sycnrhonize with anything else. I'm thinking about a good place where to place a comment about this. Probably, it can be placed before the method `appendToClassPathForInstrumentation`. - PR: https://git.openjdk.org/jdk/pull/11023
Re: RFR: 8296472: Remove ObjectLocker around appendToClassPathForInstrumentation call [v3]
> This patch moves the acquisition of the boot class loader lock out of the JVM > and into the Java function. > Tested with tier1-4, and jvmti and jdi tests locally. Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision: really revert the file - Changes: - all: https://git.openjdk.org/jdk/pull/11023/files - new: https://git.openjdk.org/jdk/pull/11023/files/781b44fc..66e9133b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=11023=02 - incr: https://webrevs.openjdk.org/?repo=jdk=11023=01-02 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/11023.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11023/head:pull/11023 PR: https://git.openjdk.org/jdk/pull/11023
