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

8254029: ObjectMonitor cleanup/minor bug-fix changes extracted from JDK-8253064 #641

Closed
wants to merge 2 commits into from

Conversation

@dcubed-ojdk
Copy link
Member

@dcubed-ojdk dcubed-ojdk commented Oct 13, 2020


Progress

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

Issue

  • JDK-8254029: ObjectMonitor cleanup/minor bug-fix changes extracted from JDK-8253064

Reviewers

Download

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

Daniel D. Daugherty and others added 2 commits Oct 12, 2020
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 13, 2020

👋 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.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Oct 13, 2020

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

  • hotspot

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.

Loading

@openjdk openjdk bot added the hotspot label Oct 13, 2020
@dcubed-ojdk
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk commented Oct 13, 2020

/label add hotspot-runtime
/label remove hotspot

Loading

@dcubed-ojdk
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk commented Oct 13, 2020

This sub-task is tracking ObjectMonitor cleanup/minor bug-fix changes
extracted from Erik's and Dan's work on JDK-8253064. This extraction
is done to ease the code review for the JDK-8253064 changes.

My personal fork includes the commit for:
JDK-8254335: logging/logStream.hpp includes memory/resourceArea.hpp but d…
because the project is currently based on jdk-16+19 and I pushed that commit for jdk-16+20.
Please ignore that commit when reviewing this fix.

Loading

@dcubed-ojdk
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk commented Oct 13, 2020

The changes covered by this bug are:

  • Redo the "final" ObjectMonitor audit and print stats so that we no longer
    race between the ServiceThread and a thread generating ObjectMonitors
    until the end of time. The final audit is now done by the VMThread in the
    very late stages of VM exit (after other threads have blocked). Because
    we have two distinict VM exit code paths, we call the final audit hook
    from both paths.

    The race was introduced by the following:

    JDK-8246476 remove AsyncDeflateIdleMonitors option and the safepoint based deflation mechanism
    
  • Do some renaming that should have been done by JDK-8246476.

  • Delete the SharedGlobals::stw_cycle that should have been deleted
    by JDK-8246476.

  • Do more "self" cleanup and use of as_Java_thread().

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Oct 13, 2020

@dcubed-ojdk
The hotspot-runtime label was successfully added.

Loading

@openjdk openjdk bot removed the hotspot label Oct 13, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 13, 2020

@dcubed-ojdk
The hotspot label was successfully removed.

Loading

@dcubed-ojdk
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk commented Oct 14, 2020

Mach5 Tier[1-3],4,5,6,7 testing looks good; there are four test
failures associated with unrelated, known bugs; there are four
other test failures on a machine with bad hardware so those are
invalid.

Update: Tier8 testing is still in process, but so far the results
look good.

Loading

@dcubed-ojdk dcubed-ojdk marked this pull request as ready for review Oct 14, 2020
@openjdk openjdk bot added the rfr label Oct 14, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 14, 2020

Webrevs

Loading

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi Dan,

Overall this looks good to me.

A few minor comments/queries below.

Thanks,
David

Loading

@@ -463,6 +453,10 @@ void VM_Exit::doit() {

set_vm_exited();

// The ObjectMonitor subsystem uses perf counters so do this before
// we call exit_globals() so we don't run afoul of perfMemory_exit().
ObjectSynchronizer::do_final_audit_and_print_stats();
Copy link
Member

@dholmes-ora dholmes-ora Oct 15, 2020

Choose a reason for hiding this comment

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

Given this used to be done in the prolog, is there anything above this line that might interfere with it somehow? Or more simply put, why not do this first so that it is as close as possible to where it used to be?

Loading

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk Oct 15, 2020

Choose a reason for hiding this comment

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

The async deflation request was made in doit_prologue() which
meant that the ServiceThread would eventually do the work, but
that was in a racy fashion which is what motivated this change.

exit_globals() below used to make the call to audit_and_print_stats()
so we're actually doing that part a little bit earlier than before. The only
boundary condition that I've ever found in all of my testing was that the
audit needed to be done before the perfMemory_exit() call that's made
in exit_globals(). I have not seen any issues caused by the code that's
executed in VM_Exit::doit() before we get to the final audit.

Loading

Copy link
Member

@dholmes-ora dholmes-ora Oct 15, 2020

Choose a reason for hiding this comment

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

Thanks. I missed the fact that the old code was only an async deflation request.

Loading

void ObjectSynchronizer::do_final_audit_and_print_stats() {
assert(Thread::current()->is_VM_thread(), "sanity check");

if (is_final_audit()) { // Only do the audit once.
Copy link
Member

@dholmes-ora dholmes-ora Oct 15, 2020

Choose a reason for hiding this comment

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

It seems impossible that you could ever call this twice. A simple file static flag for a debug build would be simpler if you just wanted to assert that.

Loading

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk Oct 15, 2020

Choose a reason for hiding this comment

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

I modeled the make-sure-this-is-only-called-once logic on
exit_globals(). I originally implemented it as a static flag just
like exit_globals(). Is it necessary? I don't think so either, but
I'm guarding against future changes since the final audit
should only be done once.

In the changeset that follows this one (JDK-8253064), this
is_final_audit() query function is needed to change some of
the logic that gets executed in the "final" audit. I backported
that bit to this bug fix so there was one less thing to review
in JDK-8253064.

Loading

Copy link
Member

@dholmes-ora dholmes-ora Oct 15, 2020

Choose a reason for hiding this comment

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

The final_audit flag seems overkill. If the audit code needs to know it is the final audit that could be passed as a parameter.
The make-sure-this-is-only-called-once logic gives the false impression that it is entirely possible for this to be called more than once when that is not the case. We can only reach this code after the termination safepoint has been reached. A simple assert would be clearer IMO, but I don't insist on any changes here.

Loading

// The ObjectMonitor subsystem uses perf counters so do this before
// we signal that the VM thread is gone. We don't want to run afoul
// of perfMemory_exit() in exit_globals().
ObjectSynchronizer::do_final_audit_and_print_stats();
Copy link
Member

@dholmes-ora dholmes-ora Oct 15, 2020

Choose a reason for hiding this comment

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

Again I wonder why this was moved so far from the original call to deflate? It makes sense to do the final deflation once the final safepoint has been reached, but should we do it first once the safepoint has been reached?

Loading

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk Oct 15, 2020

Choose a reason for hiding this comment

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

Again the old code made an async deflation request which meant
that the ServiceThread would eventually do the work, but that was
in a racy fashion which is what motivated this change.

The exit_globals() call is a little less obvious for this code path.
That happens over in src/hotspot/share/runtime/thread.cpp in the
Threads::destroy_vm() function. In destroy_vm() we wait for the
VMThread to exit via a VMThread::wait_for_vm_thread_exit() call
and a bit later we call exit_globals().

In the new code, we're calling do_final_audit_and_print_stats() as
part of the VMThread's exit path code in the VMThread::run()
function so, again, we're doing the final audit a little bit earlier than
we used to do it before.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Oct 15, 2020

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

8254029: ObjectMonitor cleanup/minor bug-fix changes extracted from JDK-8253064

Reviewed-by: dholmes, eosterlund

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

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.

Loading

@openjdk openjdk bot added the ready label Oct 15, 2020
@dcubed-ojdk
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk commented Oct 15, 2020

@dholmes-ora - Thanks for the review! Please let me know if
the above replies answer your questions.

Loading

void ObjectSynchronizer::do_final_audit_and_print_stats() {
assert(Thread::current()->is_VM_thread(), "sanity check");

if (is_final_audit()) { // Only do the audit once.
Copy link
Member

@dholmes-ora dholmes-ora Oct 15, 2020

Choose a reason for hiding this comment

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

The final_audit flag seems overkill. If the audit code needs to know it is the final audit that could be passed as a parameter.
The make-sure-this-is-only-called-once logic gives the false impression that it is entirely possible for this to be called more than once when that is not the case. We can only reach this code after the termination safepoint has been reached. A simple assert would be clearer IMO, but I don't insist on any changes here.

Loading

@@ -463,6 +453,10 @@ void VM_Exit::doit() {

set_vm_exited();

// The ObjectMonitor subsystem uses perf counters so do this before
// we call exit_globals() so we don't run afoul of perfMemory_exit().
ObjectSynchronizer::do_final_audit_and_print_stats();
Copy link
Member

@dholmes-ora dholmes-ora Oct 15, 2020

Choose a reason for hiding this comment

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

Thanks. I missed the fact that the old code was only an async deflation request.

Loading

@dcubed-ojdk
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk commented Oct 16, 2020

@dholmes-ora - Thanks for the review and thanks for not
insisting on changes to the final_audit flag.

Loading

@dcubed-ojdk
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk commented Oct 16, 2020

@fisk - Please review this when you get a chance since this is an
extraction for your ObjectMonitor list cleanup and TSM removal work
(https://bugs.openjdk.java.net/browse/JDK-8253064).

Loading

Copy link
Contributor

@fisk fisk left a comment

Looks great in general. I only have one question regarding when deflation would not be called by a Java thread (which is now checked for).

Loading

// The ServiceThread's async deflation request has been processed.
_last_async_deflation_time_ns = os::javaTimeNanos();
set_is_async_deflation_requested(false);
if (self->is_Java_thread()) {
Copy link
Contributor

@fisk fisk Oct 19, 2020

Choose a reason for hiding this comment

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

When would this not be run by a Java thread?

Loading

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk Oct 19, 2020

Choose a reason for hiding this comment

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

ObjectSynchronizer::do_final_audit_and_print_stats() calls:

L2958: ObjectSynchronizer::deflate_idle_monitors();

and do_final_audit_and_print_stats() is called from two places:

  • void VM_Exit::doit() - The do_final_audit_and_print_stats() call happens
    when the VMThread is executing the VM_Exit VM-op. That happens as
    a result of a vm_exit() call a.k.a. System.exit().

  • VMThread::run() - The do_final_audit_and_print_stats() call happens
    after the VMThread has been terminated and finished processing
    VM-ops. So this is the jni_DestroyJavaVM() path that happens when
    the program falls off the end of main() and that thread does an
    orderly shutdown of the VMThread.

A key piece of this changeset is moving the final audit to be executed
by the VMThread on one of its two exit code paths. The final audit
includes a calls to deflate_idle_monitors() to make sure things are as
clean as possible before doing the audit_and_print_stats() call.

None of this happens if the right logging is not enabled.

Loading

Copy link
Contributor

@fisk fisk Oct 19, 2020

Choose a reason for hiding this comment

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

Okay, that makes sense. Thanks for the explanation.

Loading

@dcubed-ojdk
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk commented Oct 19, 2020

@fisk - Thanks for the review. Please let me know if I have resolved
your question.

Loading

fisk
fisk approved these changes Oct 19, 2020
Copy link
Contributor

@fisk fisk left a comment

Looks good. Thanks for fixing!

Loading

@dcubed-ojdk
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk commented Oct 20, 2020

/integrate

Loading

@openjdk openjdk bot closed this Oct 20, 2020
@openjdk openjdk bot added integrated and removed ready labels Oct 20, 2020
@openjdk openjdk bot removed the rfr label Oct 20, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 20, 2020

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

Your commit was automatically rebased without conflicts.

Pushed as commit c87cdf7.

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

Loading

@dcubed-ojdk dcubed-ojdk deleted the JDK-8254029 branch Oct 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants