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

Generate our own sun.misc.Unsafe if it is not available #4124

Merged
merged 4 commits into from
Sep 23, 2021

Conversation

laurit
Copy link
Contributor

@laurit laurit commented Sep 14, 2021

Couldn't resist doing this.
When running a modular application on jdk11 users will have to add jdk.unsupported module to make sun.misc.Unsafe available. If sun.misc.Unsafe is not available generate our own version of it that delegates to jdk.internal.misc.Unsafe just like sun.misc.Unsafe in jdk does.
This pr also helps grpc/netty that is bundled in the agent access some jdk internals that it currently can't access.
Resolves #4100
Resolves #4096

import net.bytebuddy.jar.asm.Type;

/**
* Helper class for generating our own copy of sun.misc.Unsafe that delegates to
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious if you considered an approach that just compiles Java, I guess we'd be able to expose jdk.internal via --add-exports?

Copy link
Member

Choose a reason for hiding this comment

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

this sounds nice if it can work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My initial attempt was to generate a java version of sun.misc.Unsafe that uses method handles (I went for method handles from the start because I had a vague memory that some methods in internal unsafe have been renamed) to forward to internal unsafe but this failed with error: package exists in another module: jdk.unsupported. I did try a bit to coax it to work but failed. In retrospect I think I should have just moved it into a module compiled with java8, though then if I'd want to replace some method handles with direct calls I'd probably need to create a stub of internal unsafe. Anyway while fighting the compiler I realized that I could just generate bytecode and asm is my favourite framework and I haven't had a chance to use it for almost a year so it was pretty much drop everything else and start generating bytecode.

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

very interesting... 😁

I opened #4128 to track creating a smoke test at some point to exercise this.

Comment on lines 65 to 66
// tell grpc/netty that it is ok to use setAccessible(true) so it can access DirectByteBuffer
System.setProperty("io.grpc.netty.shaded.io.netty.tryReflectionSetAccessible", "true");
Copy link
Member

Choose a reason for hiding this comment

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

since we don't shade io.grpc, this could affect user app?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that is a good catch. I'll remove this.

import net.bytebuddy.jar.asm.Type;

/**
* Helper class for generating our own copy of sun.misc.Unsafe that delegates to
Copy link
Member

Choose a reason for hiding this comment

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

this sounds nice if it can work

exports.put(
unsafeClass.getPackage().getName(),
Collections.singleton(UnsafeInitializer.class.getModule()));
instrumentation.redefineModule(
Copy link
Member

Choose a reason for hiding this comment

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

Should we do that before the if (testUnsafePresent... check? If there's a sun.misc.Unsafe present in the classpath do we need to expose the jdk.internal one to the agent explicitly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that grpc/netty that we use also desires access to some things that would require extra jvm arguments. Thought that might as well use the opportunity and let netty also use jvm internals. For example granting access to internal unsafe allows netty to allocate uninitialized arrays

17:58:55.427 INFO  i.o.s.LinuxTestContainerManager - STDERR: [otel.javaagent 2021-09-15 14:58:55:441 +0000] [main] DEBUG io.grpc.netty.shaded.io.netty.util.internal.PlatformDependent0 - jdk.internal.misc.Unsafe.allocateUninitializedArray(int): available

To be honest I have no idea whether this really is beneficial.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, if exporter netty's able to use some of jdk.internal methods to make itself more efficient (I've no idea though, I just assume that 😅 ) that's a pretty good reason to expose Unsafe to the agent even if sun.misc is available.

@trask
Copy link
Member

trask commented Sep 16, 2021

@laurit how did you test this without jlink? I'd like to try that out.

@open-telemetry/java-instrumentation-approvers planning to merge this before the 1.6.0 release tomorrow, unless someone prefers to delay it until after the release.

@laurit
Copy link
Contributor Author

laurit commented Sep 16, 2021

@trask Turns out I didn't test this properly :(. The problem is that we add caffeine in boot loader not agent loader so it can't see the generated class.
To reproduce create a new project, add a main class and module-info. Leave the module info empty (later you can try adding requires jdk.unsupported;). Run main method with agent, agent will fail because unsafe is not found.

@trask trask merged commit 0f3d0cb into open-telemetry:main Sep 23, 2021
@trask
Copy link
Member

trask commented Sep 23, 2021

🥳

@bcmedeiros
Copy link

Good job, guys, thanks for the fix :)

@laurit laurit deleted the unsafe branch July 6, 2023 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants