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

6246: Making the agent stop using Unsafe #35

Closed
wants to merge 2 commits into from
Closed

Conversation

@tabjy
Copy link
Contributor

tabjy commented Jan 17, 2020

This patch addresses JMC-6246.

The usage of Unsafe.defineClass() is replaced with:

  • for pre-Java 9, reflective access on ClassLoader.getSystemClassLoader().defineClass()
  • for Java 9 and later, MethodHandles.Lookup.defineClass()

Please note that LookUp.defineClass() can only define classes within the same package of the lookup. Therefore generated events classes will be in the dedicated package org.openjdk.jmc.agent.generated_events, instead of being in the same package of the instrumented class.

Progress

  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

JMC-6246: Making the agent stop using Unsafe

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 17, 2020

👋 Welcome back kxu! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request (refresh this page to view it).

@openjdk openjdk bot added the rfr label Jan 17, 2020
@mlbridge
Copy link

mlbridge bot commented Jan 17, 2020

Webrevs

@thegreystone
Copy link
Collaborator

thegreystone commented Jan 17, 2020

Doesn't this affect in which class loader the event classes are defined, and especially their unload behaviour?

@tabjy
Copy link
Contributor Author

tabjy commented Jan 17, 2020

In the case of reflective access, it's called directly on the system class loader. For Lookup.defineClass, the event class is defined on the class loader of the lookup, which in this case is the system class loader that loads org.openjdk.jmc.agent.generated_events.Dummy.

In both cases, events are defined with the system class loader, just like Unsafe.defineClass.

@thegreystone
Copy link
Collaborator

thegreystone commented Jan 19, 2020

I was under the impression that the class loader and protection domain arguments given to define class would define the class with that class loader and protection domain, allowing the event class to be unloaded when the class where it is being used is unloaded. If this is not done, we’re basically introducing a class leak.

@tabjy
Copy link
Contributor Author

tabjy commented Jan 22, 2020

You're right. Sorry I didn't consider thoroughly. I'm cancelling this PR for a better solution.

@tabjy tabjy closed this Jan 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.