RFR: 8315884: New Object to ObjectMonitor mapping [v6]

[Coleen Phillimore]

On Mon, 15 Jul 2024 00:44:31 GMT, Axel Boldt-Christmas <aboldtch at openjdk.org> wrote:

>> src/hotspot/share/runtime/basicLock.cpp line 37:
>> 
>>> 35:     if (mon != nullptr) {
>>> 36:       mon->print_on(st);
>>> 37:     }
>> 
>> I am not sure if we wanted to do this, but we know the owner, therefore we could also look-up the OM from the table, and print it. It wouldn't have all that much to do with the BasicLock, though.
>
> Yeah maybe it is unwanted. Not sure how we should treat these prints of the frames. My thinking was that there is something in the cache, print it. But maybe just treating it as some internal data, maybe print "monitor { <Cached ObjectMonitor* address> }" or similar is better.

It seems generally useful to print the monitor in the cache if it's there.  I don't think we should do a table search here.  I think this looks fine as it is, and might be helpful for debugging if it turns out to be the wrong monitor.

>> src/hotspot/share/runtime/lightweightSynchronizer.cpp line 80:
>> 
>>> 78: 
>>> 79:   ConcurrentTable* _table;
>>> 80:   volatile size_t _table_count;
>> 
>> Looks like a misnomer to me. We only have one table, but we do have N entries/nodes. This is counted when new nodes are allocated or old nodes are freed. Consider renaming this to '_entry_count' or '_node_count'? I'm actually a bit surprised if ConcurrentHashTable doesn't already track this...
>
> I think I was thinking of the names as a prefix to refer to the `Count of the table` and `Size of the table`. And not the `Number of tables`. But I can see the confusion. 
> 
> `ConcurrentHashTable` tracks no statistics except for JFR which added some counters directly into the implementation. All statistics are for the users to manage, even if there are helpers for gather these statistics. 
> 
> The current implementation is based on what we do for the StringTable and SymbolTable

In the other tables, it's called _items_count and it determines the load_factor for triggering concurrent work.  We should rename this field items_count to match, and also since it's consistent.

>> src/hotspot/share/runtime/lightweightSynchronizer.cpp line 159:
>> 
>>> 157:   static size_t min_log_size() {
>>> 158:     // ~= log(AvgMonitorsPerThreadEstimate default)
>>> 159:     return 10;
>> 
>> Uh wait - are we assuming that threads hold 1024 monitors *on average* ? Isn't this a bit excessive? I would have thought maybe 8 monitors/thread. Yes there are workloads that are bonkers. Or maybe the comment/flag name does not say what I think it says.
>> 
>> Or why not use AvgMonitorsPerThreadEstimate directly?
>
> Maybe that is resonable. I believe I had that at some point but it had to deal with how to handle extreme values of `AvgMonitorsPerThreadEstimate` as well as what to do when `AvgMonitorsPerThreadEstimate` was disabled `=0`. One 4 / 8 KB allocation seems harmless.
> 
> But this was very arbitrary. This will probably be changed when/if the resizing of the table becomes more synchronised with deflation, allowing for shrinking the table.

Shrinking the table is NYI.  Maybe we should revisit this initial value then.

>> src/hotspot/share/runtime/lightweightSynchronizer.cpp line 563:
>> 
>>> 561:     assert(locking_thread == current || locking_thread->is_obj_deopt_suspend(), "locking_thread may not run concurrently");
>>> 562:     if (_no_safepoint) {
>>> 563:       ::new (&_nsv) NoSafepointVerifier();
>> 
>> I'm thinking that it might be easier and cleaner to just re-do what the NoSafepointVerifier does? It just calls thread->inc/dec
>> _no_safepoint_count().
>
> I wanted to avoid having to add `NoSafepointVerifier` implementation details in the synchroniser code. I guess `ContinuationWrapper` already does this. 
> 
> Simply creating a `NoSafepointVerifier` when you expect no safepoint is more obvious to me, shows the intent better.

This looks strange to me also, but it's be better than changing the no_safepoint_count directly, since NSV handles when the current thread isn't a JavaThread, so you'd have to duplicate that in this VerifyThreadState code too.

    NoSafepointVerifier::NoSafepointVerifier() : _thread(Thread::current()) {
      if (_thread->is_Java_thread()) {
        JavaThread::cast(_thread)->inc_no_safepoint_count();
      }
    }

>> src/hotspot/share/runtime/lightweightSynchronizer.hpp line 68:
>> 
>>> 66:   static void exit(oop object, JavaThread* current);
>>> 67: 
>>> 68:   static ObjectMonitor* inflate_into_object_header(Thread* current, JavaThread* inflating_thread, oop object, const ObjectSynchronizer::InflateCause cause);
>> 
>> My IDE flags this with a warning 'Parameter 'cause' is const-qualified in the function declaration; const-qualification of parameters only has an effect in function definitions' *shrugs*
>
> Yeah. The only effect is has is that you cannot reassign the variable. It was the style taken from [synchronizer.hpp](https://github.com/openjdk/jdk/blob/15997bc3dfe9dddf21f20fa189f97291824892de/src/hotspot/share/runtime/synchronizer.hpp) where all `InflateCause` parameters are const.

Do you get this for inflate_fast_locked_object also?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1688011833
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1688162915
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1688378429
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1688385921
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1688397480