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

Use Metrics implementation from the base JDK for container detection support #7246

Closed
jerboaa opened this issue Aug 21, 2023 · 3 comments · Fixed by #7265
Closed

Use Metrics implementation from the base JDK for container detection support #7246

jerboaa opened this issue Aug 21, 2023 · 3 comments · Fixed by #7265

Comments

@jerboaa
Copy link
Collaborator

jerboaa commented Aug 21, 2023

Feature request

GraalVM native-image historically included a fork of OpenJDK's system metrics classes. Since then, the Metrics implementation as part of the JDK as undergone many revisions and bug fixes. The GraalVM code clone, on the other hand, has had few updates. One approach would be to update the GraalVM code and keep it in sync with the JDK code. A better way, however, seems to be to use the Metrics classes from the JDK so as to get fixes in GraalVM by nature of the JDK code getting fixed.

Is your feature request related to a problem?

This move would avoid the following issues observed by GraalVM users in the wild:

Describe the solution you'd like.

First propose a minimal patch to OpenJDK so as to avoid the cyclic dependency of Metrics with ByteBuffers. Then, remove the metrics copy of substrate like this exemplary patch.

Describe who do you think will benefit the most.

GraalVM users who run GraalVM and/or produced native images on a wide variety of container orchestration frameworks (common case these days).

Describe alternatives you've considered.

The alternative is to keep the copy and update it regularly with JDK code changes, which is a maintenance headache.

Additional context.

This move also would avoid needing to write separate container testing framework in GraalVM as the JDK has tests covering such scenarios.

Express whether you'd like to help contributing this feature

I'm happy to contribute the minimal OpenJDK change upstream, produce relevant backports for it, and then remove the clone in GraalVM and replacing it by the JDK version instead.

@jerboaa
Copy link
Collaborator Author

jerboaa commented Aug 24, 2023

https://bugs.openjdk.org/browse/JDK-8314940 tracks the minimal change in OpenJDK.

Native Image automation moved this from To do to Done Aug 25, 2023
@jerboaa
Copy link
Collaborator Author

jerboaa commented Aug 28, 2023

https://bugs.openjdk.org/browse/JDK-8314940 tracks the minimal change in OpenJDK.

Corresponding labs jdk 21 change (i.e. will only affect Mandrel):
graalvm/labs-openjdk-21@db1723c

@jerboaa
Copy link
Collaborator Author

jerboaa commented Aug 28, 2023

https://bugs.openjdk.org/browse/JDK-8314940 tracks the minimal change in OpenJDK.

Looks like the OpenJDK change is a no-go. Admittedly it was an OpenJDK foreign change and should get solved in the foreign code - GraalVM - instead (which makes some sense).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Native Image
  
Done
Development

Successfully merging a pull request may close this issue.

1 participant