On Fri, 12 Jul 2024 11:09:35 GMT, Roman Kennke <rken...@openjdk.org> wrote:
>> Axel Boldt-Christmas has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Update arguments.cpp
>
> src/hotspot/share/oops/instanceKlass.cpp line 1090:
>
>> 1088:
>> 1089: // Step 2
>> 1090: // If we were to use wait() instead of waitUninterruptibly() then
>
> This is a nice correction (even though, the actual call below is
> wait_uninterruptibly() ;-) ), but seems totally unrelated.
I was thinking it was referring to `ObjectSynchronizer::waitUninterruptibly`
added the same commit as the comment b3bf31a0a08da679ec2fd21613243fb17b1135a9
> src/hotspot/share/oops/markWord.cpp line 27:
>
>> 25: #include "precompiled.hpp"
>> 26: #include "oops/markWord.hpp"
>> 27: #include "runtime/basicLock.inline.hpp"
>
> I don't think this include is needed (at least not by the changed code parts,
> I haven't checked existing code).
It is probably included through some other transitive include. However all the
metadata functions are now inlined. These are used here. `inline markWord
BasicLock::displaced_header() const` and `inline void
BasicLock::set_displaced_header(markWord header)`
> src/hotspot/share/runtime/arguments.cpp line 1820:
>
>> 1818: warning("New lightweight locking not supported on this platform");
>> 1819: }
>> 1820: if (UseObjectMonitorTable) {
>
> Uhm, wait a second. That list of platforms covers all existing platforms
> anyway, so the whole block could be removed? Or is there a deeper meaning
> here that I don't understand?
Zero. Used as as start point for porting to new platforms.
> 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.
> src/hotspot/share/runtime/basicLock.inline.hpp line 45:
>
>> 43: return reinterpret_cast<ObjectMonitor*>(get_metadata());
>> 44: #else
>> 45: // Other platforms does not make use of the cache yet,
>
> If it's not used, why does it matter to special case the code here?
Because it is not used it there may be uninitialised values there.
See https://github.com/openjdk/jdk/pull/20067#discussion_r1671959763
> src/hotspot/share/runtime/lightweightSynchronizer.cpp line 28:
>
>> 26:
>> 27: #include "classfile/vmSymbols.hpp"
>> 28: #include "javaThread.inline.hpp"
>
> This include is incorrect (and my IDE says it's not needed).
Correct, is should be `runtime/javaThread.inline.hpp`. Fixed.
> src/hotspot/share/runtime/lightweightSynchronizer.cpp line 31:
>
>> 29: #include "jfrfiles/jfrEventClasses.hpp"
>> 30: #include "logging/log.hpp"
>> 31: #include "logging/logStream.hpp"
>
> Include of logStream.hpp not needed?
Yeah we removed all log streams. Removed.
> 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
> src/hotspot/share/runtime/lightweightSynchronizer.cpp line 88:
>
>> 86:
>> 87: public:
>> 88: Lookup(oop obj) : _obj(obj) {}
>
> Make explicit?
Done.
> src/hotspot/share/runtime/lightweightSynchronizer.cpp line 97:
>
>> 95:
>> 96: bool equals(ObjectMonitor** value) {
>> 97: // The entry is going to be removed soon.
>
> What does this comment mean?
Not sure where it came from. Removed.
> src/hotspot/share/runtime/lightweightSynchronizer.cpp line 112:
>
>> 110:
>> 111: public:
>> 112: LookupMonitor(ObjectMonitor* monitor) : _monitor(monitor) {}
>
> Make explicit?
Done.
> 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.
> src/hotspot/share/runtime/lightweightSynchronizer.cpp line 349:
>
>> 347: assert(LockingMode == LM_LIGHTWEIGHT, "must be");
>> 348:
>> 349: if (try_read) {
>
> All the callers seem to pass try_read = true. Why do we have the branch at
> all?
I'll clean this up. From experiments if was never better to use `insert_get`
over a `get; insert_get`, even if we tried to be cleaver on when we skipped the
initial get.
> src/hotspot/share/runtime/lightweightSynchronizer.cpp line 401:
>
>> 399:
>> 400: if (inserted) {
>> 401: // Hopefully the performance counters are allocated on distinct
>
> It doesn't look like the counters are on distinct cache lines (see
> objectMonitor.hpp, lines 212ff). If this is a concern, file a bug to
> investigate it later? The comment here is a bit misplaced, IMO.
It originates from
https://github.com/openjdk/jdk/blob/15997bc3dfe9dddf21f20fa189f97291824892de/src/hotspot/share/runtime/synchronizer.cpp#L1543
I think we just kept it and did not think more about it.
Not sure what it is referring to. Maybe @dcubed-ojdk knows more, they
originated from him (9 years old comment).
> src/hotspot/share/runtime/lightweightSynchronizer.cpp line 477:
>
>> 475: if (obj->mark_acquire().has_monitor()) {
>> 476: if (_length > 0 && _contended_oops[_length-1] == obj) {
>> 477: // assert(VM_Version::supports_recursive_lightweight_locking(),
>> "must be");
>
> Uncomment or remove assert?
Yeah not sure why it was ever uncommented. To me it seems like that the assert
should be invariant. But will investigate.
> src/hotspot/share/runtime/lightweightSynchronizer.cpp line 554:
>
>> 552: bool _no_safepoint;
>> 553: union {
>> 554: struct {} _dummy;
>
> Uhh ... Why does this need to be wrapped in a union and struct?
A poor man's optional.
> 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.
> src/hotspot/share/runtime/lightweightSynchronizer.cpp line 748:
>
>> 746: }
>> 747:
>> 748: // Fast-locking does not use the 'lock' argument.
>
> I believe the comment is outdated.
Removed.
> src/hotspot/share/runtime/lightweightSynchronizer.cpp line 969:
>
>> 967:
>> 968: for (;;) {
>> 969: // Fetch the monitor from the table
>
> Wrong intendation.
Fixed.
> src/hotspot/share/runtime/lightweightSynchronizer.cpp line 1157:
>
>> 1155: // enter can block for safepoints; clear the unhandled object oop
>> 1156: PauseNoSafepointVerifier pnsv(&nsv);
>> 1157: object = nullptr;
>
> What is the point of that statement? object is not an out-arg (afaict), and
> not used subsequently.
`CHECK_UNHANDLED_OOPS` + `-XX:+CheckUnhandledOops`
https://github.com/openjdk/jdk/blob/15997bc3dfe9dddf21f20fa189f97291824892de/src/hotspot/share/oops/oopsHierarchy.hpp#L53-L55
> 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.
> src/hotspot/share/runtime/lockStack.inline.hpp line 232:
>
>> 230: oop obj = monitor->object_peek();
>> 231: assert(obj != nullptr, "must be alive");
>> 232: assert(monitor ==
>> LightweightSynchronizer::get_monitor_from_table(JavaThread::current(), obj),
>> "must be exist in table");
>
> "must be exist in table" -> "must exist in table"
Done.
> src/hotspot/share/runtime/objectMonitor.cpp line 56:
>
>> 54: #include "runtime/safepointMechanism.inline.hpp"
>> 55: #include "runtime/sharedRuntime.hpp"
>> 56: #include "runtime/synchronizer.hpp"
>
> This include is not used.
Removed.
> src/hotspot/share/runtime/objectMonitor.hpp line 193:
>
>> 191: ObjectWaiter* volatile _WaitSet; // LL of threads wait()ing on the
>> monitor
>> 192: volatile int _waiters; // number of waiting threads
>> 193: private:
>
> You can now also remove the 'private:' here
Done.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1677240569
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1677240591
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1677240598
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1677240629
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1677240633
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1677240644
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1677240655
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1677240709
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1677240664
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1677240684
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1677240695
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1677240712
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1677240735
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1677240747
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1677240787
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1677240807
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1677240936
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1677241002
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1677241011
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1677241037
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1677241082
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1677241093
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1677241121
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1677241145
