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

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

Closed
wants to merge 8 commits into from

Conversation

dcubed-ojdk
Copy link
Member

@dcubed-ojdk dcubed-ojdk commented Jan 7, 2021

This is a trivial fix to make the "-XX:AvgMonitorsPerThreadEstimate"
option work correctly for values < 1024.

I've locally built and tested this fix on my MBP13. It will be included in my
next Mach5 Tier[1-3] testing batch.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8259349: -XX:AvgMonitorsPerThreadEstimate=1 does not work right

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1992/head:pull/1992
$ git checkout pull/1992

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 7, 2021

👋 Welcome back dcubed! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Jan 7, 2021

@dcubed-ojdk The following label will be automatically applied to this pull request:

  • hotspot-runtime

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.

@openjdk openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Jan 7, 2021
@dcubed-ojdk
Copy link
Member Author

dcubed-ojdk commented Jan 7, 2021

Note: I'm testing out stacking patches in Git so the changes to src/hotspot/share/runtime/threadSMR.cpp
are actually for https://bugs.openjdk.java.net/browse/JDK-8259397.

This is the correct commit for this PR review: 0fe88c0

@dcubed-ojdk dcubed-ojdk marked this pull request as ready for review January 7, 2021 22:45
@openjdk openjdk bot added the rfr Pull request is ready for review label Jan 7, 2021
@mlbridge
Copy link

mlbridge bot commented Jan 7, 2021

Webrevs

coleenp
coleenp approved these changes Jan 7, 2021
Copy link
Contributor

@coleenp coleenp left a comment

Choose a reason for hiding this comment

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

LGTM!

@openjdk
Copy link

openjdk bot commented Jan 7, 2021

@dcubed-ojdk 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:

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

Reviewed-by: coleenp, dholmes

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 54 new commits pushed to the master branch:

  • a6ab9e4: 8258576: Try to get zerobased CCS if heap is above 32 and CDS is disabled
  • a3561ae: 8258243: C2: assert failed ("Bad derived pointer") with -XX:+VerifyRegisterAllocator
  • 4663704: 8259583: Remove unused decode_env::_codeBuffer
  • 9d4e84f: 8259565: Zero: compiler/runtime/criticalnatives fails because CriticalJNINatives is not supported
  • 98ccfbf: 8255710: Opensource unit/regression tests for CMM
  • 61c5b95: 7194219: java/awt/Component/UpdatingBootTime/UpdatingBootTime.html fails on Linux
  • 77f6290: 8258254: Move PtrQueue flush to PtrQueueSet subclasses
  • 2255ab7: 8258810: Improve enum traits
  • ae0532e: 8259044: JVM lacks data type qualifier when using -XX:+PrintAssembly with AArch64-Neon backend
  • b378f54: Merge
  • ... and 44 more: https://git.openjdk.java.net/jdk/compare/090bd3afc37555582bde61d03e2e32350ae5d714...master

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 master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jan 7, 2021
@dcubed-ojdk
Copy link
Member Author

@coleenp - Thanks for the fast review!

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

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

Hi Dan,

The fix is fine in principle, but some comments below.

Thanks,
David

void ObjectSynchronizer::set_in_use_list_ceiling(size_t new_value) {
// _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.

…; fix bad assert tripped by the now working AvgMonitorsPerThreadEstimate option.
@dcubed-ojdk
Copy link
Member Author

@coleenp or @dholmes-ora - a re-review would be appreciated. Thanks!

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

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

Hi Dan,
Thanks for the explanations. There is more cleanup now that the type of the flag has been changed (which is why I queried that - sorry for not being clear on intent).

Thanks,
David

@@ -717,7 +717,7 @@ const intx ObjectAlignmentInBytes = 8;
\
/* notice: the max range value here is max_jint, not max_intx */ \
Copy link
Member

Choose a reason for hiding this comment

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

The comment is no longer applicable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Deleted

// 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?

}

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.

coleenp
coleenp approved these changes Jan 8, 2021
Copy link
Contributor

@coleenp coleenp left a comment

Choose a reason for hiding this comment

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

I had a couple of questions.

// This is a 'jint' because the range of AvgMonitorsPerThreadEstimate
// is 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?

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.

@mlbridge
Copy link

mlbridge bot commented Jan 9, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

On 9/01/2021 8:50 am, Daniel D.Daugherty wrote:

On Fri, 8 Jan 2021 22:30:15 GMT, David Holmes <dholmes at openjdk.org> wrote:

Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision:

delete bad assert found by linux build.

src/hotspot/share/runtime/globals.hpp line 718:

716: range(0, max_jint) \
717: \
718: /* notice: the max range value here is max_jint, not max_intx */ \

The comment is no longer applicable.

Deleted

src/hotspot/share/runtime/synchronizer.cpp line 257:

255: // which is called after cmd line options are processed.
256: // This is a 'jint' because the range of AvgMonitorsPerThreadEstimate
257: // is 0..max_jint:

Do we need this comment now?

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

But the "This is jint" part is no longer relevant, the comment was
really reading:

This is a jint [rather than intx] because the range of AMPTE is
0..max_jint [not 0..max_intx]

but now AMPTE is an int we don't need to make that clarification on the
type.

src/hotspot/share/runtime/synchronizer.cpp line 1178:

1176: }
1177:
1178: void ObjectSynchronizer::set_in_use_list_ceiling(size_t new_value) {

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

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.

If you have a jint field being set by a setter function then the setter
function should take a jint argument. The types of the other fields is
not really relevant. But this mish-mash of int/jint/size_t really bugs
me. :(

Cheers,
David

@dcubed-ojdk
Copy link
Member Author

I'm really tempted to back out the change that made AvgMonitorsPerThreadEstimate
an int instead of an intx and all the associated follow-on changes. Fixing that
type wasn't the intent of this fix and I'm spending way too much time on stuff that
has absolutely nothing to do with the bug that I'm trying to solve.

@mlbridge
Copy link

mlbridge bot commented Jan 9, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

On 9/01/2021 12:28 pm, Daniel D.Daugherty wrote:

On Fri, 8 Jan 2021 23:09:15 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision:

dholmes CR - delete a stale comment.

I had a couple of questions.

I'm really tempted to back out the change that made AvgMonitorsPerThreadEstimate
an `int` instead of an `intx` and all the associated follow-on changes. Fixing that
type wasn't the intent of this fix and I'm spending way too much time on stuff that
has absolutely nothing to do with the bug that I'm trying to solve.

Sorry about that. The cast and the comment about the cast is what jumped
out at me here. The need for a setter was unclear (and you've now
explained it is needed for another fix), but the fact the setter takes a
size_t param to be assigned to a jint field just looks wrong to me. That
is irrespective of the type of AMPTE.

David

@dcubed-ojdk
Copy link
Member Author

dcubed-ojdk commented Jan 11, 2021

In the "learn something new every day" category, I just learned that
command line options can be of type size_t. I've updated the patch
again. Please take another look. Mach5 Tier[1-3] testing is in process.

@dcubed-ojdk
Copy link
Member Author

dcubed-ojdk commented Jan 11, 2021

Mach5 Tier[1-3] testing looks good; no Tier[12] failures; 2 unrelated
Tier3 failures.

@coleenp and @dholmes-ora - a re-review would be appreciated.

I believe this fix will also address:

https://bugs.openjdk.java.net/browse/JDK-8256307 cleanup AvgMonitorsPerThreadEstimate and _in_use_list_ceiling types

I'm sorry that I forgot about that bug...

Copy link
Contributor

@coleenp coleenp left a comment

Choose a reason for hiding this comment

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

I like the new changes!

"Used to estimate a variable ceiling based on number of threads " \
"for use with MonitorUsedDeflationThreshold (0 is off).") \
range(0, max_jint) \
range(0, max_uintx) \
Copy link
Contributor

Choose a reason for hiding this comment

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

This matches all the other size_t ranges.

Copy link
Member

Choose a reason for hiding this comment

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

Yes it matches - but pity we don't have a max_size_t definition :(

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.

Oh, Ok, on both points.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

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

Hi Dan,
I'm happy with the consistent use of size_t - thanks. :)

A couple of minor comments remaining.

Thanks,
David

"Used to estimate a variable ceiling based on number of threads " \
"for use with MonitorUsedDeflationThreshold (0 is off).") \
range(0, max_jint) \
range(0, max_uintx) \
Copy link
Member

Choose a reason for hiding this comment

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

Yes it matches - but pity we don't have a max_size_t definition :(

#endif
assert(l_in_use_list_ceiling > 0, "in_use_list_ceiling=" SIZE_FORMAT
": must be > 0", l_in_use_list_ceiling);
Atomic::add(&_in_use_list_ceiling, -AvgMonitorsPerThreadEstimate);
Copy link
Member

Choose a reason for hiding this comment

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

Don't you need to cast AMPTE to a signed type to take the negative of it?
Why not just use Atomic::sub?

Copy link
Member Author

Choose a reason for hiding this comment

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

Apparently no cast is needed since the current patch
passes Mach[1-3]. I don't remember the reason that
Atomic::add() of a negative value was used, but that's
not new in the patch so I'm planning to leave it alone.

@dcubed-ojdk
Copy link
Member Author

@coleenp and @dholmes-ora - thanks for the re-reviews.

@dcubed-ojdk
Copy link
Member Author

/integrate

@openjdk openjdk bot closed this Jan 12, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 12, 2021
@openjdk
Copy link

openjdk bot commented Jan 12, 2021

@dcubed-ojdk Since your change was applied there have been 60 commits pushed to the master branch:

  • ccac7aa: 8258932: AArch64: Enhance floating-point Min/MaxReductionV with fminp/fmaxp
  • 4c75d14: 8259374: Make ThreadInVMfromNative have ResetNoHandleMark
  • 563b268: 8257709: C1: Double assignment in InstructionPrinter::print_stack
  • 400dc76: 8252015: [macos11] java.awt.TrayIcon requires updates for template images
  • ac2dee5: 8259539: JDK-8255711 broke trap messages
  • 4697cfa: 8259576: Misplaced curly brace in Matcher::find_shared_post_visit
  • a6ab9e4: 8258576: Try to get zerobased CCS if heap is above 32 and CDS is disabled
  • a3561ae: 8258243: C2: assert failed ("Bad derived pointer") with -XX:+VerifyRegisterAllocator
  • 4663704: 8259583: Remove unused decode_env::_codeBuffer
  • 9d4e84f: 8259565: Zero: compiler/runtime/criticalnatives fails because CriticalJNINatives is not supported
  • ... and 50 more: https://git.openjdk.java.net/jdk/compare/090bd3afc37555582bde61d03e2e32350ae5d714...master

Your commit was automatically rebased without conflicts.

Pushed as commit c338f11.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@dcubed-ojdk dcubed-ojdk deleted the JDK-8259349 branch January 25, 2021 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-runtime hotspot-runtime-dev@openjdk.org integrated Pull request has been integrated
3 participants