RFR: 8315884: New Object to ObjectMonitor mapping [v15]
Daniel D. Daugherty
dcubed at openjdk.org
Tue Aug 13 22:16:10 UTC 2024
On Tue, 13 Aug 2024 14:52:03 GMT, Axel Boldt-Christmas <aboldtch at 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
More information about the core-libs-dev
mailing list