8346781: [JVMCI] Limit ServiceLoader to class initializers #22869
Conversation
|
👋 Welcome back dnsimon! A progress list of the required criteria for merging this PR into |
|
@dougxc 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: 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 2 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
| @@ -107,7 +107,7 @@ private static synchronized void remove(Cleaner cl) { | |||
| /** | |||
| * Remove the cleaners whose referents have become weakly reachable. | |||
| */ | |||
| static void clean() { | |||
| public static void clean() { | |||
There was a problem hiding this comment.
Making this public avoids the need for a method substitution.
| * from (being compiled into) the library. Such code must be directly guarded by an {@code if} | ||
| * statement on this field - the guard cannot be behind a method call. | ||
| */ | ||
| public static final boolean IS_BUILDING_NATIVE_IMAGE = Boolean.parseBoolean(VM.getSavedProperty("jdk.vm.ci.services.aot")); |
There was a problem hiding this comment.
This field is no longer used in JVMCI and I will remove its usages in Graal.
| * @throws SecurityException if a security manager is present and it denies <tt> | ||
| * {@link RuntimePermission}("jvmci")</tt> | ||
| */ | ||
| public static <S> Iterable<S> load(Class<S> service) { |
There was a problem hiding this comment.
There are no usages of this method in JVMCI or Graal.
| * @throws SecurityException if a security manager is present and it denies <tt> | ||
| * {@link RuntimePermission}("jvmci")</tt> | ||
| */ | ||
| public static <S> S loadSingle(Class<S> service, boolean required) { |
There was a problem hiding this comment.
There are no usages of this method in JVMCI or Graal.
In the context of libgraal, the current use of ServiceLoader in JVMCI is problematic as libgraal does all class loading at image build time. There are static fields such as
JVMCIServiceLocator.cachedLocatorsthat need to be initialized via reflection when building libgraal.This PR removes the need for such reflection by moving all use of ServiceLoader in JVMCI into
<clinit>methods. These methods are executed when building libgraal. It also removes a few other public methods and fields that are no longer used by Graal. Given that JVMCI is still experimental and only has qualified exports to Graal, I don't think this needs a CSR.Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/22869/head:pull/22869$ git checkout pull/22869Update a local copy of the PR:
$ git checkout pull/22869$ git pull https://git.openjdk.org/jdk.git pull/22869/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 22869View PR using the GUI difftool:
$ git pr show -t 22869Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/22869.diff
Using Webrev
Link to Webrev Comment