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

Workaround CacheDataStore use with source launcher #15

Closed
wants to merge 3 commits into from

Conversation

shipilev
Copy link
Member

@shipilev shipilev commented Sep 6, 2024

I was trying to test the simplest workload that involves javac, and that is source launcher. When I attempt to use source launcher with -XX:CacheDataStore, it fails with:

$ rm -f app.cds*; build/linux-x86_64-server-release/images/jdk/bin/java -XX:CacheDataStore=app.cds -Xmx256m -Xms256m -XX:+UnlockExperimentalVMOptions -XX:+UseEpsilonGC HelloStream.java
[0.001s][warning][cds] optimized module handling/full module graph: disabled due to incompatible property: jdk.module.addmods=ALL-DEFAULT
Error occurred during initialization of VM
CacheDataStore cannot be created because AOTClassLinking is enabled but full module graph is disabled

Source launcher adds --add-modules=ALL-DEFAULT:

$ man java
...
       In source-file mode, execution proceeds as follows:

...
       • The compiled classes are executed in  the  context  of  an  unnamed  module,  as  though  --add-mod‐
         ules=ALL-DEFAULT  is  in  effect.  This is in addition to any other --add-module options that may be
         have been specified on the command line.

I think we can work that around specifically for Leyden, and also leave a TODO breadcrumb in the code that we need to figure this out on CDS side. This PR does that workaround. With it, source launcher works with simple examples:

$ build/linux-x86_64-server-release/images/jdk/bin/java -XX:CacheDataStore=app.cds -Xmx256m -Xms256m -XX:+UnlockExperimentalVMOptions -XX:+UseEpsilonGC  HelloStream.java
Warning: CacheDataStore cannot be used with default source launcher environment, disabling --add-modules=ALL-DEFAULT
hello, world

Progress

  • Change must not contain extraneous whitespace
  • Change must be properly reviewed (1 review required, with at least 1 Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/leyden/pull/15.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 6, 2024

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

@openjdk
Copy link

openjdk bot commented Sep 6, 2024

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

@openjdk openjdk bot added the rfr Pull request is ready for review label Sep 6, 2024
@mlbridge
Copy link

mlbridge bot commented Sep 6, 2024

Webrevs

@vnkozlov
Copy link
Collaborator

vnkozlov commented Sep 6, 2024

We are trying to avoid change Java launcher to check Leyden flags (especially -XX:) if we can do changes in VM.
See discussion in PR: openjdk/jdk#20516

@shipilev
Copy link
Member Author

shipilev commented Sep 6, 2024

We are trying to avoid change Java launcher to check Leyden flags (especially -XX:) if we can do changes in VM. See discussion in PR: openjdk/jdk#20516

Yeah, I understand we don't want to change launcher if we can avoid it. The actual trouble is on CDS side and how --add-modules=ALL-DEFAULT disables CacheDataStore. Given that most (if not all?) source-launched examples I saw do not require modules enabled, this breakage is for naught. So not doing --add-modules when CDS is enabled is a convenience for performance testing, plus a breadcrumb for future work.

This is similar in spirit to what we (used to?) do for JVMCI modules:

/*
* Ioi - 2023/05/19. There's no need for this with my patch-jdk.sh script, which adds
* jdk.internal.vm.ci as one of the default modules. Using -Djdk.module.addmods will
* cause the full module graph to be disabled and slow down performance.
*/
if (!TempDisableAddJVMCIModule && ClassLoader::is_module_observable("jdk.internal.vm.ci")) {
if (!create_numbered_module_property("jdk.module.addmods", "jdk.internal.vm.ci", addmods_count++)) {
return false;
}
}

@shipilev
Copy link
Member Author

shipilev commented Sep 6, 2024

So not doing --add-modules when CDS is enabled is a convenience for performance testing, plus a breadcrumb for future work.

We can move this check to VM, but it is not convenient to figure our if we are running in LM_SOURCE mode or not on the VM side.

@vnkozlov
Copy link
Collaborator

vnkozlov commented Sep 6, 2024

Lets wait @iklam comment on this.

@iklam
Copy link
Member

iklam commented Sep 6, 2024

I am not sure if Leyden should be targeting running the launcher with a Java source file. There are many things that we could do to make some things run a little faster, but I think we should resist the temptation in this case.

@shipilev
Copy link
Member Author

shipilev commented Sep 6, 2024

I think what Leyden targets is a different question.

After a day of playing with this, I think enabling source launcher does look very convenient for current performance testing, as it includes javac automatically, without any additional setup. I am not insisting on doing this workaround, it is just a handy development aid. If you all feel strongly against it, I can go the longer and less convenient route to measure CDS perf.

@iklam
Copy link
Member

iklam commented Sep 6, 2024

But you will be adding javac timing to every one of your benchmarks. Is this really what you want?

If you want convenience, I think it's better to write a simple/generic script that compiles the Java source into a JAR file and then do it the usual way.

@shipilev
Copy link
Member Author

shipilev commented Sep 6, 2024

But you will be adding javac timing to every one of your benchmarks. Is this really what you want?

Yes, it is exactly that: the easiest way for me to target both javac and the app is to add .java to my command line :)

@shipilev
Copy link
Member Author

No need for this anymore. We can just do javac tests directly.

@shipilev shipilev closed this Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfr Pull request is ready for review
Development

Successfully merging this pull request may close these issues.

3 participants