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

[Robbin Ehn]

On Tue, 10 Nov 2020 17:39:25 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:
> 
>   coleenp CR - refactor common ThreadBlockInVM code block into ObjectSynchronizer::chk_for_block_req().

Hi, thanks for fixing.

I had some comments nothing so approving.

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

> 90:         // We wait for GuaranteedSafepointInterval so that
> 91:         // is_async_deflation_needed() is checked at the same interval.
> 92:         ml.wait(GuaranteedSafepointInterval);

I don't like that we add a new global monitor just for the whitebox API.
Without WB poking this could just a plain sleep.

If we must have this new global monitor there seems be no reason for this to be no safepoint ?
So ThreadBlockInVM would not be needed if we did safepoint checks instead?

I would either skip WB notification and run the test with a very low GuaranteedSafepointInterval and just set flag and wait a second.
Or if this is really important use a local semaphore and skip the boolean, each increase on sema would result in a deflation pass.

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

> 1417:                  ", count=" SIZE_FORMAT ", max=" SIZE_FORMAT, op_name,
> 1418:                  in_use_list_ceiling(), _in_use_list.count(), _in_use_list.max());
> 1419:     timer_p->start();

ThreadBlockInVM have a miss-feature: it safepoint polls on front-edge and back-edge.
It should only have that poll on backedge, once that is fixed, this will do the wrong thing.
Also you may safepoint on both front-edge and back-edge now, so the timer would show the wrong thing then.

So to get the timer correct you should use:
SafepointMechanism::process_if_requested(thread);

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

> 1530:         // A JavaThread must check for a safepoint/handshake and honor it.
> 1531:         chk_for_block_req(self->as_Java_thread(), "deletion", "deleted_count",
> 1532:                           deleted_count, ls, &timer);

If you release oopStorage when deflating you can do this entire loop while blocked instead, which will be faster.

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

> 146:   DEFINE_PAD_MINUS_SIZE(0, OM_CACHE_LINE_SIZE, sizeof(volatile markWord) +
> 147:                         sizeof(WeakHandle));
> 148:   // Used by async deflation as a marker in the _owner field:

I have test with and without padding, I saw no difference.

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

Marked as reviewed by rehn (Reviewer).

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