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
6 changes: 2 additions & 4 deletions src/hotspot/share/code/nmethod.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -844,10 +844,8 @@ void nmethod::run_nmethod_entry_barrier() {
// By calling this nmethod entry barrier, it plays along and acts
// like any other nmethod found on the stack of a thread (fewer surprises).
nmethod* nm = this;
if (bs_nm->is_armed(nm)) {
bool alive = bs_nm->nmethod_entry_barrier(nm);
assert(alive, "should be alive");
}
bool alive = bs_nm->nmethod_entry_barrier(nm);
assert(alive, "should be alive");
}
}

Expand Down
9 changes: 9 additions & 0 deletions src/hotspot/share/gc/shared/barrierSetNMethod.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,12 @@ bool BarrierSetNMethod::nmethod_entry_barrier(nmethod* nm) {
virtual void do_oop(narrowOop* p) { ShouldNotReachHere(); }
};

if (!is_armed(nm)) {
// Some other thread got here first and healed the oops
// and disarmed the nmethod. No need to continue.
return true;
}

// If the nmethod is the only thing pointing to the oops, and we are using a
// SATB GC, then it is important that this code marks them live.
// Also, with concurrent GC, it is possible that frames in continuation stack
Expand Down Expand Up @@ -172,6 +178,9 @@ int BarrierSetNMethod::nmethod_stub_entry_barrier(address* return_address_ptr) {
nmethod* nm = cb->as_nmethod();
BarrierSetNMethod* bs_nm = BarrierSet::barrier_set()->barrier_set_nmethod();

// Check for disarmed method here to avoid going into DeoptimizeNMethodBarriersALot code
// too often. nmethod_entry_barrier checks for disarmed status itself,
// but we have no visibility into whether the barrier acted or not.
if (!bs_nm->is_armed(nm)) {
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 still like this check gone, together with the comment above.

Copy link
Member

@shipilev shipilev Jun 6, 2024

Choose a reason for hiding this comment

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

I agree with Neethu, though: I think we should proceed to the DeoptimizeNMethodBarrierALot block and cross-modify fences only when barrier acted. We know from testing that it hurts otherwise. I would prefer this change not to introduce new performance potholes, even for verification/test code.

If the argument is cleanliness on who is checking "armed", and that we decide it should be solely in backend, then the middle ground might be adding the out-parameter, like nmethod_entry_barrier(nmethod* nm, bool has_acted), and checking that before proceeding here? That feels uglier than just leaving the check here.

return 0;
}
Expand Down
10 changes: 8 additions & 2 deletions src/hotspot/share/gc/x/xBarrierSetNMethod.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,18 @@
#include "runtime/threadWXSetters.inline.hpp"

bool XBarrierSetNMethod::nmethod_entry_barrier(nmethod* nm) {
if (!is_armed(nm)) {
// Some other thread got here first and healed the oops
// and disarmed the nmethod. No need to continue.
return true;
}

XLocker<XReentrantLock> locker(XNMethod::lock_for_nmethod(nm));
log_trace(nmethod, barrier)("Entered critical zone for %p", nm);

if (!is_armed(nm)) {
// Some other thread got here first and healed the oops
// and disarmed the nmethod.
// Some other thread managed to complete while we were
// waiting for lock. No need to continue.
return true;
}

Expand Down
11 changes: 9 additions & 2 deletions src/hotspot/share/gc/z/zBarrierSetNMethod.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,22 @@
#include "runtime/threadWXSetters.inline.hpp"

bool ZBarrierSetNMethod::nmethod_entry_barrier(nmethod* nm) {
if (!is_armed(nm)) {
log_develop_trace(gc, nmethod)("nmethod: " PTR_FORMAT " visited by entry (disarmed before lock)", p2i(nm));
// Some other thread got here first and healed the oops
// and disarmed the nmethod. No need to continue.
return true;
}

ZLocker<ZReentrantLock> locker(ZNMethod::lock_for_nmethod(nm));
log_trace(nmethod, barrier)("Entered critical zone for %p", nm);

log_develop_trace(gc, nmethod)("nmethod: " PTR_FORMAT " visited by entry (try)", p2i(nm));

if (!is_armed(nm)) {
log_develop_trace(gc, nmethod)("nmethod: " PTR_FORMAT " visited by entry (disarmed)", p2i(nm));
// Some other thread got here first and healed the oops
// and disarmed the nmethod.
// Some other thread managed to complete while we were
// waiting for lock. No need to continue.
return true;
}

Expand Down