Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions src/hotspot/share/oops/weakHandle.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ class WeakHandle {
inline oop peek() const;
void release(OopStorage* storage) const;
bool is_null() const { return _obj == nullptr; }
void set_null() { _obj = nullptr; }
Copy link
Contributor

Choose a reason for hiding this comment

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

I would be a lot happier if WeakHandle.release() was non-const and set the _obj to nullptr, so that nothing can do this without release the weak handle.
This could be a follow-up RFE though since it affects other WeakHandle.release() calls.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to be clear, the weak handle is released, but the _obj field in the WeakHandle is not
cleared so it is a dangling oop reference.

I didn't want to change release(OopStorage* storage) because that had a larger fan out
of changes outside the scope of what I was trying to accomplish.


void replace(oop with_obj);

Expand Down
8 changes: 7 additions & 1 deletion src/hotspot/share/runtime/objectMonitor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,10 @@ ObjectMonitor::ObjectMonitor(oop object) :
{ }

ObjectMonitor::~ObjectMonitor() {
_object.release(_oop_storage);
Copy link
Member Author

Choose a reason for hiding this comment

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

Just to be clear, we can't call release here because not all
delete monitor calls happen while the JavaThread is in VM.
Some happen while the JavaThread is blocked.

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk Jun 4, 2023

Choose a reason for hiding this comment

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

Now we use !_object.is_null() as a flag to indicate when it is
okay (and safe) to do the release from this location.

Update: s/(and safe)/(and presumed to be safe)/

Copy link
Member

Choose a reason for hiding this comment

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

This is confusing - why is is_null being used as a proxy for checking the current thread's state?

Copy link
Member Author

Choose a reason for hiding this comment

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

I look at it a little different, I guess. Here in the destructor we check if
the object's oop storage has already been released. If it has not, then
we do the release_object() call. This is like seeing if a buffer has
been already freed and taking care of the free if it is still needed. Here
I care about the _object's state (and not the thread's state).

I don't check the current thread's state here nor do I assert it. Based
on my testing, if this destructor is called from an unsafe context and
we need to do the release_object() call, the we'll fail a sanity check
down in the release_object() call stack.

Copy link
Member

Choose a reason for hiding this comment

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

Okay it was your "(and safe)" comment that implied to me that is_null was somehow also being used to infer the thread's state was safe to do the release.

Copy link
Member Author

Choose a reason for hiding this comment

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

I add a clarification to my comment above:

Update: s/(and safe)/(and presumed to be safe)/

if (!_object.is_null()) {
// Release object's oop storage if it hasn't already been done.
release_object();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

In what case, would the object's oop storage not already been released? Is the destructor called here because of GC, where the object has been collected?

Copy link
Member Author

Choose a reason for hiding this comment

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

There are a couple of places where thread-A allocates an ObjectMonitor for inflation,
but thread-A loses the race to install an ObjectMonitor to thread-B so the ObjectMonitor
allocated by thread-A has to be deleted/freed.

In an earlier version of the patch I had release_object() calls in both of those locations
and a couple of the reviewers thought that was "clunky".

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, thanks for the clarification. I agree with having it here in the destructor for that case.

}

oop ObjectMonitor::object() const {
Expand Down Expand Up @@ -595,6 +598,9 @@ bool ObjectMonitor::deflate_monitor() {
install_displaced_markword_in_object(obj);
}

// Release object's oop storage since the ObjectMonitor has been deflated:
release_object();
Copy link
Member Author

Choose a reason for hiding this comment

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

Just to be clear, deflate_monitor() is called when a JavaThread is
in VM (or by the VM Thread) so it is safe to call release.

Copy link
Contributor

Choose a reason for hiding this comment

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

I hope OopStorage.release() will assert if it's not safe. I think it does.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it will assert() when it is called from a not safe context.


// We leave owner == DEFLATER_MARKER and contentions < 0
// to force any racing threads to retry.
return true; // Success, ObjectMonitor has been deflated.
Expand Down
1 change: 1 addition & 0 deletions src/hotspot/share/runtime/objectMonitor.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,7 @@ class ObjectMonitor : public CHeapObj<mtObjectMonitor> {
// Deflation support
bool deflate_monitor();
void install_displaced_markword_in_object(const oop obj);
void release_object() { _object.release(_oop_storage); _object.set_null(); }
};

#endif // SHARE_RUNTIME_OBJECTMONITOR_HPP
41 changes: 32 additions & 9 deletions src/hotspot/share/runtime/synchronizer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1645,6 +1645,15 @@ class VM_RendezvousGCThreads : public VM_Operation {
};
};

static size_t delete_monitors(GrowableArray<ObjectMonitor*>* delete_list) {
size_t count = 0;
for (ObjectMonitor* monitor: *delete_list) {
delete monitor;
Copy link
Member Author

Choose a reason for hiding this comment

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

Just to be clear, this is the "delete monitor" call that is happening
while the calling JavaThread is in blocked state. If we did the release
of the oop in the ObjectMonitor in the ObjectMonitor's destructor, then
we would be calling release from blocked and not from thread-in-vm.

Copy link
Member

Choose a reason for hiding this comment

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

This comment doesn't make sense to me. We are calling release whilst blocked IIUC.

Copy link
Member Author

Choose a reason for hiding this comment

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

No we are not calling release while blocked. This function (delete_monitors()) is
passed an array of ObjectMonitors to delete (delete_list). All of the ObjectMonitors
on delete_list have been deflated which means that release_object() has already
been called for all of these ObjectMonitors.

Since release_object() has already been called, the _obj field in the WeakHandle
has been cleared AND _object.is_null() will be true so the destructor will skip the
call to release_object().

Copy link
Member

Choose a reason for hiding this comment

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

Ah that was the missing link. We delete deflated monitors and we release when deflating.

count++;
}
return count;
}

// This function is called by the MonitorDeflationThread to deflate
// ObjectMonitors. It is also called via do_final_audit_and_print_stats()
// and VM_ThreadDump::doit() by the VMThread.
Expand Down Expand Up @@ -1719,16 +1728,30 @@ size_t ObjectSynchronizer::deflate_idle_monitors(ObjectMonitorsHashtable* table)
}

// After the handshake, safely free the ObjectMonitors that were
// deflated in this cycle.
for (ObjectMonitor* monitor: delete_list) {
delete monitor;
deleted_count++;

if (current->is_Java_thread()) {
// A JavaThread must check for a safepoint/handshake and honor it.
chk_for_block_req(JavaThread::cast(current), "deletion", "deleted_count",
deleted_count, ls, &timer);
// deflated and unlinked in this cycle.
if (current->is_Java_thread()) {
if (ls != NULL) {
timer.stop();
ls->print_cr("before setting blocked: unlinked_count=" SIZE_FORMAT
", in_use_list stats: ceiling=" SIZE_FORMAT ", count="
SIZE_FORMAT ", max=" SIZE_FORMAT,
unlinked_count, in_use_list_ceiling(),
_in_use_list.count(), _in_use_list.max());
}
// Mark the calling JavaThread blocked (safepoint safe) while we free
// the ObjectMonitors so we don't delay safepoints whilst doing that.
ThreadBlockInVM tbivm(JavaThread::cast(current));
if (ls != NULL) {
ls->print_cr("after setting blocked: in_use_list stats: ceiling="
SIZE_FORMAT ", count=" SIZE_FORMAT ", max=" SIZE_FORMAT,
in_use_list_ceiling(), _in_use_list.count(), _in_use_list.max());
timer.start();
}
deleted_count = delete_monitors(&delete_list);
// ThreadBlockInVM is destroyed here
} else {
// A non-JavaThread can just free the ObjectMonitors:
deleted_count = delete_monitors(&delete_list);
}
assert(unlinked_count == deleted_count, "must be");
}
Expand Down