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.

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

Jump to bottom

8259349: -XX:AvgMonitorsPerThreadEstimate=1 does not work right #1992

Closed
wants to merge 8 commits into from
Closed

8259349: -XX:AvgMonitorsPerThreadEstimate=1 does not work right #1992

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d4acfcf
8259397: ThreadsSMRSupport::print_info_on() should use try_lock_witho…
dcubed-ojdk Jan 7, 2021
0fe88c0
8259349: -XX:AvgMonitorsPerThreadEstimate=1 does not work right
dcubed-ojdk Jan 7, 2021
5b57bd9
Fix build error on windows.
dcubed-ojdk Jan 8, 2021
42343e8
Merge branch 'master' into JDK-8259349
dcubed-ojdk Jan 8, 2021
e68a5e5
dholmes CR - change AvgMonitorsPerThreadEstimate from 'intx' to 'int'…
dcubed-ojdk Jan 8, 2021
f0af3c7
delete bad assert found by linux build.
dcubed-ojdk Jan 8, 2021
1fa734f
dholmes CR - delete a stale comment.
dcubed-ojdk Jan 8, 2021
7737506
change AvgMonitorsPerThreadEstimate from int to size_t and _in_use_li…
dcubed-ojdk Jan 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 1 addition & 3 deletions src/hotspot/share/runtime/globals.hpp
View file
Open in desktop
Expand Up @@ -715,9 +715,7 @@ const intx ObjectAlignmentInBytes = 8;
"MonitorUsedDeflationThreshold is exceeded (0 is off).") \
range(0, max_jint) \
\
/* notice: the max range value here is max_jint, not max_intx */ \
/* because of overflow issue */ \
product(intx, AvgMonitorsPerThreadEstimate, 1024, DIAGNOSTIC, \
product(int, AvgMonitorsPerThreadEstimate, 1024, DIAGNOSTIC, \
"Used to estimate a variable ceiling based on number of threads " \
"for use with MonitorUsedDeflationThreshold (0 is off).") \
range(0, max_jint) \
Expand Down
20 changes: 12 additions & 8 deletions src/hotspot/share/runtime/synchronizer.cpp
View file
Open in desktop
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1998, 2020, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1998, 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -235,6 +235,8 @@ void ObjectSynchronizer::initialize() {
for (int i = 0; i < NINFLATIONLOCKS; i++) {
gInflationLocks[i] = new os::PlatformMutex();
}
// Start the ceiling with the estimate for one thread.
set_in_use_list_ceiling(AvgMonitorsPerThreadEstimate);
}

static MonitorList _in_use_list;
Expand All @@ -249,10 +251,11 @@ static MonitorList _in_use_list;
// of the thread count derived ceiling because we have used more
// ObjectMonitors than the estimated average.
//
// Start the ceiling with the estimate for one thread.
// Start the ceiling with the estimate for one thread in initialize()
// which is called after cmd line options are processed.
// This is a 'jint' because the range of AvgMonitorsPerThreadEstimate
// is 0..max_jint:
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this comment now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. The range for AvgMonitorsPerThreadEstimate is still 0..max_jint.

Copy link
Contributor

Choose a reason for hiding this comment

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

what would happen if this option was set to max_jint? Is it a reasonable max?

static jint _in_use_list_ceiling = AvgMonitorsPerThreadEstimate;
static jint _in_use_list_ceiling = 0;
bool volatile ObjectSynchronizer::_is_async_deflation_requested = false;
bool volatile ObjectSynchronizer::_is_final_audit = false;
jlong ObjectSynchronizer::_last_async_deflation_time_ns = 0;
Expand Down Expand Up @@ -1166,17 +1169,18 @@ size_t ObjectSynchronizer::in_use_list_ceiling() {

void ObjectSynchronizer::dec_in_use_list_ceiling() {
Atomic::add(&_in_use_list_ceiling, (jint)-AvgMonitorsPerThreadEstimate);
#ifdef ASSERT
size_t l_in_use_list_ceiling = in_use_list_ceiling();
#endif
assert(l_in_use_list_ceiling > 0, "in_use_list_ceiling=" SIZE_FORMAT
": must be > 0", l_in_use_list_ceiling);
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you find that this can go negative? I can see that it could go to zero at shutdown maybe.

Copy link
Member Author

Choose a reason for hiding this comment

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

In a previous round I changed the assert to:
assert(l_in_use_list_ceiling >= 0, ...
and the Linux build complained about the assert always being true.
This is because size_t is unsigned. (No complaint on macOSX.)

Copy link
Member Author

Choose a reason for hiding this comment

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

what would happen if this option was set to max_jint? Is it a reasonable max?

The _in_use_list_ceiling would be set to a very large number and we would
be allowing a very large number of monitors before a deflation cycle, i.e., we
would probably never async deflate.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, Ok, on both points.

}

void ObjectSynchronizer::inc_in_use_list_ceiling() {
Atomic::add(&_in_use_list_ceiling, (jint)AvgMonitorsPerThreadEstimate);
}

void ObjectSynchronizer::set_in_use_list_ceiling(size_t new_value) {
Copy link
Member

Choose a reason for hiding this comment

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

new_value should just be declared as int or jint now and the cast and comments removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so. For some reason @fisk used size_t for MonitorList
_count and _max fields so the ceiling that is passed in and returned
should be size_t. The only reason that the _in_use_list_ceiling is jint
is because the range is 0..max_jint. At one point I tried to change it
to size_t and ran into build issues, but I don't remember the details.

// _in_use_list_ceiling is a jint so this cast could lose precision,
// but in reality the ceiling should never get that high.
_in_use_list_ceiling = (jint)new_value;
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm sure I must have asked this before but why is AvgMonitorsPerThreadEstimate typed as intx rather than int? A jint is 32-bit and so is inton all our 32-bit and 64-bit platforms; whereasintx` is 64-bit on a 64-bit system.

And if we only ever set this field once why introduce a seemingly general use setter function instead of just doing the initialization directly in the initialize() method?

Copy link
Member Author

Choose a reason for hiding this comment

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

@dholmes-ora - Thanks for the review.

The intx type was copied from some other option when AvgMonitorsPerThreadEstimate
was introduced. I'd have to look to see why intx is used instead of int in options.

set_in_use_list_ceiling() is used in this fix and also in the follow on fix: JDK-8226416.

Copy link
Member Author

Choose a reason for hiding this comment

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

$ grep -c 'product(int,' src/hotspot/share/runtime/globals.hpp
1

$ grep -c 'product(intx,' src/hotspot/share/runtime/globals.hpp
68

Looks like we have 1 int option and 68 intx options. I'll look at changing
AvgMonitorsPerThreadEstimate from intx to int.


bool ObjectSynchronizer::is_async_deflation_needed() {
if (is_async_deflation_requested()) {
// Async deflation request.
Expand Down
3 changes: 2 additions & 1 deletion src/hotspot/share/runtime/synchronizer.hpp
View file
Open in desktop
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1998, 2020, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1998, 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -131,6 +131,7 @@ class ObjectSynchronizer : AllStatic {
static size_t in_use_list_ceiling();
static void dec_in_use_list_ceiling();
static void inc_in_use_list_ceiling();
static void set_in_use_list_ceiling(size_t new_value);
static bool is_async_deflation_needed();
static bool is_async_deflation_requested() { return _is_async_deflation_requested; }
static bool is_final_audit() { return _is_final_audit; }
Expand Down