JDK-8302989: Add missing INCLUDE_CDS checks #12691
Conversation
|
👋 Welcome back mbaesken! A progress list of the required criteria for merging this PR into |
Webrevs
|
|
The CDS related source code is not 100% compile-time conditionalized. Typically we only exclude code that can't compile without the CDS specific files being compiled. Othercode won't execute due to the value of the "flags" in those cases. If you want to 100% conditionalize CDS related code then you need more changes than just those outlined here. Are you actually trying to solve a problem or was this just an observation? |
Hi David, this was about getting less code compiled into the libjvm in case cds is disabled by configure. |
|
@MBaesken This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 538 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
How much space in libjvm does this save? There is a trade-off with code readability when doing this. If the aim is to get 100% conditional code then you need to do more. Do plan to do that or just this small tweak? |
Hi David, I would not use the macro with one - liners , I think this is not worth the effort. So no goal to get to 100 %.
I wasn't sure about the DumpSharedSpaces sections, usually the UseSharedSpaces are guarded with a few exceptions like those addressed in this change. |
|
So you avoid doing e.g. and but what about the code in I'm just not seeing that converting a handful of sites is really beneficial. The flags themselves are not normal flags defined for CDS_ONLY precisely because we wanted them available without having to conditionalize every single usage. I'm not looking to "block" this but I can't really endorse it either. *** there is probably potential for adding a |
Hi David, systemDictionaryShared.cpp is already not compiled in non-CDS mode, see hotspot/lib/JvmFeatures.gmk .
The conversion of a handful of sites is just an addition to the already rather common usage of the macro. Nothin new really ...
Maybe in a separate issue, that do you think ? Do you have some example places where to use it ? |
Sorry I must have misread something.
Nothing new but it doesn't really add anything either; especially when there remains so much non-conditional usage.
Up to you. These are what I spotted mainly: But looking closer these are all in constructors that should be CDS_ONLY. |
|
It may be more profitable (and less work) if we change these variables to But some changes may be needed in SA. |
Hi Ioi, do you think this should be done for all 4 bools ? |
I think it should be done for all four bools. UseSharedSpaces is accessed by SA in src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/VM.java. I would suggest adding this: Then we can change globalDefinitions.cpp to something like: You may need to test with a build that has SA enabled but CDS disabled. I am not sure if this is a supported combination. |
| extern "C" { | ||
| // Make sure UseSharedSpaces is accessible to the serviceability agent. | ||
| extern JNIEXPORT jboolean UseSharedSpaces; | ||
| } | ||
| #else | ||
| // in non CDS mode do not export it | ||
| extern const bool UseSharedSpaces; |
There was a problem hiding this comment.
How about this?
const bool RequireSharedSpaces = false;
This will allow the C++ compiler to eliminate all code that depend on this value when INCLUDE_CDS is false.
There was a problem hiding this comment.
Hi Ioi, I set this already some lines above (in the non-INCLUDE_CDS case).
There was a problem hiding this comment.
You need to remove the extern keyword, and add the definition of = false in globalDefinitions.hpp.
Then, the corresponding definition should be removed from globalDefinitions.cpp.
There was a problem hiding this comment.
Hi Ioi, I adjusted the *SharedSpaces in globalDefinitions.cpp / globalDefinitions.hpp .
There was a problem hiding this comment.
I am on vacation now so I won’t be able to follow up with the review. Please feel free to integrate after you get enough reviews.
Just a general comment. For blocks that are enclosed with “if (UseSharedSpace) {…}”, there’s no need to put them inside #if as these blocks will be elided by the C++ compiler. That way the code will be less messy.
There was a problem hiding this comment.
Hi Ioi, thanks for the feedback. Mostly you are correct, the #if blocks are not needed any more at most places. Should I remove them at least from my patch now or keep them? But I think there are a few exceptions - at some places the coding attempts to set the *SharedSpaces flags and this does not compile any more after switching to const. So there the #if blocks have to stay.
| #if INCLUDE_CDS | ||
| UseSharedSpaces = false; | ||
| #endif |
There was a problem hiding this comment.
This doesn't make sense - the entire no_shared_spaces method is only meaningful on a JVM with CDS. Otherwise -Xshare:on should immediately be rejected. ??
There was a problem hiding this comment.
Hi David, unfortunately this has to stay for now, otherwise we would try to set a const in the CDS-disabled build, this does not compile.
Maybe it would make sense to guard the whole function no_shared_spaces and adjust the calls/usages but I would prefer a separate issue for this.
There was a problem hiding this comment.
AFAICS all calls to no_shared_spaces are already within INCLUDE_CDS regions so the function itself can also be INCLUDE_CDS only.
| } else if (match_option(option, "-Xshare:dump")) { | ||
| #if INCLUDE_CDS | ||
| DumpSharedSpaces = true; | ||
| #endif | ||
| // -Xshare:on | ||
| } else if (match_option(option, "-Xshare:on")) { | ||
| #if INCLUDE_CDS | ||
| UseSharedSpaces = true; | ||
| RequireSharedSpaces = true; | ||
| #endif | ||
| // -Xshare:auto || -XX:ArchiveClassesAtExit=<archive file> | ||
| } else if (match_option(option, "-Xshare:auto")) { | ||
| #if INCLUDE_CDS | ||
| UseSharedSpaces = true; | ||
| RequireSharedSpaces = false; | ||
| #endif | ||
| xshare_auto_cmd_line = true; | ||
| // -Xshare:off | ||
| } else if (match_option(option, "-Xshare:off")) { |
There was a problem hiding this comment.
These flags should all immediately be rejected if used in a non-CDS build. Yes this is pre-existing but it now looks totally broken with the new changes. There should be something like:
#ifndef INCLUDE_CDS
} else if (match_option(option, "-Xshare")) { // or whatever we need to match the prefix only
warning("Option %s is not supported in this VM", option);
#else
// process flags as usual
#endif
There was a problem hiding this comment.
Hi David, I adjusted the coding following your request (however we need to use #if not #ifndef here).
|
Unfortunately with the latest revision of the patch, quite a few tests fail when the build is configured with Failing tests (there might be a few more where the reason is not fully clear from the log) sun/security/provider/X509Factory/BigCRL.java java/lang/module/ModuleDescriptorHashCodeTest.java java/math/BigInteger/largeMemory/DivisionOverflow.java java/math/BigInteger/largeMemory/StringConstructorOverflow.java runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java#id7 runtime/7158988/FieldMonitor.java runtime/verifier/TraceClassRes.java compiler/intrinsics/klass/TestIsPrimitive.java runtime/logging/ModulesTest.java runtime/CompressedOops/CompressedClassPointers.java So should we better keep the handling of |
|
Maybe we can keep the -Xshare flags and make their behavior consistent with builds that have CDS enabled: -Xshare:off - do nothing |
Yes that is to be expected. The tests have not be written to allow them to be run for arbitrary selection of the INCLUDE_XXX features. You will also have to exclude running all the CDS tests on such a build. Arguably any tests that set the -Xshare flag depend on CDS (positvely or negatively) and so should use appropriate @requires or runtime checks before using the flag.
This might help the tests but semantically this just seems wrong to me. It would be like accepting G1 flags when you only have SerialGC available. I think we are just demonstrating that fully conditionalizing CDS code requires some higher-level semantic issues to be dealt with and that it is probably not worth trying to do that. So that brings us back to whether it is worth even trying to make a small step from where we are towards more complete conditionalization? Seems an arbitrary line to set to me when the benefit is minimal. But if you want to revert to an earlier version of the patch that "works" then I won't object further. |
|
The disabling/enabling of jvm features is briefly mentioned in the build doc ( https://openjdk.org/groups/build/doc/building.html ) so I thought this is something that is supported and should work (maybe not perfect because of limited testing) . If it is really not so interesting to others to adjust tests with |
|
I noticed that the Xshare-handling IS already an issue on AIX - there cds is not supported and java/math/BigInteger/largeMemory/DivisionOverflow.java all fail on AIX (like on other platforms without cds). |
The following is the current behavior when |
|
So considering the low interest in adding the if INCLUDE_CDS checks in the codebase, there is still the fact that |
|
I'm a bit confused about the testing situation as you indicated |
|
@MBaesken This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
|
@MBaesken This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the |
The cds only coding in hotspot is usually guarded with the INCLUDE_CDS macro so that it can be removed at compile time in case the correct configure flags are set.
However at some places INCLUDE_CDS is missing and should be added.
One question - should (additionally to the UseSharedSpaces code section) the DumpSharedSpaces code sections be guarded as well with INCLUDE_CDS macros ?
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/12691/head:pull/12691$ git checkout pull/12691Update a local copy of the PR:
$ git checkout pull/12691$ git pull https://git.openjdk.org/jdk.git pull/12691/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 12691View PR using the GUI difftool:
$ git pr show -t 12691Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12691.diff
Webrev
Link to Webrev Comment