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

Add support for mixed references mode in CMake builds #5657

Merged
merged 2 commits into from
Dec 3, 2020

Conversation

sharon-wang
Copy link
Contributor

@sharon-wang sharon-wang commented Nov 5, 2020

These are the CMake GC changes needed to enable mixed references mode for eclipse-openj9/openj9#8878.

OpenJ9 changes: eclipse-openj9/openj9#11166

Set _compressObjectReferences if mixed build override not defined
_compressObjectReferences should only be defined if OMR_OVERRIDE_COMPRESS_OBJECT_REFERENCES is not defined. Otherwise, when mixed mode is enabled and the override is defined, _compressObjectReferences is considered a private field that is declared, but never used, which the compiler may complain about.

Add support for mixed references mode in CMake builds
In mixed references static mode, split the omr gc library into omrgc and omrgc_full so that the appropriate library can be used. omrgc has OMR_OVERRIDE_COMPRESS_OBJECT_REFERENCES=1 and omrgc_full has OMR_OVERRIDE_COMPRESS_OBJECT_REFERENCES=0.

Note that this will only work with an OpenJ9 CMake build. OpenJ9 UMA changes will be implemented after the CMake work is completed and the corresponding OMR changes will be provided in an upcoming PR.


Something I'm not entirely sure about is how we'd like to work with OMR_GC_LIB in tests and in omrvmstartup. In these changes, we use omrgc in compressed and full mode, but in mixed mode, we have a separate library omrgc_full for full references.

Files currently using OMR_GC_LIB:
https://github.com/eclipse/omr/blob/f8c4f85b726259de9286943d84f7151aed7eb1c1/example/CMakeLists.txt#L49

  • do we want to have gcexample and gcexample_full?

https://github.com/eclipse/omr/blob/f8c4f85b726259de9286943d84f7151aed7eb1c1/fvtest/gctest/CMakeLists.txt#L51

  • do we want to have omrgctest and omrgctest_full (this test also uses omrvmstartup, mentioned below)

https://github.com/eclipse/omr/blob/f8c4f85b726259de9286943d84f7151aed7eb1c1/omr/CMakeLists.txt#L74

  • this is just for include directories, so either lib will do?

https://github.com/eclipse/omr/blob/f8c4f85b726259de9286943d84f7151aed7eb1c1/omr/startup/CMakeLists.txt#L50

  • omrvmstartup is used in a bunch of places (i.e. fvtest), so if we want omrvmstartup and omrvmstartup_full, then all those tests will need to get split into regular (compressed) and full references versions

@sharon-wang
Copy link
Contributor Author

Please review: @gacholio @dnakamura

cmake/config.cmake Outdated Show resolved Hide resolved
Copy link
Contributor

@gacholio gacholio left a comment

Choose a reason for hiding this comment

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

I'll defer to @keithc-ca and @dnakamura for the cmake changes. The code changes look good.

@gacholio
Copy link
Contributor

gacholio commented Nov 9, 2020

Also, ideally, the library name will be j9gc in the full pointers-only case (which keeps the current builds working without more changes).

@sharon-wang
Copy link
Contributor Author

sharon-wang commented Nov 9, 2020

So, we only want to use the new name for the full library (j9gc_full/omrgc_full) when we're building in mixed mode; otherwise use the default name for the library (j9gc/omrgc) in compressed-only and full-only?

@gacholio
Copy link
Contributor

gacholio commented Nov 9, 2020

That would be ideal since it keeps the current builds the same (otherwise, DDR parser changes will need to be introduced to handle the new library name).

@gacholio
Copy link
Contributor

gacholio commented Nov 9, 2020

My comments apply to the final shared libraries built in OpenJ9. The names of the static link libraries in OMR don't matter as much, though being consistent never hurts.

@sharon-wang
Copy link
Contributor Author

The fvtests work for full or compressed references mode individually, but in mixed references mode, I think we'd need a separate "test suite" for full references that uses the omrgc_full library.

Would it be reasonable/necessary to create a separate test suite just for running fvtests for full references in mixed mode, or is it sufficient that the tests can only be run with a non-mixed build?

@gacholio
Copy link
Contributor

I never updated the OMR example GC to work properly in mixed mode, so I expect many tests to fail.

@sharon-wang sharon-wang changed the title Build omrgc and omrgc_full separately to enable mixed references mode WIP: Build omrgc and omrgc_full separately to enable mixed references mode Nov 11, 2020
@sharon-wang sharon-wang changed the title WIP: Build omrgc and omrgc_full separately to enable mixed references mode Add support for mixed references mode in CMake builds Nov 12, 2020
`_compressObjectReferences` should only be defined if
`OMR_OVERRIDE_COMPRESS_OBJECT_REFERENCES` is not defined.
Otherwise, when mixed mode is enabled and the override is defined,
`_compressObjectReferences` is considered a private field that is
declared, but never used, which the compiler may complain about.

Signed-off-by: Sharon Wang <sharon-wang-cpsc@outlook.com>
@sharon-wang
Copy link
Contributor Author

I never updated the OMR example GC to work properly in mixed mode, so I expect many tests to fail.

Do we intend to have the example code work with mixed mode in this PR?

@gacholio
Copy link
Contributor

Do we intend to have the example code work with mixed mode in this PR?

I would say no, but it does mean that there can be no testing of mixed mode in OMR (which I'm personally fine with in the short term).

Unless there's strong objection, let's get this going for OpenJ9 then come back to the example later on.

In mixed references static mode, split the omr gc library into omrgc
and omrgc_full so that the appropriate library can be used.

Only set OMR_OVERRIDE_COMPRESS_OBJECT_REFERENCES in mixed references
static mode.

Signed-off-by: Sharon Wang <sharon-wang-cpsc@outlook.com>
@sharon-wang
Copy link
Contributor Author

Just a ping to @keithc-ca @dnakamura for review/approval on the CMake changes

@keithc-ca
Copy link
Contributor

This looks good to me. I have tested it with eclipse-openj9/openj9#11166 in both cmake builds configured for compressed references and cmake builds configured for static mixed references.

I think there should be plans to simplify the handling of references. Back when the only choices were compressed or not, OMR_GC_COMPRESSED_POINTERS was a reasonable flag. We've since added OMR_GC_FULL_POINTERS and OMR_OVERRIDE_COMPRESS_OBJECT_REFERENCES and, in my opinion, it's touch to juggle all the combinations in one's head. I think it would be clearer if there was one macro, say, OMR_GC_POINTER_MODE, which would be configured as one of the following:

  • OMR_GC_POINTER_MODE_FULL - no compression (the only choice for 32-bit environments)
  • OMR_GC_POINTER_MODE_COMPRESSED - compressed pointers (what OMR_GC_COMPRESSED_POINTERS used to mean)
  • OMR_GC_POINTER_MODE_MIXED_STATIC - both compressed and non-compressed references supported by compiling certain code twice
  • OMR_GC_POINTER_MODE_MIXED_DYNAMIC - both compressed and non-compressed references supported via dynamic runtime tests

See also ibmruntimes/openj9-openjdk-jdk11#361.

@sharon-wang
Copy link
Contributor Author

Thanks for the reviews everyone! I think this is in a good point to merge, unless there are any further comments?

I can open an new issue for consolidating the pointer flag to align with the plan in ibmruntimes/openj9-openjdk-jdk11#361. I agree @keithc-ca it would reduce complexity with all the different modes.

@fjeremic fjeremic self-assigned this Dec 3, 2020
@fjeremic
Copy link
Contributor

fjeremic commented Dec 3, 2020

@genie-omr build all

@sharon-wang
Copy link
Contributor Author

osx failure appears to be a machine issue -- all tests passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants