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

[Daniel D.Daugherty]

On Tue, 10 Nov 2020 20:34:12 GMT, Robbin Ehn <rehn at openjdk.org> wrote:

>> 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 major so approving.

@robehn - Thanks for the review!! And thanks for approving.

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

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

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

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

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