Skip to content

8339114: DaCapo xalan performance with -XX:+UseObjectMonitorTable #24098

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
Closed

8339114: DaCapo xalan performance with -XX:+UseObjectMonitorTable #24098

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
32235be
8339114: DaCapo xalan performance with -XX:+UseObjectMonitorTable
Mar 18, 2025
9a394d2
Remove _hit variable
Mar 18, 2025
6407833
Use read_monitor(mark)
Mar 19, 2025
15a54aa
Clear BasicLock OM cache when monitor is being deflated
Mar 26, 2025
fc5062a
Include objectMonitor.inline.hpp
Mar 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/hotspot/share/runtime/basicLock.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
#include "runtime/objectMonitor.hpp"
#include "runtime/synchronizer.hpp"

void BasicLock::print_on(outputStream* st, oop owner) const {
void BasicLock::print_on(outputStream* st, oop owner) {
st->print("monitor");
if (UseObjectMonitorTable) {
ObjectMonitor* mon = object_monitor_cache();
Expand Down
6 changes: 4 additions & 2 deletions src/hotspot/share/runtime/basicLock.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ class BasicLock {
static int metadata_offset_in_bytes() { return (int)offset_of(BasicLock, _metadata); }

public:
BasicLock() : _metadata(0) {}

// LM_MONITOR
void set_bad_metadata_deopt() { set_metadata(badDispHeaderDeopt); }

Expand All @@ -59,12 +61,12 @@ class BasicLock {
static int displaced_header_offset_in_bytes() { return metadata_offset_in_bytes(); }

// LM_LIGHTWEIGHT
inline ObjectMonitor* object_monitor_cache() const;
inline ObjectMonitor* object_monitor_cache();
inline void clear_object_monitor_cache();
inline void set_object_monitor_cache(ObjectMonitor* mon);
static int object_monitor_cache_offset_in_bytes() { return metadata_offset_in_bytes(); }

void print_on(outputStream* st, oop owner) const;
void print_on(outputStream* st, oop owner);

// move a basic lock (used during deoptimization)
void move_to(oop obj, BasicLock* dest);
Expand Down
10 changes: 8 additions & 2 deletions src/hotspot/share/runtime/basicLock.inline.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#define SHARE_RUNTIME_BASICLOCK_INLINE_HPP

#include "runtime/basicLock.hpp"
#include "runtime/objectMonitor.inline.hpp"

inline markWord BasicLock::displaced_header() const {
assert(LockingMode == LM_LEGACY, "must be");
Expand All @@ -37,10 +38,15 @@ inline void BasicLock::set_displaced_header(markWord header) {
Atomic::store(&_metadata, header.value());
}

inline ObjectMonitor* BasicLock::object_monitor_cache() const {
inline ObjectMonitor* BasicLock::object_monitor_cache() {
assert(UseObjectMonitorTable, "must be");
#if !defined(ZERO) && (defined(X86) || defined(AARCH64) || defined(RISCV64) || defined(PPC64) || defined(S390))
return reinterpret_cast<ObjectMonitor*>(get_metadata());
ObjectMonitor* monitor = reinterpret_cast<ObjectMonitor*>(get_metadata());
if (monitor != nullptr && monitor->is_being_async_deflated()) {
clear_object_monitor_cache();
return nullptr;
}
return monitor;
#else
// Other platforms do not make use of the cache yet,
// and are not as careful with maintaining the invariant
Expand Down
55 changes: 38 additions & 17 deletions src/hotspot/share/runtime/lightweightSynchronizer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -637,7 +637,7 @@ void LightweightSynchronizer::enter_for(Handle obj, BasicLock* lock, JavaThread*
} else {
do {
// It is assumed that enter_for must enter on an object without contention.
monitor = inflate_and_enter(obj(), ObjectSynchronizer::inflate_cause_monitor_enter, locking_thread, current);
monitor = inflate_and_enter(obj(), lock, ObjectSynchronizer::inflate_cause_monitor_enter, locking_thread, current);
// But there may still be a race with deflation.
} while (monitor == nullptr);
}
Expand Down Expand Up @@ -693,7 +693,7 @@ void LightweightSynchronizer::enter(Handle obj, BasicLock* lock, JavaThread* cur
spin_yield.wait();
}

ObjectMonitor* monitor = inflate_and_enter(obj(), ObjectSynchronizer::inflate_cause_monitor_enter, current, current);
ObjectMonitor* monitor = inflate_and_enter(obj(), lock, ObjectSynchronizer::inflate_cause_monitor_enter, current, current);
if (monitor != nullptr) {
cache_setter.set_monitor(monitor);
return;
Expand All @@ -707,7 +707,7 @@ void LightweightSynchronizer::enter(Handle obj, BasicLock* lock, JavaThread* cur
}
}

void LightweightSynchronizer::exit(oop object, JavaThread* current) {
void LightweightSynchronizer::exit(oop object, BasicLock* lock, JavaThread* current) {
assert(LockingMode == LM_LIGHTWEIGHT, "must be");
assert(current == Thread::current(), "must be");

Expand Down Expand Up @@ -742,7 +742,18 @@ void LightweightSynchronizer::exit(oop object, JavaThread* current) {

assert(mark.has_monitor(), "must be");
// The monitor exists
ObjectMonitor* monitor = ObjectSynchronizer::read_monitor(current, object, mark);
ObjectMonitor* monitor;
if (UseObjectMonitorTable) {
monitor = lock->object_monitor_cache();
if (monitor == nullptr) {
monitor = current->om_get_from_monitor_cache(object);
if (monitor == nullptr) {
monitor = get_monitor_from_table(current, object);
}
}
} else {
monitor = ObjectSynchronizer::read_monitor(mark);
}
if (monitor->has_anonymous_owner()) {
assert(current->lock_stack().contains(object), "current must have object on its lock stack");
monitor->set_owner_from_anonymous(current);
Expand Down Expand Up @@ -987,7 +998,7 @@ ObjectMonitor* LightweightSynchronizer::inflate_fast_locked_object(oop object, O
return monitor;
}

ObjectMonitor* LightweightSynchronizer::inflate_and_enter(oop object, ObjectSynchronizer::InflateCause cause, JavaThread* locking_thread, JavaThread* current) {
ObjectMonitor* LightweightSynchronizer::inflate_and_enter(oop object, BasicLock* lock, ObjectSynchronizer::InflateCause cause, JavaThread* locking_thread, JavaThread* current) {
assert(LockingMode == LM_LIGHTWEIGHT, "only used for lightweight");
VerifyThreadState vts(locking_thread, current);

Expand All @@ -1013,18 +1024,20 @@ ObjectMonitor* LightweightSynchronizer::inflate_and_enter(oop object, ObjectSync

NoSafepointVerifier nsv;

// Lightweight monitors require that hash codes are installed first
ObjectSynchronizer::FastHashCode(locking_thread, object);

// Try to get the monitor from the thread-local cache.
// There's no need to use the cache if we are locking
// on behalf of another thread.
if (current == locking_thread) {
monitor = current->om_get_from_monitor_cache(object);
monitor = lock->object_monitor_cache();
if (monitor == nullptr) {
monitor = current->om_get_from_monitor_cache(object);
}
}

// Get or create the monitor
if (monitor == nullptr) {
// Lightweight monitors require that hash codes are installed first
ObjectSynchronizer::FastHashCode(locking_thread, object);
monitor = get_or_insert_monitor(object, current, cause);
}

Expand Down Expand Up @@ -1172,9 +1185,6 @@ bool LightweightSynchronizer::quick_enter(oop obj, BasicLock* lock, JavaThread*
assert(obj != nullptr, "must be");
NoSafepointVerifier nsv;

// If quick_enter succeeds with entering, the cache should be in a valid initialized state.
CacheSetter cache_setter(current, lock);

LockStack& lock_stack = current->lock_stack();
if (lock_stack.is_full()) {
// Always go into runtime if the lock stack is full.
Expand All @@ -1201,17 +1211,28 @@ bool LightweightSynchronizer::quick_enter(oop obj, BasicLock* lock, JavaThread*
#endif

if (mark.has_monitor()) {
ObjectMonitor* const monitor = UseObjectMonitorTable ? current->om_get_from_monitor_cache(obj) :
ObjectSynchronizer::read_monitor(mark);
ObjectMonitor* monitor;
if (UseObjectMonitorTable) {
// C2 fast-path may have put the monitor in the cache in the BasicLock.
monitor = lock->object_monitor_cache();
if (monitor == nullptr) {
// Otherwise look up the monitor in the thread's OMCache.
monitor = current->om_get_from_monitor_cache(obj);
}
} else {
monitor = ObjectSynchronizer::read_monitor(mark);
}

if (monitor == nullptr) {
// Take the slow-path on a cache miss.
return false;
}

if (monitor->try_enter(current)) {
// ObjectMonitor enter successful.
cache_setter.set_monitor(monitor);
if (UseObjectMonitorTable) {
lock->set_object_monitor_cache(monitor);
Copy link
Contributor

@coleenp coleenp Mar 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hit a bug where this was a deflating monitor so I don't think we can do this.

Copy link
Contributor

@coleenp coleenp Mar 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

runtime/Monitor/UseObjectMonitorTableTest.java#ExtremeDeflation
This test failed when I updated the monitor via CacheSetter when we don't succeed to get the lock.

Copy link
Contributor Author

@rkennke rkennke Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if I understand the problem. Why can't we do that? We successfully looked up the monitor from the OMCache, why can't we stick it in the BasicLock cache, to avoid having to look it up in the OMCache again? Whatever code would pull it out of the BasicLock and observe a deflating monitor might also pull it out of the OMCache and observe a deflating monitor, or is that not correct? I know that OMCache::get_monitor() filters deflating monitors, but that is not a guarantee that a monitor would not flip to deflating after that check? So ... the user of the monitor would always have to deal with deflation after it has pulled a monitor from any of the caches anyway?

If it is our contract that only successfully locked monitors go into the BasicLock cache, then we can't do that optimization, I think.

Copy link
Contributor

@coleenp coleenp Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this is okay. My change that ran afoul of this test found the monitor in the first 32 entries of the ObjectSynchronizer::_in_use_list and filtered for deflating monitors also. Then put the monitor it found in the OMCache. Since it didn't help performance, I backed this out of my change. It may be safer to put the monitor in the BasicLock field because it's cleared. All this to say, I don't really know if this is right.

Copy link
Contributor

@coleenp coleenp Mar 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like from GHA, this also fails the same test. I think you should remove this optimization since I don't think it helps any.

Copy link
Contributor Author

@rkennke rkennke Mar 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'it does not help any' is not correct. The point of this PR is to put the OM in the BasicLock cache early, even if it does not succeed to enter, so that slower paths can pick it up from there. Also, simply removing this particular line would not change anything, because we put the OM in the BasicLock cache in the C2 fast-path in the same way.

I think the BasicLock OM cache should be treated the same way as the thread's OMCache. The BasicLock cache is basically the L1 cache, while the OMCache is the L2 cache. And just like the OMCache lookup, we should clear the BasicLock cache when we observe a deflating monitor. This fixes the failing test.
@xmas92 should also look at this.

Copy link
Contributor

@coleenp coleenp Mar 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the concept of a L1 and L2 cache is good and clear. And does help performance of look ups in the OMCache. I reconstructed the crash I got with the runtime/Monitor/UseObjectMonitorTableTest.java#ExtremeDeflation test. I was setting the OMCache also with the monitor found, because I was only trying to avoid redoing the lookup in the table itself, but in this test, we find a deflated monitor in the OMCache, and then got the assert that it's not in the table.

I don't know if you want to recheck if the monitor is deflated at this time, even though it might be racy. It's unlikely and the check and clearing in BasicLock is better.

}

if (monitor->spin_enter(current)) {
return true;
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/hotspot/share/runtime/lightweightSynchronizer.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,12 @@ class LightweightSynchronizer : AllStatic {
public:
static void enter_for(Handle obj, BasicLock* lock, JavaThread* locking_thread);
static void enter(Handle obj, BasicLock* lock, JavaThread* current);
static void exit(oop object, JavaThread* current);
static void exit(oop object, BasicLock* lock, JavaThread* current);

static ObjectMonitor* inflate_into_object_header(oop object, ObjectSynchronizer::InflateCause cause, JavaThread* locking_thread, Thread* current);
static ObjectMonitor* inflate_locked_or_imse(oop object, ObjectSynchronizer::InflateCause cause, TRAPS);
static ObjectMonitor* inflate_fast_locked_object(oop object, ObjectSynchronizer::InflateCause cause, JavaThread* locking_thread, JavaThread* current);
static ObjectMonitor* inflate_and_enter(oop object, ObjectSynchronizer::InflateCause cause, JavaThread* locking_thread, JavaThread* current);
static ObjectMonitor* inflate_and_enter(oop object, BasicLock* lock, ObjectSynchronizer::InflateCause cause, JavaThread* locking_thread, JavaThread* current);

static void deflate_monitor(Thread* current, oop obj, ObjectMonitor* monitor);

Expand Down
1 change: 1 addition & 0 deletions src/hotspot/share/runtime/objectMonitor.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ class ObjectWaiter : public CHeapObj<mtThread> {
#define OM_CACHE_LINE_SIZE DEFAULT_CACHE_LINE_SIZE

class ObjectMonitor : public CHeapObj<mtObjectMonitor> {
friend class LightweightSynchronizer;
friend class ObjectSynchronizer;
friend class ObjectWaiter;
friend class VMStructs;
Expand Down
3 changes: 2 additions & 1 deletion src/hotspot/share/runtime/synchronizer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -682,7 +682,8 @@ void ObjectSynchronizer::jni_enter(Handle obj, JavaThread* current) {
ObjectMonitor* monitor;
bool entered;
if (LockingMode == LM_LIGHTWEIGHT) {
entered = LightweightSynchronizer::inflate_and_enter(obj(), inflate_cause_jni_enter, current, current) != nullptr;
BasicLock lock;
entered = LightweightSynchronizer::inflate_and_enter(obj(), &lock, inflate_cause_jni_enter, current, current) != nullptr;
} else {
monitor = inflate(current, obj(), inflate_cause_jni_enter);
entered = monitor->enter(current);
Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/runtime/synchronizer.inline.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ inline void ObjectSynchronizer::exit(oop object, BasicLock* lock, JavaThread* cu
current->dec_held_monitor_count();

if (LockingMode == LM_LIGHTWEIGHT) {
LightweightSynchronizer::exit(object, current);
LightweightSynchronizer::exit(object, lock, current);
} else {
exit_legacy(object, lock, current);
}
Expand Down