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

[David Holmes]

On Thu, 15 Oct 2020 13:47:18 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

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

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.

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

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

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

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