On Mon, 15 Jul 2024 00:44:31 GMT, Axel Boldt-Christmas <abold...@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
