Avoid holding locks when calling ObjectFree callback
We were calling into agent code (through the ObjectFree event) while
holding the ObjectTagTable's allow_disallow_lock_ and other locks.
Depending on the GC being used, this could prevent other threads from
accessing tag-data while the ObjectFree is ongoing. Combined with the
ObjectFree event making use of jrawMonitorID locks (something they are
explicitly allowed to do) this could easily lead to deadlocks.
To fix this we made the ObjectTagTable delay sending the events until
locks are released after calling either the Allow or Broadcast
functions which notify the table that the sweep is finished. We did
need to modify the SemiSpace collector to call the Broadcast function
as well.
This has the unfortunate effect that previously all ObjectFree events
would occur before the end of the Runtime.gc function which is no
longer the case. This is all allowed by the spec however.
We needed to modify test 905 to check for gc having occurred using
phantom references since this change means that ObjectFree events
might happen after a Runtime.gc call returns.
Test: ./test/testrunner/run_build_test_target.py art-gss-gc
Test: ./test.py --host
Test: ./art/tools/run-libjdwp-tests.sh --mode=host
Bug: 76205593
Change-Id: I4565e9828d134899a1623c2f490ac183e7be5ab6
diff --git a/openjdkjvmti/object_tagging.cc b/openjdkjvmti/object_tagging.cc
index ba242ef..db67079 100644
--- a/openjdkjvmti/object_tagging.cc
+++ b/openjdkjvmti/object_tagging.cc
@@ -42,6 +42,34 @@
// Instantiate for jlong = JVMTI tags.
template class JvmtiWeakTable<jlong>;
+void ObjectTagTable::Allow() {
+ JvmtiWeakTable<jlong>::Allow();
+ SendDelayedFreeEvents();
+}
+
+void ObjectTagTable::Broadcast(bool broadcast_for_checkpoint) {
+ JvmtiWeakTable<jlong>::Broadcast(broadcast_for_checkpoint);
+ if (!broadcast_for_checkpoint) {
+ SendDelayedFreeEvents();
+ }
+}
+
+void ObjectTagTable::SendDelayedFreeEvents() {
+ std::vector<jlong> to_send;
+ {
+ art::MutexLock mu(art::Thread::Current(), lock_);
+ to_send.swap(null_tags_);
+ }
+ for (jlong t : to_send) {
+ SendSingleFreeEvent(t);
+ }
+}
+
+void ObjectTagTable::SendSingleFreeEvent(jlong tag) {
+ event_handler_->DispatchEventOnEnv<ArtJvmtiEvent::kObjectFree>(
+ jvmti_env_, art::Thread::Current(), tag);
+}
+
bool ObjectTagTable::Set(art::mirror::Object* obj, jlong new_tag) {
if (new_tag == 0) {
jlong tmp;
@@ -49,6 +77,7 @@
}
return JvmtiWeakTable<jlong>::Set(obj, new_tag);
}
+
bool ObjectTagTable::SetLocked(art::mirror::Object* obj, jlong new_tag) {
if (new_tag == 0) {
jlong tmp;
@@ -60,9 +89,10 @@
bool ObjectTagTable::DoesHandleNullOnSweep() {
return event_handler_->IsEventEnabledAnywhere(ArtJvmtiEvent::kObjectFree);
}
+
void ObjectTagTable::HandleNullSweep(jlong tag) {
- event_handler_->DispatchEventOnEnv<ArtJvmtiEvent::kObjectFree>(
- jvmti_env_, art::Thread::Current(), tag);
+ art::MutexLock mu(art::Thread::Current(), lock_);
+ null_tags_.push_back(tag);
}
} // namespace openjdkjvmti
diff --git a/openjdkjvmti/object_tagging.h b/openjdkjvmti/object_tagging.h
index 4181302..ca05a05 100644
--- a/openjdkjvmti/object_tagging.h
+++ b/openjdkjvmti/object_tagging.h
@@ -48,7 +48,18 @@
class ObjectTagTable final : public JvmtiWeakTable<jlong> {
public:
ObjectTagTable(EventHandler* event_handler, ArtJvmTiEnv* env)
- : event_handler_(event_handler), jvmti_env_(env) {}
+ : lock_("Object tag table lock", art::LockLevel::kGenericBottomLock),
+ event_handler_(event_handler),
+ jvmti_env_(env) {}
+
+ // Denotes that weak-refs are visible on all threads. Used by semi-space.
+ void Allow() override
+ REQUIRES_SHARED(art::Locks::mutator_lock_)
+ REQUIRES(!allow_disallow_lock_);
+ // Used by cms and the checkpoint system.
+ void Broadcast(bool broadcast_for_checkpoint) override
+ REQUIRES_SHARED(art::Locks::mutator_lock_)
+ REQUIRES(!allow_disallow_lock_);
bool Set(art::mirror::Object* obj, jlong tag) override
REQUIRES_SHARED(art::Locks::mutator_lock_)
@@ -77,6 +88,16 @@
void HandleNullSweep(jlong tag) override;
private:
+ void SendDelayedFreeEvents()
+ REQUIRES_SHARED(art::Locks::mutator_lock_)
+ REQUIRES(!allow_disallow_lock_);
+
+ void SendSingleFreeEvent(jlong tag)
+ REQUIRES_SHARED(art::Locks::mutator_lock_)
+ REQUIRES(!allow_disallow_lock_, !lock_);
+
+ art::Mutex lock_ BOTTOM_MUTEX_ACQUIRED_AFTER;
+ std::vector<jlong> null_tags_ GUARDED_BY(lock_);
EventHandler* event_handler_;
ArtJvmTiEnv* jvmti_env_;
};
diff --git a/runtime/gc/collector/semi_space.cc b/runtime/gc/collector/semi_space.cc
index 8cd484f..c58b59d 100644
--- a/runtime/gc/collector/semi_space.cc
+++ b/runtime/gc/collector/semi_space.cc
@@ -251,6 +251,7 @@
ReaderMutexLock mu(self_, *Locks::heap_bitmap_lock_);
SweepSystemWeaks();
}
+ Runtime::Current()->BroadcastForNewSystemWeaks();
Runtime::Current()->GetClassLinker()->CleanupClassLoaders();
// Revoke buffers before measuring how many objects were moved since the TLABs need to be revoked
// before they are properly counted.
diff --git a/test/905-object-free/src/art/Test905.java b/test/905-object-free/src/art/Test905.java
index 62b6e62..dddd1aa 100644
--- a/test/905-object-free/src/art/Test905.java
+++ b/test/905-object-free/src/art/Test905.java
@@ -16,10 +16,53 @@
package art;
+import java.lang.ref.PhantomReference;
+import java.lang.ref.ReferenceQueue;
import java.util.ArrayList;
import java.util.Arrays;
public class Test905 {
+ // Taken from jdwp tests.
+ public static class MarkerObj {
+ public static int cnt = 0;
+ public void finalize() { cnt++; }
+ }
+ public static class GcMarker {
+ private final ReferenceQueue mQueue;
+ private final ArrayList<PhantomReference> mList;
+ public GcMarker() {
+ mQueue = new ReferenceQueue();
+ mList = new ArrayList<PhantomReference>(3);
+ }
+ public void add(Object referent) {
+ mList.add(new PhantomReference(referent, mQueue));
+ }
+ public void waitForGc() {
+ waitForGc(mList.size());
+ }
+ public void waitForGc(int numberOfExpectedFinalizations) {
+ if (numberOfExpectedFinalizations > mList.size()) {
+ throw new IllegalArgumentException("wait condition will never be met");
+ }
+ // Request finalization of objects, and subsequent reference enqueueing.
+ // Repeat until reference queue reaches expected size.
+ do {
+ System.runFinalization();
+ Runtime.getRuntime().gc();
+ try { Thread.sleep(10); } catch (Exception e) {}
+ } while (isLive(numberOfExpectedFinalizations));
+ }
+ private boolean isLive(int numberOfExpectedFinalizations) {
+ int numberFinalized = 0;
+ for (int i = 0, n = mList.size(); i < n; i++) {
+ if (mList.get(i).isEnqueued()) {
+ numberFinalized++;
+ }
+ }
+ return numberFinalized < numberOfExpectedFinalizations;
+ }
+ }
+
public static void run() throws Exception {
doTest();
}
@@ -44,7 +87,7 @@
allocate(l, 1);
l.clear();
- Runtime.getRuntime().gc();
+ gcAndWait();
getAndPrintTags();
System.out.println("---");
@@ -56,12 +99,12 @@
}
l.clear();
- Runtime.getRuntime().gc();
+ gcAndWait();
getAndPrintTags();
System.out.println("---");
- Runtime.getRuntime().gc();
+ gcAndWait();
getAndPrintTags();
System.out.println("---");
@@ -80,7 +123,7 @@
for (int i = 1; i <= 100000; ++i) {
stressAllocate(i);
}
- Runtime.getRuntime().gc();
+ gcAndWait();
long[] freedTags1 = getCollectedTags(0);
long[] freedTags2 = getCollectedTags(1);
System.out.println("Free counts " + freedTags1.length + " " + freedTags2.length);
@@ -103,6 +146,17 @@
System.out.println(Arrays.toString(freedTags));
}
+ private static GcMarker getMarker() {
+ GcMarker m = new GcMarker();
+ m.add(new MarkerObj());
+ return m;
+ }
+
+ private static void gcAndWait() {
+ GcMarker marker = getMarker();
+ marker.waitForGc();
+ }
+
private static native void setupObjectFreeCallback();
private static native void enableFreeTracking(boolean enable);
private static native long[] getCollectedTags(int index);