On Tue, 13 Aug 2024 14:52:03 GMT, Axel Boldt-Christmas <abold...@openjdk.org>
wrote:
>> When inflating a monitor the `ObjectMonitor*` is written directly over the
>> `markWord` and any overwritten data is displaced into a displaced
>> `markWord`. This is problematic for concurrent GCs which needs extra care or
>> looser semantics to use this displaced data. In Lilliput this data also
>> contains the klass forcing this to be something that the GC has to take into
>> account everywhere.
>>
>> This patch introduces an alternative solution where locking only uses the
>> lock bits of the `markWord` and inflation does not override and displace the
>> `markWord`. This is done by keeping associations between objects and
>> `ObjectMonitor*` in an external hash table. Different caching techniques are
>> used to speedup lookups from compiled code.
>>
>> A diagnostic VM option is introduced called `UseObjectMonitorTable`. It is
>> only supported in combination with the LM_LIGHTWEIGHT locking mode (the
>> default).
>>
>> This patch has been evaluated to be performance neutral when
>> `UseObjectMonitorTable` is turned off (the default).
>>
>> Below is a more detailed explanation of this change and how `LM_LIGHTWEIGHT`
>> and `UseObjectMonitorTable` works.
>>
>> # Cleanups
>>
>> Cleaned up displaced header usage for:
>> * BasicLock
>> * Contains some Zero changes
>> * Renames one exported JVMCI field
>> * ObjectMonitor
>> * Updates comments and tests consistencies
>>
>> # Refactoring
>>
>> `ObjectMonitor::enter` has been refactored an a
>> `ObjectMonitorContentionMark` witness object has been introduced to the
>> signatures. Which signals that the contentions reference counter is being
>> held. More details are given below in the section about deflation.
>>
>> The initial purpose of this was to allow `UseObjectMonitorTable` to interact
>> more seamlessly with the `ObjectMonitor::enter` code.
>>
>> _There is even more `ObjectMonitor` refactoring which can be done here to
>> create a more understandable and enforceable API. There are a handful of
>> invariants / assumptions which are not always explicitly asserted which
>> could be trivially abstracted and verified by the type system by using
>> similar witness objects._
>>
>> # LightweightSynchronizer
>>
>> Working on adapting and incorporating the following section as a comment in
>> the source code
>>
>> ## Fast Locking
>>
>> CAS on locking bits in markWord.
>> 0b00 (Fast Locked) <--> 0b01 (Unlocked)
>>
>> When locking and 0b00 (Fast Locked) is observed, it may be beneficial to
>> avoid inflating by spinning a bit.
>>
>> If 0b10 (Inflated) is observed or there is to...
>
> Axel Boldt-Christmas has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - Remove the last OMWorld references
> - Rename omworldtable_work to object_monitor_table_work
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,
nit typo: s/does not/do not/
src/hotspot/share/runtime/basicLock.inline.hpp line 54:
> 52: inline void BasicLock::clear_object_monitor_cache() {
> 53: assert(UseObjectMonitorTable, "must be");
> 54: set_metadata(0);
Should this be a literal `0` or should it be `nullptr`?
Update: The metadata field is of type `unintptr_t`. Got it.
src/hotspot/share/runtime/deoptimization.cpp line 1650:
> 1648: mon_info->lock()->set_bad_metadata_deopt();
> 1649: }
> 1650: #endif
I like this!
src/hotspot/share/runtime/globals.hpp line 1964:
> 1962:
> \
> 1963: product(int, LightweightFastLockingSpins, 13, DIAGNOSTIC,
> \
> 1964: "Specifies the number of time lightweight fast locking will "
> \
nit typo: s/number of time/number of times/
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 34:
> 32: #include "oops/oop.inline.hpp"
> 33: #include "runtime/atomic.hpp"
> 34: #include "memory/allStatic.hpp"
nit: this include is out of order.
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 43:
> 41: #include "runtime/mutexLocker.hpp"
> 42: #include "runtime/objectMonitor.hpp"
> 43: #include "runtime/objectMonitor.inline.hpp"
Shouldn't have both includes here.
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 81:
> 79: oop _obj;
> 80:
> 81: public:
nit - please indent by one more space.
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 86:
> 84: uintx get_hash() const {
> 85: uintx hash = _obj->mark().hash();
> 86: assert(hash != 0, "should have a hash");
Hmmm... I can remember seeing hash values of zero in some
of my older legacy inflation stress runs. Is a hash value of zero
not a thing with lightweight locking?
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 104:
> 102: ObjectMonitor* _monitor;
> 103:
> 104: public:
nit - please indent by one more space.
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 126:
> 124:
> 125: static void dec_items_count() {
> 126: Atomic::inc(&_items_count);
Shouldn't this be `Atomic::dec`?
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 130:
> 128:
> 129: static double get_load_factor() {
> 130: return (double)_items_count/(double)_table_size;
nit - please add spaces around `/` operator.
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 169:
> 167: }
> 168:
> 169: public:
nit - please indent by one space.
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 184:
> 182: bool has_monitor = obj->mark().has_monitor();
> 183: assert(has_monitor == (monitor != nullptr),
> 184: "Inconsistency between markWord and OMW table has_monitor: %s
> monitor: " PTR_FORMAT,
Do you still want to use the name "OMW table"?
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 213:
> 211:
> 212: static bool should_shrink() {
> 213: // No implemented;
nit typo: s/No/Not/
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 322:
> 320: oop obj = om->object_peek();
> 321: st->print("monitor " PTR_FORMAT " ", p2i(om));
> 322: st->print("object " PTR_FORMAT, p2i(obj));
The monitor output style is to use `=` and commas, e.g.
`monitor=<value>, object=<value>`
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 341:
> 339:
> 340: ObjectMonitor*
> LightweightSynchronizer::get_or_insert_monitor_from_table(oop object,
> JavaThread* current, bool* inserted) {
> 341: assert(LockingMode == LM_LIGHTWEIGHT, "must be");
Do you want to assert: `inserted != nullptr`?
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 367:
> 365: ResourceMark rm(current);
> 366: log_trace(monitorinflation)("inflate: object=" INTPTR_FORMAT ",
> mark="
> 367: INTPTR_FORMAT ", type='%s' cause %s",
> p2i(object),
nit typo: s/cause %s/cause=%s/
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 414:
> 412:
> 413: intptr_t hash = obj->mark().hash();
> 414: assert(hash != 0, "must be set when claiming the object monitor");
Hmmm... I can remember seeing hash values of zero in some
of my older legacy inflation stress runs. Is a hash value of zero
not a thing with lightweight locking?
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 468:
> 466: oop obj = *o;
> 467: if (obj->mark_acquire().has_monitor()) {
> 468: if (_length > 0 && _contended_oops[_length-1] == obj) {
nit - please add space around `-` operator.
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 501:
> 499: // Make room on lock_stack
> 500: if (lock_stack.is_full()) {
> 501: // Inflate contented objects
nit typo: s/contented/contended/
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 545:
> 543: bool _no_safepoint;
> 544:
> 545: public:
nit - please indent by one space.
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 664:
> 662:
> 663: // Used when deflation is observed. Progress here requires progress
> 664: // from the deflator. After observing the that the deflator is not
nit typo: s/observing the that the deflator/observing that the deflator/
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 760:
> 758:
> 759: // LightweightSynchronizer::inflate_locked_or_imse is used to to get an
> inflated
> 760: // ObjectMonitor* with LM_LIGHTWEIGHT. It is used from contexts which
> requires
nit typo: s/used from contexts which requires/used from contexts which require/
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 773:
> 771: JavaThread* current = THREAD;
> 772:
> 773: for(;;) {
nit: please add space before `(`.
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 778:
> 776: // No lock, IMSE.
> 777: THROW_MSG_(vmSymbols::java_lang_IllegalMonitorStateException(),
> 778: "current thread is not owner", nullptr);
nit - please indent by one more space.
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 785:
> 783: // Fast locked by other thread, IMSE.
> 784: THROW_MSG_(vmSymbols::java_lang_IllegalMonitorStateException(),
> 785: "current thread is not owner", nullptr);
nit - please indent by one more space.
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 799:
> 797: if (lock_stack.contains(obj)) {
> 798: // Current thread owns the lock but someone else inflated
> 799: // fix owner and pop lock stack
Please consider:
// Current thread owns the lock but someone else inflated it.
// Fix owner and pop lock stack.
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 805:
> 803: // Fast locked (and inflated) by other thread, or deflation in
> progress, IMSE.
> 804: THROW_MSG_(vmSymbols::java_lang_IllegalMonitorStateException(),
> 805: "current thread is not owner", nullptr);
nit - please indent by one more space.
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 1045:
> 1043: if (monitor->is_being_async_deflated()) {
> 1044: // The MonitorDeflation thread is deflating the monitor. The
> locking thread
> 1045: // must spin until further progress have been made.
nit typo: s/progress have been made./progress has been made./
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 1075:
> 1073: // lock, then we make the locking_thread thread
> 1074: // the ObjectMonitor owner and remove the
> 1075: // lock from the locking_thread thread's lock
> stack.
nit typos: s/locking_thread thread/locking_thread/
in three places.
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 1190:
> 1188:
> 1189: #ifndef _LP64
> 1190: // Only for 32bit which have limited support for fast locking outside
> the runtime.
nit typo: s/which have limited support/which has limited support/
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 1194:
> 1192: // Recursive lock successful.
> 1193: current->inc_held_monitor_count();
> 1194: // Clears object monitor cache, because ?
What does this comment mean?
src/hotspot/share/runtime/lightweightSynchronizer.hpp line 36:
> 34:
> 35: class LightweightSynchronizer : AllStatic {
> 36: private:
nit: please indent by 1 space.
src/hotspot/share/runtime/lightweightSynchronizer.hpp line 41:
> 39:
> 40: static ObjectMonitor* add_monitor(JavaThread* current, ObjectMonitor*
> monitor, oop obj);
> 41: static bool remove_monitor(Thread* current, oop obj, ObjectMonitor*
> monitor);
Hmmm... `add_monitor` has `monitor` and then `obj` params.
`remove_monitor` has `obj` and then `monitor` params. Why
not use the same order?
src/hotspot/share/runtime/lightweightSynchronizer.hpp line 57:
> 55: static bool resize_table(JavaThread* current);
> 56:
> 57: private:
nit: please indent by 1 space.
src/hotspot/share/runtime/lightweightSynchronizer.hpp line 61:
> 59: static bool fast_lock_spin_enter(oop obj, LockStack& lock_stack,
> JavaThread* current, bool observed_deflation);
> 60:
> 61: public:
nit: please indent by 1 space.
src/hotspot/share/runtime/lightweightSynchronizer.hpp line 69:
> 67: static ObjectMonitor* inflate_locked_or_imse(oop object,
> ObjectSynchronizer::InflateCause cause, TRAPS);
> 68: static ObjectMonitor* inflate_fast_locked_object(oop object,
> JavaThread* locking_thread, JavaThread* current,
> ObjectSynchronizer::InflateCause cause);
> 69: static ObjectMonitor* inflate_and_enter(oop object, JavaThread*
> locking_thread, JavaThread* current, ObjectSynchronizer::InflateCause cause);
All of these are "inflate" functions, but:
- two of them have `object` parameter next to the `cause` parameter
- two of them have `object` parameter first
- one of them has `current` parameter before the other thread parameter
(`inflating_thread`)
- two of them have the `current` parameter after the other thread parameter
(`locking_thread`)
Please consider making the parameter order consistent.
src/hotspot/share/runtime/lockStack.hpp line 130:
> 128: class OMCache {
> 129: friend class VMStructs;
> 130: public:
Please indent by one space.
src/hotspot/share/runtime/lockStack.hpp line 133:
> 131: static constexpr int CAPACITY = 8;
> 132:
> 133: private:
Please indent by one space.
src/hotspot/share/runtime/lockStack.hpp line 140:
> 138: const oop _null_sentinel = nullptr;
> 139:
> 140: public:
Please indent by one space.
src/hotspot/share/runtime/objectMonitor.cpp line 669:
> 667: install_displaced_markword_in_object(obj);
> 668: }
> 669: }
This can be cleaned up by putting L664 and L665 on the same line,
fixing the indents on L666-7 and deleting L668.
src/hotspot/share/runtime/objectMonitor.cpp line 682:
> 680: // deflation process.
> 681: void ObjectMonitor::install_displaced_markword_in_object(const oop obj) {
> 682: assert(!UseObjectMonitorTable, "Lightweight has no dmw");
Perhaps:
`assert(!UseObjectMonitorTable, "ObjectMonitorTable has no dmw");`
src/hotspot/share/runtime/objectMonitor.hpp line 388:
> 386: // Deflation support
> 387: bool deflate_monitor(Thread* current);
> 388: private:
nit - please indent by one space
src/hotspot/share/runtime/serviceThread.cpp line 44:
> 42: #include "runtime/jniHandles.hpp"
> 43: #include "runtime/serviceThread.hpp"
> 44: #include "runtime/lightweightSynchronizer.hpp"
Include is in the wrong place.
src/hotspot/share/runtime/synchronizer.cpp line 404:
> 402:
> 403: bool ObjectSynchronizer::quick_enter_legacy(oop obj, JavaThread* current,
> 404: BasicLock * lock) {
nit - please fix this indent.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715569720
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715571831
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715582675
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715589490
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715617774
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715618379
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715621030
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715623399
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715640861
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715642307
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715643617
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715647133
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715649276
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715651080
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715661781
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715663965
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715662800
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715675411
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715678877
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715680995
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715682621
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715690997
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715696004
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715697549
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715698164
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715698568
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715700114
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715700801
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715716901
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715720857
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715724929
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715726115
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715603974
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715601643
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715604333
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715604553
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715614530
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715880925
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715881116
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715881379
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715922871
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715924876
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715900092
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715926418
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715940064
