Add support for mixed references mode in CMake builds #11166

sharon-wang · 2020-11-12T19:00:56Z

Mixed references mode is enabled by specifying --with-mixedrefs=[static|dynamic] in the configure options. If --with-mixedrefs is provided without indicating static or dynamic, static is assumed by default.

In static mixed references mode, OMR_MIXED_REFERENCES_MODE_STATIC is enabled in OMR. When this is enabled, two GC libraries will be built:

j9gc/omrgc, where OMR_OVERRIDE_COMPRESS_OBJECT_REFERENCES=1 , and
j9gc_full/omrgc_full, where OMR_OVERRIDE_COMPRESS_OBJECT_REFERENCES=0

The other GC libraries are also split into regular and _full in static mode.

In full, compressed or mixed dynamic mode, the original "unsplit" libraries will continue to be built (i.e. j9gc/omrgc and related GC libraries).

Summary of Changes

New CMake SPEC files have been created for each platform/OS to support mixed references mode
Existing GC library CMakeLists files have been updated for mixed references static mode (build separate _full GC libraries)
GC library selection has been updated to support the new _full GC libraries when in mixed references static mode
Documentation has been added to note --with-mixedrefs=[static|dynamic] is now available (only with CMake)
Jenkins pipeline changes have been added to support building CMake Mixed References mode

Note: GC libraries related to Verbose have not been updated. Investigation is ongoing by the GC team (cc @dmitripivkine). This means that verbose is not supported for the full references GC library in mixed references static mode.

These changes only enable mixed references mode with CMake. Once this work is all merged, the equivalent UMA changes will be made.

When using a mixedrefs build, you can specify JVM options -Xcompressedrefs or -Xnocompressedrefs to choose between compressed and full pointers.

Here's a list of all the jenkins shortnames to run mixed refs CMake builds. These will run builds with extra configure options --with-mixedrefs and --with-cmake. (This list is up to date with changes from #11459)

aixmxd: ppc64_aix_mixed
zlinuxmxd: s390x_linux_mixed
plinuxmxd: ppc64le_linux_mixed
xlinuxmxd: x86-64_linux_mixed
winmxd: x86-64_windows_mixed
osxmxd: x86-64_mac_mixed
alinux64mxd: aarch64_linux_mixed
~~zosmxd: s390x_zos_mixed~~ // CMake is not currently available for ZOS

Issue Discussion: #8878
Related Issue "Merge compressedrefs and non-compressedrefs tests builds into one": #9231

OMR Changes

Add support for mixed references mode in CMake builds eclipse/omr#5657

Extensions Changes

sharon-wang · 2020-11-12T19:10:31Z

Please review @gacholio @keithc-ca @dnakamura

runtime/gc_modron_startup/mminit.cpp

buildenv/jenkins/variables/defaults.yml

runtime/gc_modron_startup/CMakeLists.txt

runtime/gc_modron_startup/mminit.cpp

runtime/include/j9lib.h.in

fjeremic · 2020-11-12T20:38:47Z

Is the plan to eventually switch Adopt builds to build with --with-mixedrefs=dynamic by default?

gacholio · 2020-11-12T20:42:54Z

Is the plan to eventually switch Adopt builds to build with --with-mixedrefs=dynamic by default?

Static for now - dynamic doesn't perform well.

runtime/compiler/control/DLLMain.cpp

runtime/gc/dllinit.c

runtime/gc_modron_startup/mminit.cpp

gacholio · 2020-11-13T17:44:22Z

Is GC check linked with the GC? Is that why it needs splitting? If it's just a performance concern, I'd leave it dynamic.

sharon-wang · 2020-11-13T17:52:43Z

Is GC check linked with the GC? Is that why it needs splitting? If it's just a performance concern, I'd leave it dynamic.

Yes, j9gccheck and j9gcchk both link against omrgc

gacholio · 2020-11-13T17:55:59Z

OK - I guess the ideal would be to have a dynamic OMR for GC check, but that's for later on (if ever).

runtime/gc_modron_startup/mminit.cpp

runtime/compiler/control/DLLMain.cpp

runtime/gc/dllinit.c

sharon-wang · 2020-11-13T19:26:02Z

@gacholio What's the expected behaviour of using -Xcompressedrefs/-Xnocompressedrefs with a mixedrefs build? Are there restrictions I should document when using those options with a static or dynamic mixedrefs build?

gacholio · 2020-11-13T19:55:26Z

The expectation is that they will function exactly the same as the dual-VM builds.

gacholio · 2020-11-13T20:11:23Z

The redirector will need to be updated to always select the "default" directory for compressed and not.

sharon-wang · 2020-11-18T22:17:50Z

There's one more change I need to add, which I'm planning to add to mxdptrs.cmake. Currently OMR_MIXED_REFERENCES_MODE_STATIC is getting set as a cache variable in the omr changes eclipse/omr#5657, but it's not occurring early enough. As such, make all would need to be run twice for the build to succeed after a make clean.

If those changes are approved, I'll squash the commits.

runtime/cmake/caches/mxdptrs.cmake

sharon-wang · 2020-11-18T22:29:36Z

@gacholio Do we want to add pipeline changes to allow builds using --with-mixedrefs=dynamic?

gacholio · 2020-11-19T01:24:48Z

@gacholio Do we want to add pipeline changes to allow builds using --with-mixedrefs=dynamic?

Eventually perhaps, but we're going to be building the static version for the forseable future.

Create new CMake spec files for mixed references mode Update CMakeLists.txt for GC libs to support mixed refs mode Update GC library selection to support mixed references static Add configure documentation for --with-mixedrefs=[static|dynamic] Add pipeline changes to build CMake mixed references mode Set OMR_MIXED_REFERENCES_MODE_STATIC in mxdptrs.cmake Declare the two glue interface libraries before adding omr Signed-off-by: Sharon Wang <sharon-wang-cpsc@outlook.com>

sharon-wang · 2020-11-24T16:16:12Z

@dnakamura Ping for your thoughts on the CMake changes here

sharon-wang · 2020-11-25T19:57:07Z

Do we want to run a PR build with these changes? The shortname for the linux mixed refs build is xlinuxmxdcm. We'll need to use the changes from eclipse/omr#5657.

keithc-ca · 2020-11-25T20:16:26Z

I'd like to hear from @dnakamura on this and eclipse/omr#5657.

runtime/cmake/caches/osx_x86-64_cmprssptrs.cmake

dnakamura · 2020-11-26T19:33:41Z

runtime/CMakeLists.txt

+# version in gc_glue_java, below, when appropriate.
+j9vm_add_library(${OMR_GC_GLUE_TARGET} INTERFACE)
+j9vm_add_library(${OMR_GC_GLUE_FULL_TARGET} INTERFACE)
+


is there a reason to use a forward declaration instead of just including gc_glue_java ?

Yes, it's because the _full version of the glue library is only built if OMR_MIXED_REFERENCES_MODE_STATIC=ON, but OMR_MIXED_REFERENCES_MODE_STATIC is set as a cache variable later than when the glue libraries are referenced in other libraries. This results in having to run make all twice to get the build working, since on the second run, the cache variable is already set.

Like the comment says:

# Declare the GC glue interface libraries that omr might reference, # including the 'full' version. We'll only add sources to the 'full' # version in gc_glue_java, below, when appropriate.

we could just remove the conditional around gc_glue_java full. Interface libraries are never built on their own. So we can just define it but never reference it anywhere

The point is that it must be declared before including the omr.

I understand. My point is that you could just flush out both interfaces regardless. If you never link against the extra glue, it has no effect on the build system. With either solution, if for some reason you do link against the extra glue (with OMR_MIXED_REFERENCES_MODE_STATIC not set) you would get a link error at compile time.

I think you have it backward: if we (unconditionally) provide complete library definitions, there would be no link error if the extra glue library were used (unintentionally).

How do we want to proceed?

I'm satisfied with this in it's current form, but I think this should wait until eclipse/omr#5657 is merged in case it evolves in a way that would alter how this handles OMR_GC_POINTER_MODE == "mixed".

Sounds good to me. @dnakamura What are your thoughts?

sharon-wang · 2020-12-01T21:41:29Z

Here's a list of all the jenkins shortnames to run mixed refs CMake builds. These will run builds with extra configure options --with-mixedrefs and --with-cmake.

aixmxdcm: ppc64_aix_mixed_cm
zlinuxmxdcm: s390x_linux_mixed_cm
plinuxmxdcm: ppc64le_linux_mixed_cm
xlinuxmxdcm: x86-64_linux_mixed_cm
winmxdcm: x86-64_windows_mixed_cm
osxmxdcm: x86-64_mac_mixed_cm
alinux64mxdcm: aarch64_linux_mixed_cm
~~zosmxdcm: s390x_zos_mixed_cm~~ // CMake is not currently available for ZOS

keithc-ca · 2020-12-04T14:28:33Z

Jenkins test sanity zlinuxmxdcm jdk11

keithc-ca · 2020-12-04T15:56:08Z

Testing is currently only available on x86-64_linux_mixed; to be remedied, at least in part, by adoptium/aqa-tests#2087.

keithc-ca · 2020-12-04T15:56:29Z

Jenkins test sanity xlinuxmxdcm jdk11

sharon-wang · 2020-12-04T16:19:43Z

The test is still failing because in this PR, only x86-64_linux_mixed_cm is exposed.

I can add x86-64_linux_mixed (and the equivalents for the other platforms), which will use CMake by default for JDK11 builds, or should we update the AdoptOpenJDK platform specs to use the <PLATFORM>_mixed_cm versions instead of <PLATFORM>_mixed?

keithc-ca · 2020-12-04T16:39:37Z

Let's wait to see how the xlinux tests fair before adjusting anything else.

llxia · 2020-12-04T16:40:19Z

Test build failed because of Cannot find key PLATFORM: s390x_linux_mixed. Currently, we only added x86-64_linux_mixed, so the above xlinux PR build should work.

I think by default, the suffix _cm and _uma will be removed. That is how the cm test build works now. So there is no change needed for the spec name.

sharon-wang · 2020-12-04T17:34:47Z

Failure message from test build for xlinux

*** DETECTED_SPEC value is linux_x86-64_cmprssptrs, settled SPEC value is linux_x86-64_mxdptrs. SPEC value does not match. Please reset or unset SPEC.

It seems that some additional handling for mixed builds is missing from: https://github.com/AdoptOpenJDK/TKG/blob/master/src/org/openj9/envInfo/JavaInfo.java?

I think these lines end up executing, which is causing the DETECTED_SPEC to be compressed.

        if (cmprssptrs != null && cmprssptrs.contains("Compressed References")) {
            spec += "_cmprssptrs";
        }

sharon-wang · 2020-12-04T17:39:57Z

Ah, I see there's a PR open to address handling mixed refs: adoptium/TKG#124

keithc-ca · 2020-12-04T17:41:16Z

I'm inclined to merge this even though the test story is incomplete.

sharon-wang force-pushed the mixedbuild_cmake branch from 245b43a to 05e6d8f Compare November 12, 2020 19:09

This was referenced Nov 12, 2020

Add support for mixed references mode in CMake builds eclipse/omr#5657

Merged

Add new configure option --with-mixedrefs for mixed pointer mode ibmruntimes/openj9-openjdk-jdk11#359

Merged

dmitripivkine reviewed Nov 12, 2020

View reviewed changes

runtime/gc_modron_startup/mminit.cpp Show resolved Hide resolved

dmitripivkine added comp:doc comp:gc comp:vm labels Nov 12, 2020

dmitripivkine requested review from gacholio and keithc-ca November 12, 2020 19:46

keithc-ca requested changes Nov 12, 2020

View reviewed changes

sharon-wang mentioned this pull request Nov 12, 2020

Clean up extra ifdefs in runtime/include/j9lib.h.in #11174

Closed