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

Daniel D.Daugherty dcubed at openjdk.java.net
Thu Oct 15 14:17:14 UTC 2020


On Thu, 15 Oct 2020 00:41:08 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> 8254029: ObjectMonitor cleanup/minor bug-fix changes extracted from JDK-8253064
>
> src/hotspot/share/runtime/synchronizer.cpp line 2949:
> 
>> 2947:   assert(Thread::current()->is_VM_thread(), "sanity check");
>> 2948:
>> 2949:   if (is_final_audit()) {  // Only do the audit once.
> 
> 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.

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.

> src/hotspot/share/runtime/vmOperations.cpp line 458:
> 
>> 456:   // The ObjectMonitor subsystem uses perf counters so do this before
>> 457:   // we call exit_globals() so we don't run afoul of perfMemory_exit().
>> 458:   ObjectSynchronizer::do_final_audit_and_print_stats();
> 
> 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?

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.

> src/hotspot/share/runtime/vmThread.cpp line 194:
> 
>> 192:   // we signal that the VM thread is gone. We don't want to run afoul
>> 193:   // of perfMemory_exit() in exit_globals().
>> 194:   ObjectSynchronizer::do_final_audit_and_print_stats();
> 
> 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?

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.

-------------

PR: https://git.openjdk.java.net/jdk/pull/641


More information about the hotspot-runtime-dev mailing list