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

[Robbin Ehn]

On Tue, 10 Nov 2020 20:45:18 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> 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.
>
> We may still decide to do this fix (even though the _object field is now weak):
> 
>     JDK-8249638 Re-instate idle monitor deflation as part of System.gc()
>     https://bugs.openjdk.java.net/browse/JDK-8249638
> 
> and if we do, then we'll still need the request mechanism.

So why not use a local semaphore and wait with safepoint check instead?

>> 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);
>
> The baseline code (ObjectSynchronizer::deflate_common_idle_monitors())
> uses ThreadBlockInVM currently. I don't want to change that as part of
> this work. If we want to generally change uses of ThreadBlockInVM to
> something else, then we should do that with a dedicated bug.

Currently there is no issue with ThreadBlockInVM since there is no code inside those scopes.
This adds code there which assumes the timer will be 'resumed', and logs "resume" when it actually could be going to a safepoint.

>> 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.
>> 
>> (From what I remember you can actually release during a safepoint, but that is not future-prof as I understood it then. So I skipped going into that rabbit hole this time also.)
>
> @fisk said we should not release the oopStorage during a safepoint
> because that's not safe or will not be safe. I can't remember which.

Yes that's why I said you can release it during deflation instead.

(not saying you should do this in this feeature change-set)

>> 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.
>
> We've removed enough padding with this work already. If we
> want to do more padding removal, then we need to use a
> different RFE.

Sure, this was more a FYI.

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

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