RFR: 8253064: monitor list simplifications and getting rid of TSM [v4]

[Coleen Phillimore]

On Tue, 10 Nov 2020 02:35:17 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> Changes from @fisk and @dcubed-ojdk to:
>> 
>> - simplify ObjectMonitor list management
>> - get rid of Type-Stable Memory (TSM)
>> 
>> This change has been tested with Mach5 Tier[1-3],4,5,6,7,8; no new regressions.
>> Aurora Perf runs have also been done (DaCapo-h2, Quick Startup/Footprint,
>> SPECjbb2015-Tuned-G1, SPECjbb2015-Tuned-ParGC, Volano)
>>   - a few minor regressions (<= -0.24%)
>>   - Volano is 6.8% better
>> 
>> Eric C. has also done promotion perf runs on these bits and says "the results look fine".
>
> Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision:
> 
>   dholmes-ora - convert inner while loop to do-while loop in unlink_deflated().

Changes requested by coleenp (Reviewer).

src/hotspot/share/oops/markWord.cpp line 63:

> 61:   fatal("bad header=" INTPTR_FORMAT, value());
> 62: }
> 63: 

This makes it so much clearer where the displaced markWord is.

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

> 748:   product(intx, MonitorUsedDeflationThreshold, 90, EXPERIMENTAL,            \
> 749:           "Percentage of used monitors before triggering deflation (0 is "  \
> 750:           "off). The check is performed on GuaranteedSafepointInterval "    \

Should there still be experimental options after this change?

src/hotspot/share/runtime/monitorDeflationThread.cpp line 85:

> 83:       // visible to external suspension.
> 84: 
> 85:       ThreadBlockInVM tbivm(jt);

Does this have to be a JavaThread?  Could it be a non-java thread since deflating monitors doesn't have to call any Java code?  You'd have to lock down the Monitor list maybe, but couldn't this be a NamedThread?  This isn't a request to change it right now.

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

> 1639: 
> 1640: // Do the final audit and print of ObjectMonitor stats; must be done
> 1641: // by the VMThread (at VM exit time).

Can you take (at VM exit time) out of parenthesis?  it made me wonder when else is this called.

src/hotspot/share/runtime/objectMonitor.cpp line 509:

> 507: //
> 508: bool ObjectMonitor::deflate_monitor() {
> 509:   if (is_busy()) {

is_busy should be checked != 0 since it doesn't return a bool.

src/hotspot/share/runtime/objectMonitor.cpp line 540:

> 538:     if (try_set_owner_from(NULL, DEFLATER_MARKER) != NULL) {
> 539:       // The owner field is no longer NULL so we lost the race since the
> 540:       // ObjectMonitor is now busy.

So here would contentions be > 0?  Can it be asserted?  Doesn't need to be, the comment really helps to understand why the cas failed.

src/hotspot/share/runtime/objectMonitor.cpp line 551:

> 549:       if (try_set_owner_from(DEFLATER_MARKER, NULL) != DEFLATER_MARKER) {
> 550:         // Deferred decrement for the JT EnterI() that cancelled the async deflation.
> 551:         add_to_contentions(-1);

contentions is essentially a refcount, isn't it.  Can you fix the comment to include this at line 360 since that's not the only purpose of this count.

  // Keep track of contention for JVM/TI and M&M queries.
  add_to_contentions(1);

src/hotspot/share/runtime/synchronizer.hpp line 61:

> 59:   bool has_next() const { return _current != NULL; }
> 60:   ObjectMonitor* next();
> 61: };

Can MonitorList be defined in the .cpp file?  I don't see anything outside of synchronizer.cpp that refers to it.

src/hotspot/share/runtime/synchronizer.hpp line 28:

> 26: #define SHARE_RUNTIME_SYNCHRONIZER_HPP
> 27: 
> 28: #include "logging/logStream.hpp"

If you need to put MonitorList in the header file, use a forward declaration for LogStream instead of #including logstream.hpp.

src/hotspot/share/runtime/objectMonitor.hpp line 171:

> 169:   volatile int _SpinDuration;
> 170: 
> 171:   jint  _contentions;               // Number of active contentions in enter(). It is used by is_busy()

Future RFE - can we replace jint with int32_t or even int or some C++ types.  We're trying not to have Java types leak into runtime code since this doesn't directly interface with Java.

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

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