Skip to content

8334617: Build always compiles gtest depending on Serial GC, failing build if disabled #31049

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.

Sign up for GitHub

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

Open
tschatzl wants to merge 3 commits into
base: master
Choose a base branch
from
+0 −0
Open

8334617: Build always compiles gtest depending on Serial GC, failing build if disabled #31049

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bbce89d
8334617
tschatzl May 5, 2026
4a3a3c1
* use the default collector, do not force Serial GC for anything
tschatzl May 8, 2026
b278aac
* extract g1 as default changes
tschatzl May 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion make/RunTestsPrebuiltSpec.gmk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,10 @@ TEST_JOBS ?= 0
# Use hard-coded values for java flags (one size, fits all!)
JAVA_FLAGS := -Duser.language=en -Duser.country=US
JAVA_FLAGS_BIG := -Xms64M -Xmx2048M
JAVA_FLAGS_SMALL := -XX:+UseSerialGC -Xms32M -Xmx512M -XX:TieredStopAtLevel=1
JAVA_FLAGS_SMALL := -Xms32M -Xmx512M -XX:TieredStopAtLevel=1
ifneq ($(call check-jvm-feature, serialgc), true)
JAVA_FLAGS_SMALL += -XX:+UseSerialGC
endif
BUILDJDK_JAVA_FLAGS_SMALL := -Xms32M -Xmx512M -XX:TieredStopAtLevel=1
BUILD_JAVA_FLAGS := $(JAVA_FLAGS_BIG)

Expand Down
2 changes: 1 addition & 1 deletion make/autoconf/boot-jdk.m4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,7 +482,7 @@ AC_DEFUN_ONCE([BOOTJDK_SETUP_BOOT_JDK_ARGUMENTS],
AC_MSG_CHECKING([flags for boot jdk java command for small workloads])

# 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])
Copy link
Copy Markdown
Member

@shipilev shipilev May 6, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@shipilev shipilev May 6, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

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 :)

Copy link
Copy Markdown
Contributor Author

@tschatzl tschatzl May 6, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@erikj79 erikj79 May 6, 2026

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Member

@dholmes-ora dholmes-ora May 7, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I just want to state for the record (again) that despite the nice illusionary picture painted by all the #ifdef INCLUDE_XXX in the code, it was never a goal/expectation to be able to build OpenJDK with an arbitrary combination of selected features. The selectivity arose from the requirement to have a Minimal VM and Compact Profiles back in JDK 8. And of course as new GCs are developed they get their own special include/exclude handling. In that regard Serial GC is/was always expected to be present.

Copy link
Copy Markdown
Contributor Author

@tschatzl tschatzl May 7, 2026

Choose a reason for hiding this comment

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

Jmod and later some javap executions fail as they use the JAVA_TOOL_FLAGS_SMALL flags.

I'll do some more testing with these flags removed, to me the perf difference is negligible (when we diagnosed/added these flags, iirc the diagnosed issue has been mostly VM startup where G1 has been much slower. This is not the case any more for a long time).

If not I will introduce a separate BUILD_JAVA_TOOL_FLAGS_SMALL.

UTIL_ADD_JVM_ARG_IF_OK([-Xms32M],boot_jdk_jvmargs_small,[$JAVA])
UTIL_ADD_JVM_ARG_IF_OK([-Xmx512M],boot_jdk_jvmargs_small,[$JAVA])
UTIL_ADD_JVM_ARG_IF_OK([-XX:TieredStopAtLevel=1],boot_jdk_jvmargs_small,[$JAVA])
Expand Down
14 changes: 14 additions & 0 deletions make/autoconf/util.m4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,20 @@ AC_DEFUN([UTIL_ADD_JVM_ARG_IF_OK],
fi
])

################################################################################
# Test if the GC feature $4 is supported by the resulting JVM build,
# and $1 a valid argument to $3 (often is $JAVA, passed as $3),
# append $1 to $2
# Also set JVM_ARG_OK to true/false depending on outcome.
AC_DEFUN([UTIL_ADD_JVM_GC_ARG_IF_OK],
[
if JVM_FEATURES_IS_ACTIVE([$4]); then
UTIL_ADD_JVM_ARG_IF_OK([$1], [$2], [$3])
else
JVM_ARG_OK=false
fi
])

################################################################################
# Register a --with argument but mark it as deprecated
# $1: The name of the with argument to deprecate, not including --with-
Expand Down
0 .../gtest/gc/shared/test_collectorPolicy.cpp → .../gtest/gc/serial/test_collectorPolicy.cpp
View file
Open in desktop
File renamed without changes.