RFR: 8253064: monitor list simplifications and getting rid of TSM

[David Holmes]

On Tue, 13 Oct 2020 20:31:44 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".

Hi Dan,

Overall this looks great. Comparing old and new code is complex but the new code on its own is generally much simpler/clearer (not all though :) ).

I have a few nits, comments and queries below.

Thanks,
David

src/hotspot/share/runtime/monitorDeflationThread.hpp line 44:

> 42: 
> 43:   // Hide this thread from external view.
> 44:   bool is_hidden_from_external_view() const      { return true; }

Style nit: a single space after "const" suffices

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

> 378:   if (event.should_commit()) {
> 379:     event.set_monitorClass(object()->klass());
> 380:     event.set_address((uintptr_t)this);

This looks wrong - the event should refer to the Object whose monitor we have entered, not the OM itself.

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

> 1470:   event->set_monitorClass(monitor->object()->klass());
> 1471:   event->set_timeout(timeout);
> 1472:   event->set_address((uintptr_t)monitor);

Again the event should refer to the Object, not the OM.

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

> 143:   // have busy multi-threaded access. _header and _object are set at initial
> 144:   // inflation. The _object does not change, so it is a good choice to share
> 145:   // its the cache line with _header.

Typo: its the

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

> 58: 
> 59: void MonitorList::add(ObjectMonitor* m) {
> 60:   for (;;) {

Style nit: a do/while loop would like nicer here.

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

> 92:       // Find next live ObjectMonitor.
> 93:       ObjectMonitor* next = m;
> 94:       while (next != NULL && next->is_being_async_deflated()) {

Nit: This loop seems odd. Given we know m is_being_async_deflated, this should either be a do/while loop, or else we should initialize:

ObjectMonitor* next = m->next_om();

and dispense with the awkwardly named next_next.

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

> 124:     }
> 125: 
> 126:     if (self->is_Java_thread() &&

Non-JavaThreads may have a monitor list ???

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

> 134: 
> 135:       // Honor block request.
> 136:       ThreadBlockInVM tbivm(self->as_Java_thread());

ThreadBlockInVM is generally used to wrap blocking code, not to cause the current thread to block (which it will do as a side-effect if a safepoint/handshake is requested). Surely this should just be call to `process_if_requested` (or the new `process_if_requested_with_exit_check`)?

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

> 1423:     }
> 1424: 
> 1425:     if (self->is_Java_thread() &&

Again unclear how a non-JavaThread is doing this? Isn't this always done by the MonitorDeflation thread??

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

> 1433: 
> 1434:       // Honor block request.
> 1435:       ThreadBlockInVM tbivm(self->as_Java_thread());

Same comment as previous use of TBIVM. (It's hard to see in the PR UI how they two blocks relate.)

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

> 1458: // This function is called by the MonitorDeflationThread to deflate
> 1459: // ObjectMonitors. It is also called via do_final_audit_and_print_stats()
> 1460: // by the VMThread.

Ah! I think this addresses my previous comments about a non-JavaThread doing this.

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

> 1499:       if (ls != NULL) {
> 1500:         timer.stop();
> 1501:         ls->print_cr("before handshaking: unlinked_count=" SIZE_FORMAT ", in_use_list stats: ceiling=" SIZE_FORMAT ", count=" SIZE_FORMAT ", max=" SIZE_FORMAT,

Style nit: line too long

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

> 1518:     // deflated in this cycle.
> 1519:     size_t deleted_count = 0;
> 1520:     for (ObjectMonitor* monitor: delete_list) {

I didn't realize C++ has a "foreach" loop construct! Is this in our allowed C++ usage?

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

> 1531: 
> 1532:         // Honor block request.
> 1533:         ThreadBlockInVM tbivm(self->as_Java_thread());

Ditto previous comments on use of TBIVM.

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

> 1710: // Check the in_use_list; log the results of the checks.
> 1711: void ObjectSynchronizer::chk_in_use_list(outputStream* out, int *error_cnt_p) {
> 1712:   size_t l_in_use_count = _in_use_list.count();

Style nit: what is this `l_` prefix? Is that a one or a  small L? why do we want want/need it? (Applies elsewhere too)

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

> 1746:                                           int* error_cnt_p) {
> 1747:   if (n->owner_is_DEFLATER_MARKER()) {
> 1748:     // This should not happen, but if it does, it is not fatal.

Deflating an in-use monitor is not fatal? Please explain how things would recover.

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

> 171: 
> 172:   static MonitorList   _in_use_list;
> 173:   static jint          _in_use_list_ceiling;

Can you add some commentary on what this ceiling is as I could not understand its role just by looking at the code. Thanks.

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

> 219: 
> 220: MonitorList ObjectSynchronizer::_in_use_list;
> 221: // Start the ceiling with one thread:

This relates to me not understanding what this ceiling is (as commented elsewhere) but why does this say "start with one thread" when the value of AvgMonitorsPerThreadEstimate defaults to 1024 ??

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

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