-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
8281455: Change JVM options with small ranges from 64 to 32 bits, for gc_globals.hpp #14259
Conversation
👋 Welcome back azafari! A progress list of the required criteria for merging this PR into |
@afshin-zafari The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Although it's a bit early to review, when I looked at this briefly, I believe also the |
Done. |
Webrevs
|
Wouldn't that be good to keep the alignment for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More candidates:
ParGCArrayScanChunk
MarkStackSize(Max) (these should probably be a size_t
)
VerifyArchivedFields
ScavengeALotInterval
FullGCALotInterval
FullGCALotStart
FullGCALotDummies
VerifyGCLevel
MaxTenuringThreshold
InitialTenuringThreshold
GCDrainStackTargetSize
Some of these aren't uintx->uint conversions, so feel free to ignore.
@@ -349,7 +349,7 @@ void PSAdaptiveSizePolicy::compute_eden_space_size( | |||
log_debug(gc, ergo)( | |||
"PSAdaptiveSizePolicy::compute_eden_space_size: gc time limit" | |||
" gc_cost: %f " | |||
" GCTimeLimit: " UINTX_FORMAT, | |||
" GCTimeLimit: " UINT32_FORMAT, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use %u
as format specifiers for uint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
%u
is used.
@@ -91,11 +91,11 @@ void GCOverheadChecker::check_gc_overhead_limit(GCOverheadTester* time_overhead, | |||
|
|||
if (UseGCOverheadLimit) { | |||
if (gc_overhead_limit_exceeded()) { | |||
log_trace(gc, ergo)("GC is exceeding overhead limit of " UINTX_FORMAT "%%", GCTimeLimit); | |||
log_trace(gc, ergo)("GC is exceeding overhead limit of %d%%", GCTimeLimit); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why this place uses %d
as format specifier compared to the UINT32_FORMAT
the previous places did. Use %u
;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
%u
is used.
@@ -166,7 +166,7 @@ | |||
"A System.gc() request invokes a concurrent collection; " \ | |||
"(effective only when using concurrent collectors)") \ | |||
\ | |||
product(uintx, GCLockerEdenExpansionPercent, 5, \ | |||
product(uint, GCLockerEdenExpansionPercent, 5, \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The \
needs to be aligned back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
// Table of values: | ||
// sizeof intx 4 8 | ||
// min_intx 0x80000000 0x8000000000000000 | ||
// max_intx 0x7FFFFFFF 0x7FFFFFFFFFFFFFFF | ||
// max_uintx 0xFFFFFFFF 0xFFFFFFFFFFFFFFFF | ||
|
||
typedef unsigned int uint; NEEDS_CLEANUP | ||
const uint max_uint = (uint)-1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change should use the existing UINT_MAX
instead of adding a new constant. Instead of a cast of -1
, one should also use ~0
to get all-ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UINT_MAX
is used. New definition of max_uint is removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested changes in Comments are applied including the 'more candidates'.
@@ -229,7 +229,7 @@ void G1Arguments::initialize() { | |||
|
|||
// By default do not let the target stack size to be more than 1/4 of the entries | |||
if (FLAG_IS_DEFAULT(GCDrainStackTargetSize)) { | |||
FLAG_SET_ERGO(GCDrainStackTargetSize, MIN2(GCDrainStackTargetSize, (uintx)TASKQUEUE_SIZE / 4)); | |||
FLAG_SET_ERGO(GCDrainStackTargetSize, MIN2(GCDrainStackTargetSize, (uint)TASKQUEUE_SIZE / 4)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TASKQUEUE_SIZE should be a constant of type size_t rather than a macro. GCDrainStackTargetSize should be of type size_t. Then we don't need any casts here. But some uses may need cleaning up, either to remove casts or to adjust surrounding types. I suggest this be split out as a separate RFE to avoid cluttering the simplicity of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change is applied and some cleanup made. Maybe no need to a new RFE, if you agree.
@@ -47,7 +47,7 @@ JVMFlag::Error InitialTenuringThresholdConstraintFuncParallel(uintx value, bool | |||
if (UseParallelGC && (value > MaxTenuringThreshold)) { | |||
JVMFlag::printError(verbose, | |||
"InitialTenuringThreshold (" UINTX_FORMAT ") must be " | |||
"less than or equal to MaxTenuringThreshold (" UINTX_FORMAT ")\n", | |||
"less than or equal to MaxTenuringThreshold (%u)\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(github UI won't let me attach comment to the function signature)
The type of the value parameter should be changed in conjunction with the type change for the option,
both here and in jvmFlagConstraintsParallel.hpp. Similarly for MaxTenuringThresholdXXX below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@@ -553,7 +553,7 @@ bool PSScavenge::invoke_no_policy() { | |||
_tenuring_threshold, | |||
survivor_limit); | |||
|
|||
log_debug(gc, age)("Desired survivor size " SIZE_FORMAT " bytes, new threshold %u (max threshold " UINTX_FORMAT ")", | |||
log_debug(gc, age)("Desired survivor size " SIZE_FORMAT " bytes, new threshold %u (max threshold %u)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider also, instead of SIZE_FORMAT we can use "%zu".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
%zu
is used instead.
@@ -99,7 +99,7 @@ uint AgeTable::compute_tenuring_threshold(size_t desired_survivor_size) { | |||
} | |||
|
|||
|
|||
log_debug(gc, age)("Desired survivor size " SIZE_FORMAT " bytes, new threshold " UINTX_FORMAT " (max threshold " UINTX_FORMAT ")", | |||
log_debug(gc, age)("Desired survivor size " SIZE_FORMAT " bytes, new threshold " UINTX_FORMAT " (max threshold %u)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider also, instead of SIZE_FORMAT we can use "%zu".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
%zu
is used.
@@ -431,6 +431,7 @@ const intx min_intx = (intx)1 << (sizeof(intx)*BitsPerByte-1); | |||
const intx max_intx = (uintx)min_intx - 1; | |||
const uintx max_uintx = (uintx)-1; | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stray blank line insertion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@@ -439,7 +440,6 @@ const uintx max_uintx = (uintx)-1; | |||
|
|||
typedef unsigned int uint; NEEDS_CLEANUP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated, but I wonder why this "NEEDS_CLEANUP"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, it is because the uint
is already defined within the guarded lines 149-154:
#ifdef __USE_MISC
/* Old compatibility names for C types. */
typedef unsigned long int ulong;
typedef unsigned short int ushort;
typedef unsigned int uint;
#endif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not seeing such a block? Neither in the current version nor in several recent versions. And I didn't
find __USE_MISC
anywhere in the JDK. I did find what it's for and where it's supposed to come from:
https://stackoverflow.com/questions/10231885/what-is-use-misc-macro-used-for
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was curious and found it in /usr/include/x86_64-linux-gnu/sys/types.h on ubuntu. I guess it needs cleanup because it might not be defined with USE_MISC? There are too many int type names to choose from for int. Why does it say "Old compatibility names for C types."? Should we even be using uint ? I don't see it in https://en.cppreference.com/w/cpp/language/types - should we prefer "unsigned"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This NEEDS_CLEANUP block shouldn't be part of this change.
There are a lot of uses of the options that have casts, many of which ought to be removable with this change. |
During this change, I changed all the cases that I found them to be changed. Would you please, give some examples or some 'search criteria' that I can use to find the cases. Then, I will do them here in this PR or create a new RFE for them. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The review comments are applied as stated.
The type of GCDrainStackTargetSize
is changed and corresponding casts are also modified. Not so many changes were they. They can be excluded from this PR if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a good change to me to avoid Wconversion warnings for mis-sized argument values but I'll leave it up to the GC experts.
@@ -526,7 +526,7 @@ void PSAdaptiveSizePolicy::compute_old_gen_free_space( | |||
log_debug(gc, ergo)( | |||
"PSAdaptiveSizePolicy::compute_old_gen_free_space: gc time limit" | |||
" gc_cost: %f " | |||
" GCTimeLimit: " UINTX_FORMAT, | |||
" GCTimeLimit: " UINT32_FORMAT, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be %u also?
reset_gc_overhead_limit_count(); | ||
} else if (print_gc_overhead_limit_would_be_exceeded) { | ||
assert(_gc_overhead_limit_count > 0, "Should not be printing"); | ||
log_trace(gc, ergo)("GC would exceed overhead limit of " UINTX_FORMAT "%% %d consecutive time(s)", | ||
log_trace(gc, ergo)("GC would exceed overhead limit of %u %% %d consecutive time(s)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An extra space has been added before "%%"; please remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@@ -439,7 +440,6 @@ const uintx max_uintx = (uintx)-1; | |||
|
|||
typedef unsigned int uint; NEEDS_CLEANUP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not seeing such a block? Neither in the current version nor in several recent versions. And I didn't
find __USE_MISC
anywhere in the JDK. I did find what it's for and where it's supposed to come from:
https://stackoverflow.com/questions/10231885/what-is-use-misc-macro-used-for
@@ -2308,7 +2308,7 @@ void G1CMTask::drain_local_queue(bool partially) { | |||
// of things to do) or totally (at the very end). | |||
size_t target_size; | |||
if (partially) { | |||
target_size = MIN2((size_t)_task_queue->max_elems()/3, (size_t)GCDrainStackTargetSize); | |||
target_size = MIN2((size_t)_task_queue->max_elems()/3, GCDrainStackTargetSize); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was hoping max_elems() wouldn't need a cast. Fixing that seems like it might be one of those long
and more complicated threads I was referring to earlier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The max_elems()
return uint
and the cast is needed.
@@ -212,12 +212,12 @@ | |||
/* where does the range max value of (max_jint - 1) come from? */ \ | |||
product(size_t, MarkStackSizeMax, NOT_LP64(4*M) LP64_ONLY(512*M), \ | |||
"Maximum size of marking stack") \ | |||
range(1, (max_jint - 1)) \ | |||
range(1, (SIZE_MAX - 1)) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a large increase in the max size. Not at all sure it's appropriate. Similarly for MarkStackSize below.
"Number of entries we will try to leave on the stack " \ | ||
"during parallel gc") \ | ||
range(0, max_juint) \ | ||
range(0, SIZE_MAX) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a large increase in the max on 64bit platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you should revert the change from max_juint to SIZE_MAX. Is the change to size_t a cleanup to remove a cast?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted back to original.
I spotted some when looking earlier, but haven't re-found any of them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few comments. Narrowing the types of these arguments was to prevent Wconversion warnings where they're used and then later narrowed to 32 bit ints. From my sampling of some of the arguments changed, there were no casts, which was the problem. Some of these (sample GCTimeLimit) are promoted back up to uintx but having the field be the narrowest representation allows the code to use it without an implicit conversion to a narrower int type. So I think this is a good step for cleaning up the int types.
"How much buffer to keep for pause time") \ | ||
range(0, max_juint) \ | ||
range(0, UINT_MAX) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you change from max_juint -> UINT_MAX ? max_juint should have compile without integer size mismatch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or, not in this change, but should we prefer UINT_MAX (found in /usr/include/limits.h) to our globalDefinitions.hpp definitions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I see Thomas's comments about this. I guess changing a couple at a time would be ok.
"Number of entries we will try to leave on the stack " \ | ||
"during parallel gc") \ | ||
range(0, max_juint) \ | ||
range(0, SIZE_MAX) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you should revert the change from max_juint to SIZE_MAX. Is the change to size_t a cleanup to remove a cast?
@@ -439,7 +440,6 @@ const uintx max_uintx = (uintx)-1; | |||
|
|||
typedef unsigned int uint; NEEDS_CLEANUP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was curious and found it in /usr/include/x86_64-linux-gnu/sys/types.h on ubuntu. I guess it needs cleanup because it might not be defined with USE_MISC? There are too many int type names to choose from for int. Why does it say "Old compatibility names for C types."? Should we even be using uint ? I don't see it in https://en.cppreference.com/w/cpp/language/types - should we prefer "unsigned"?
/label add hotspot-runtime |
@coleenp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm, it's a step forward to cleaning up the gc_globals.hpp types/ranges.
@afshin-zafari 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 461 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good and definitely a step forward.
Thank you Coleen, Kim, Thomas and Amit for your comments and reviewing this PR. |
Going to push as commit 7ce967a.
Your commit was automatically rebased without conflicts. |
@afshin-zafari Pushed as commit 7ce967a. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
The
uintx/intx
options whose ranges are small enough are changed touint/int
, otherwise gcc complainswhen
-Wconversion
is used in build.Their uses in printf formats are changed accordingly.
Tests
Locally hotspot:tier1 tested on linux-x64
mach5 tiers 1-4 on Linux and Windows 64bits platforms passed.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14259/head:pull/14259
$ git checkout pull/14259
Update a local copy of the PR:
$ git checkout pull/14259
$ git pull https://git.openjdk.org/jdk.git pull/14259/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 14259
View PR using the GUI difftool:
$ git pr show -t 14259
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14259.diff
Webrev
Link to Webrev Comment