Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8296472: Remove ObjectLocker around appendToClassPathForInstrumentation call #11023

Closed
wants to merge 3 commits into from

Conversation

coleenp
Copy link
Contributor

@coleenp coleenp commented Nov 7, 2022

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.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change requires a CSR request to be approved

Issues

  • JDK-8296472: Remove ObjectLocker around appendToClassPathForInstrumentation call
  • JDK-8297169: Remove ObjectLocker around appendToClassPathForInstrumentation call (CSR)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11023/head:pull/11023
$ git checkout pull/11023

Update a local copy of the PR:
$ git checkout pull/11023
$ git pull https://git.openjdk.org/jdk pull/11023/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11023

View PR using the GUI difftool:
$ git pr show -t 11023

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11023.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 7, 2022

👋 Welcome back coleenp! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Nov 7, 2022
@openjdk
Copy link

openjdk bot commented Nov 7, 2022

@coleenp The following labels will be automatically applied to this pull request:

  • core-libs
  • hotspot
  • serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Nov 7, 2022
@mlbridge
Copy link

mlbridge bot commented Nov 7, 2022

Webrevs

@coleenp coleenp changed the title 8296472: Remove a JVMTI ObjectLocker 8296472: Remove ObjectLocker around appendToClassPathForInstrumentation call Nov 7, 2022
@coleenp
Copy link
Contributor Author

coleenp commented Nov 7, 2022

I reran jvmti tests on the change to revert ClassLoaders.java.

@AlanBateman
Copy link
Contributor

I reran jvmti tests on the change to revert ClassLoaders.java.

You can revert the comment too because it would be confusing to say that it locks the class loader when it doesn't.

@coleenp
Copy link
Contributor Author

coleenp commented Nov 7, 2022

Thanks for spotting that Alan, I thought I'd reverted the entire file.

@sspitsyn
Copy link
Contributor

sspitsyn commented Nov 7, 2022

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.

@dholmes-ora
Copy link
Member

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.

@AlanBateman
Copy link
Contributor

AlanBateman commented Nov 8, 2022

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 needs to be called to really extend the class path. That method is not accessible so there is no way (except via JNI) for a custom system class loader to delegate the append to its parent. 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.

Update: Ignore the pondering about SystemDictionary::java_system_loader(), I paged in the details to remind myself.

@AlanBateman
Copy link
Contributor

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.

@coleenp
Copy link
Contributor Author

coleenp commented Nov 8, 2022

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.

@AlanBateman
Copy link
Contributor

AlanBateman commented Nov 8, 2022

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.

@coleenp
Copy link
Contributor Author

coleenp commented Nov 8, 2022

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.

@dholmes-ora
Copy link
Member

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."

@AlanBateman
Copy link
Contributor

"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).

@coleenp
Copy link
Contributor Author

coleenp commented Nov 15, 2022

What do you think of this release note ? https://bugs.openjdk.org/browse/JDK-8297073

@coleenp
Copy link
Contributor Author

coleenp commented Nov 16, 2022

The release note is closed/delivered which I think blocked reviewing this PR. Please review!

Copy link
Contributor

@sspitsyn sspitsyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.
Thanks,
Serguei

@openjdk
Copy link

openjdk bot commented Nov 16, 2022

@coleenp This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8296472: Remove ObjectLocker around appendToClassPathForInstrumentation call

Reviewed-by: sspitsyn, alanb, dholmes

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 199 new commits pushed to the master branch:

  • 651e547: 8297217: Incorrect generation name in the heap verification log message with Serial GC
  • dd55310: 8297303: ProblemList java/awt/Mouse/EnterExitEvents/DragWindowTest.java on macosx-all
  • 3ea8971: 8269817: serviceability/jvmti/DynamicCodeGenerated/DynamicCodeGeneratedTest.java timed out with -Xcomp
  • 0a3b0fc: 8296784: Provide clean mallinfo/mallinfo2 wrapper for Linux glibc platforms
  • 7b3d581: 8297293: Add java/nio/channels/FileChannel/FileExtensionAndMap.java to ProblemList
  • 251e065: 8296764: NMT: reduce loads in os::malloc
  • 0845b39: 8296796: Provide clean, platform-agnostic interface to C-heap trimming
  • c50a904: 8297195: AWTAccessor and SwingAccessor should avoid double racy reads from non-volatile fields
  • 906f1ca: 8292317: Missing null check for Iterator.forEachRemaining implementations
  • 0ec575a: 8297289: problem list runtime/vthread/RedefineClass.java and TestObjectAllocationSampleEvent.java
  • ... and 189 more: https://git.openjdk.org/jdk/compare/3baad069a65a0ac138a6aaabf451758672d12fbc...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Nov 16, 2022
@dholmes-ora
Copy link
Member

Still think this needs a CSR request - sorry. If it warrants a RN it warrants a CSR request.

@openjdk openjdk bot added csr Pull request needs approved CSR before integration and removed ready Pull request is ready to be integrated labels Nov 17, 2022
@coleenp
Copy link
Contributor Author

coleenp commented Nov 17, 2022

https://bugs.openjdk.org/browse/JDK-8297169 Probably needs better wording.

@openjdk openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Nov 18, 2022
@coleenp
Copy link
Contributor Author

coleenp commented Nov 18, 2022

CSR is approved, please review.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks.

@coleenp
Copy link
Contributor Author

coleenp commented Nov 21, 2022

Thanks David and Alan for the code review and all the help with the CSR and release notes.
/integrate

@openjdk
Copy link

openjdk bot commented Nov 21, 2022

Going to push as commit 5c33454.
Since your change was applied there have been 211 commits pushed to the master branch:

  • 0800813: 8293584: CodeCache::old_nmethods_do incorrectly filters is_unloading nmethods
  • 16ab754: 8196018: java/awt/Scrollbar/ScrollbarMouseWheelTest/ScrollbarMouseWheelTest.java fails
  • 8b8d848: 8293422: DWARF emitted by Clang cannot be parsed
  • 59d8f67: 8297265: G1: Remove unnecessary null-check in RebuildCodeRootClosure::do_code_blob
  • 2fc340a: 8148041: Test java/awt/Mouse/TitleBarDoubleClick/TitleBarDoubleClick fails on Ubuntu with mouseReleased event after double click on title bar
  • e420661: 8297238: RISC-V: C2: Use Matcher::vector_element_basic_type when checking for vector element type in predicate
  • 891c706: 8295276: AArch64: Add backend support for half float conversion intrinsics
  • 3c09498: 8297241: Update sun/java2d/DirectX/OnScreenRenderingResizeTest/OnScreenRenderingResizeTest.java
  • 45d1807: 6312651: Compiler should only use verified interface types for optimization
  • bcc6b12: 8296945: PublicMethodsTest is slow due to dependency verification with debug builds
  • ... and 201 more: https://git.openjdk.org/jdk/compare/3baad069a65a0ac138a6aaabf451758672d12fbc...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Nov 21, 2022
@openjdk openjdk bot closed this Nov 21, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 21, 2022
@openjdk
Copy link

openjdk bot commented Nov 21, 2022

@coleenp Pushed as commit 5c33454.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@coleenp coleenp deleted the cl-lock branch November 21, 2022 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org hotspot hotspot-dev@openjdk.org integrated Pull request has been integrated serviceability serviceability-dev@openjdk.org
4 participants