From f4cdbf6a8676b8ada856b875074c54f70fa01313 Mon Sep 17 00:00:00 2001 From: Neethu Prasad Date: Fri, 17 May 2024 13:17:59 +0000 Subject: [PATCH 1/5] 8331911: Reconsider locking for recently disarmed nmethods --- .../gc/shenandoah/shenandoahBarrierSetNMethod.cpp | 9 ++++++++- src/hotspot/share/gc/x/xBarrierSetNMethod.cpp | 9 ++++++++- src/hotspot/share/gc/z/zBarrierSetNMethod.cpp | 10 +++++++++- 3 files changed, 25 insertions(+), 3 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahBarrierSetNMethod.cpp b/src/hotspot/share/gc/shenandoah/shenandoahBarrierSetNMethod.cpp index 20954156b9e4a..7c24b60969d24 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahBarrierSetNMethod.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahBarrierSetNMethod.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2019, 2022, Red Hat, Inc. All rights reserved. + * Copyright (c) 2019, 2024, Red Hat, Inc. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -36,6 +36,13 @@ #include "runtime/threadWXSetters.inline.hpp" bool ShenandoahBarrierSetNMethod::nmethod_entry_barrier(nmethod* nm) { + + if (!is_armed(nm)) { + // Some other thread got here first and healed the oops + // and disarmed the nmethod. + return true; + } + ShenandoahReentrantLock* lock = ShenandoahNMethod::lock_for_nmethod(nm); assert(lock != nullptr, "Must be"); ShenandoahReentrantLocker locker(lock); diff --git a/src/hotspot/share/gc/x/xBarrierSetNMethod.cpp b/src/hotspot/share/gc/x/xBarrierSetNMethod.cpp index 002d6bc00c5d7..4fd5704daf801 100644 --- a/src/hotspot/share/gc/x/xBarrierSetNMethod.cpp +++ b/src/hotspot/share/gc/x/xBarrierSetNMethod.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2018, 2022, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2018, 2024, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -32,6 +32,13 @@ #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. + return true; + } + XLocker locker(XNMethod::lock_for_nmethod(nm)); log_trace(nmethod, barrier)("Entered critical zone for %p", nm); diff --git a/src/hotspot/share/gc/z/zBarrierSetNMethod.cpp b/src/hotspot/share/gc/z/zBarrierSetNMethod.cpp index 2c58b0155648a..aa36d59bc1c3f 100644 --- a/src/hotspot/share/gc/z/zBarrierSetNMethod.cpp +++ b/src/hotspot/share/gc/z/zBarrierSetNMethod.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2018, 2023, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2018, 2024, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -37,6 +37,14 @@ #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)", p2i(nm)); + // Some other thread got here first and healed the oops + // and disarmed the nmethod. + return true; + } + ZLocker locker(ZNMethod::lock_for_nmethod(nm)); log_trace(nmethod, barrier)("Entered critical zone for %p", nm); From 5a0d5562ba0166e463fdeda0abc353ffa7cdafb9 Mon Sep 17 00:00:00 2001 From: Neethu Date: Fri, 17 May 2024 07:17:46 -0700 Subject: [PATCH 2/5] 8331911: Fix whitespace --- .../share/gc/shenandoah/shenandoahBarrierSetNMethod.cpp | 4 ++-- src/hotspot/share/gc/x/xBarrierSetNMethod.cpp | 4 ++-- src/hotspot/share/gc/z/zBarrierSetNMethod.cpp | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahBarrierSetNMethod.cpp b/src/hotspot/share/gc/shenandoah/shenandoahBarrierSetNMethod.cpp index 7c24b60969d24..50fbbd77b6b46 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahBarrierSetNMethod.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahBarrierSetNMethod.cpp @@ -36,13 +36,13 @@ #include "runtime/threadWXSetters.inline.hpp" bool ShenandoahBarrierSetNMethod::nmethod_entry_barrier(nmethod* nm) { - + if (!is_armed(nm)) { // Some other thread got here first and healed the oops // and disarmed the nmethod. return true; } - + ShenandoahReentrantLock* lock = ShenandoahNMethod::lock_for_nmethod(nm); assert(lock != nullptr, "Must be"); ShenandoahReentrantLocker locker(lock); diff --git a/src/hotspot/share/gc/x/xBarrierSetNMethod.cpp b/src/hotspot/share/gc/x/xBarrierSetNMethod.cpp index 4fd5704daf801..754edcc85bc6e 100644 --- a/src/hotspot/share/gc/x/xBarrierSetNMethod.cpp +++ b/src/hotspot/share/gc/x/xBarrierSetNMethod.cpp @@ -32,13 +32,13 @@ #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. return true; } - + XLocker locker(XNMethod::lock_for_nmethod(nm)); log_trace(nmethod, barrier)("Entered critical zone for %p", nm); diff --git a/src/hotspot/share/gc/z/zBarrierSetNMethod.cpp b/src/hotspot/share/gc/z/zBarrierSetNMethod.cpp index aa36d59bc1c3f..3a9ced564e935 100644 --- a/src/hotspot/share/gc/z/zBarrierSetNMethod.cpp +++ b/src/hotspot/share/gc/z/zBarrierSetNMethod.cpp @@ -37,14 +37,14 @@ #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)", p2i(nm)); // Some other thread got here first and healed the oops // and disarmed the nmethod. return true; } - + ZLocker locker(ZNMethod::lock_for_nmethod(nm)); log_trace(nmethod, barrier)("Entered critical zone for %p", nm); From 8c5bad2ee52f0aaf483c3b8b2a13c0c2d077f57d Mon Sep 17 00:00:00 2001 From: neethu-prasad Date: Thu, 23 May 2024 17:57:56 +0000 Subject: [PATCH 3/5] Address feedback on whitespace and comments --- .../gc/shenandoah/shenandoahBarrierSetNMethod.cpp | 9 ++++----- src/hotspot/share/gc/x/xBarrierSetNMethod.cpp | 9 ++++----- src/hotspot/share/gc/z/zBarrierSetNMethod.cpp | 11 +++++------ 3 files changed, 13 insertions(+), 16 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahBarrierSetNMethod.cpp b/src/hotspot/share/gc/shenandoah/shenandoahBarrierSetNMethod.cpp index 50fbbd77b6b46..7b30e1ecbac70 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahBarrierSetNMethod.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahBarrierSetNMethod.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2019, 2024, Red Hat, Inc. All rights reserved. + * Copyright (c) 2019, 2022, Red Hat, Inc. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -36,10 +36,9 @@ #include "runtime/threadWXSetters.inline.hpp" bool ShenandoahBarrierSetNMethod::nmethod_entry_barrier(nmethod* nm) { - if (!is_armed(nm)) { // Some other thread got here first and healed the oops - // and disarmed the nmethod. + // and disarmed the nmethod. No need to continue. return true; } @@ -48,8 +47,8 @@ bool ShenandoahBarrierSetNMethod::nmethod_entry_barrier(nmethod* nm) { ShenandoahReentrantLocker locker(lock); 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; } diff --git a/src/hotspot/share/gc/x/xBarrierSetNMethod.cpp b/src/hotspot/share/gc/x/xBarrierSetNMethod.cpp index 754edcc85bc6e..3dc76463028b8 100644 --- a/src/hotspot/share/gc/x/xBarrierSetNMethod.cpp +++ b/src/hotspot/share/gc/x/xBarrierSetNMethod.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2018, 2024, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2018, 2022, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -32,10 +32,9 @@ #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. + // and disarmed the nmethod. No need to continue. return true; } @@ -43,8 +42,8 @@ bool XBarrierSetNMethod::nmethod_entry_barrier(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; } diff --git a/src/hotspot/share/gc/z/zBarrierSetNMethod.cpp b/src/hotspot/share/gc/z/zBarrierSetNMethod.cpp index 3a9ced564e935..b8ecc3eddd3cd 100644 --- a/src/hotspot/share/gc/z/zBarrierSetNMethod.cpp +++ b/src/hotspot/share/gc/z/zBarrierSetNMethod.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2018, 2024, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2018, 2023, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -37,11 +37,10 @@ #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)", p2i(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. + // and disarmed the nmethod. No need to continue. return true; } @@ -52,8 +51,8 @@ bool ZBarrierSetNMethod::nmethod_entry_barrier(nmethod* 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; } From c59034dafb81f49c6106fc6df260ac76daf87922 Mon Sep 17 00:00:00 2001 From: Neethu Prasad Date: Wed, 29 May 2024 15:30:15 +0000 Subject: [PATCH 4/5] Update full name From d474c1b74e09fc2cc7eb96243b8ec7efbd042b04 Mon Sep 17 00:00:00 2001 From: neethu-prasad Date: Wed, 5 Jun 2024 20:40:49 +0000 Subject: [PATCH 5/5] 8331911: Remove is_armed check from callers --- src/hotspot/share/code/nmethod.cpp | 6 ++---- src/hotspot/share/gc/shared/barrierSetNMethod.cpp | 9 +++++++++ 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/src/hotspot/share/code/nmethod.cpp b/src/hotspot/share/code/nmethod.cpp index 8cc36ba1b64ad..92ee8714f2773 100644 --- a/src/hotspot/share/code/nmethod.cpp +++ b/src/hotspot/share/code/nmethod.cpp @@ -849,10 +849,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"); } } diff --git a/src/hotspot/share/gc/shared/barrierSetNMethod.cpp b/src/hotspot/share/gc/shared/barrierSetNMethod.cpp index 548e6b671eff0..79e3f47ed57bd 100644 --- a/src/hotspot/share/gc/shared/barrierSetNMethod.cpp +++ b/src/hotspot/share/gc/shared/barrierSetNMethod.cpp @@ -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 @@ -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)) { return 0; }