8334617: -XX:+UseSerialGC sent to newly built JDK even if serialgc is disabled#31049
8334617: -XX:+UseSerialGC sent to newly built JDK even if serialgc is disabled#31049tschatzl wants to merge 1 commit intoopenjdk:masterfrom
Conversation
Hi all, please review this change that allows building of the JDK without Serial GC enabled. There were some hardcoded entries for -XX:+UseSerialGC in the makefiles, and one of the gtests is serial gc specific and needed to be moved. I opted to a different fix than suggested in the CR, instead of wholesale ignoring unrecognized options, only add serial gc to the options if the feature has been enabled. Tested manually with and without `--with-jvm-features=-serialgc`, both image building and running the test works in both cases (being skipped/not found if serial gc has not been enabled). Other testing: gha Thanks, Thomas
|
👋 Welcome back tschatzl! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
openjdk
Bot
changed the title
openjdk
Bot
added
build
|
The total number of required reviews for this PR has been set to 2 based on the presence of this label: |
|
|
||
| # Use serial gc for small short lived tools if possible | ||
| UTIL_ADD_JVM_ARG_IF_OK([-XX:+UseSerialGC],boot_jdk_jvmargs_small,[$JAVA]) | ||
| UTIL_ADD_JVM_GC_ARG_IF_OK([-XX:+UseSerialGC],boot_jdk_jvmargs_small,[$JAVA],[serialgc]) |
There was a problem hiding this comment.
Something is off here. This builds up boot_jdk_jvmargs, which has no relationship to the JVM feature flags enabled currently in the build. That is, if you have a boot JDK that does not carry Serial, it would still fail.
There is a block below, saying:
# Don't presuppose SerialGC is present in the buildjdk. Also, we cannot test
# the buildjdk, but on the other hand we know what it will support.
Which seems to say that we just "know" Serial would be there. But that is a flimsy precondition. So maybe we should "just" stop opting into Serial GC everywhere, and rely on default collector choice.
There was a problem hiding this comment.
Actually, I believe not using Serial for small tools is completely in line with the intent of JEP 523, so it would be a proverbial dog-fooding to rely on G1 for small tools. If G1 is significantly slower in scenario where Serial used to be a better choice, OpenJDK developers should be the first to suffer :)
There was a problem hiding this comment.
In my somewhat limited local tests (for jep 523), the difference of using g1 for everything is <1% non-significant for a full slowdebug rebuild (in all metrics, i.e. user/sys/cpu/total time, 5 runs, alpha=0.05), i.e. a make images.
I am fine with just removing the -XX:+SerialGC optimization. I did notice that comment, and agree with you, but did not want to do extensive build perf testing.
See also https://bugs.openjdk.org/browse/JDK-8359802?focusedId=14802646&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14802646 on a different machine.
If nobody objects I'll remove this optimization after some more testing.
There was a problem hiding this comment.
If default GC choice and sizing is good enough, I'm all for dropping the explicit flags. Back when we set serialgc, I believe the difference was quite significant. If you can show that the difference is negligible, then please remove it.
If the difference is still relevant, then the correct fix is to only modify the BUILD_JAVA flags, and then only if we are using the just built JDK as BUILD_JDK. Looking at the code, as Alexey pointed out, we aren't even setting serialgc in the BUILD_JAVA_FLAGS_SMALL. We are however setting JAVA_TOOL_FLAGS_SMALL based on JAVA_FLAGS_SMALL, and we then use JAVA_TOOL_FLAGS_SMALL with JMOD, which is run from the BUILD_JDK. That is a bug. We need a separate BUILD_JAVA_TOOL_FLAGS_SMALL. Is it the jmod command that failed when you disabled serialgc, or something else?
3 participants
Hi all,
please review this change that allows building of the JDK without Serial GC enabled.
There were some hardcoded entries for -XX:+UseSerialGC in the makefiles, and one of the gtests is serial gc specific and needed to be moved. Noticed during tests with JEP 523: Make G1 the Default Garbage Collector in All Environments.
I opted to a different fix than suggested in the CR, instead of wholesale ignoring unrecognized options, only add serial gc to the relevant options if the feature is enabled in the build. The change seems small enough.
Tested manually with and without
--with-jvm-features=-serialgc, both image building and running the test works in both cases (being skipped/not found if serial gc has not been enabled).Other testing: gha
Thanks,
Thomas
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/31049/head:pull/31049$ git checkout pull/31049Update a local copy of the PR:
$ git checkout pull/31049$ git pull https://git.openjdk.org/jdk.git pull/31049/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 31049View PR using the GUI difftool:
$ git pr show -t 31049Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/31049.diff
Using Webrev
Link to Webrev Comment