Fix 2042-reference-processing synchronization
The phantomRefs HashMap was not consistently lock-protected.
Change it to a ConcurrentHashMap instead and remove the lock.
We conjecture this caused the first bug listed below.
We initially thought this might also fix the second one and
tried to remove the TODO and re-enable that part of the test.
Unfortunately that again resulted in a very intermittent local
failure on host.
Add comments to the CC collector to explain a particularly
surprising use of memory_order_relaxed, which I keep having
to rejustify to myself.
Bug: 285914076
Bug: 216481630
Test: Treehugger
Change-Id: I2e5616c5ad0eab59f4a0e89d451e7e34a94448f3
diff --git a/runtime/gc/collector/concurrent_copying.cc b/runtime/gc/collector/concurrent_copying.cc
index 3e95871..18a4edc 100644
--- a/runtime/gc/collector/concurrent_copying.cc
+++ b/runtime/gc/collector/concurrent_copying.cc
@@ -205,6 +205,12 @@
break;
}
} while (!field->CasWeakRelaxed(from_ref, to_ref));
+ // "Relaxed" is not technically sufficient by C++ rules. However, we use a "release"
+ // operation to originally store the forwarding pointer, or a constructor fence if we
+ // directly obtained to_ref from Copy(). We then count on the fact that all later accesses
+ // to the to_ref object are data/address-dependent on the forwarding pointer, and there is
+ // no reasonable way for the compiler to eliminate that depenency. This is very similar to
+ // the reasoning we must use for final fields in any case.
}
} else {
// Used for preserving soft references, should be OK to not have a CAS here since there should be
@@ -3843,6 +3849,7 @@
break;
}
} while (!field->CasWeakRelaxed(from_ref, to_ref));
+ // See comment in MarkHeapReference() for memory ordering.
} else {
field->Assign(to_ref);
}
diff --git a/test/2042-reference-processing/src/Main.java b/test/2042-reference-processing/src/Main.java
index ed67052..c7f547f 100644
--- a/test/2042-reference-processing/src/Main.java
+++ b/test/2042-reference-processing/src/Main.java
@@ -21,7 +21,7 @@
import java.math.BigInteger;
import java.util.concurrent.ArrayBlockingQueue;
import java.util.concurrent.atomic.AtomicInteger;
-import java.util.HashMap;
+import java.util.concurrent.ConcurrentHashMap;
import java.util.TreeMap;
/**
@@ -48,7 +48,6 @@
static volatile boolean pleaseStop;
AtomicInteger totalFinalized = new AtomicInteger(0);
- Object phantomRefsLock = new Object();
int maxDropped = 0;
int liveObjects = 0;
@@ -66,7 +65,7 @@
// Maps from object number to Reference; Cleared references are deleted when queues are
// processed.
TreeMap<Integer, MyWeakReference> weakRefs = new TreeMap<>();
- HashMap<Integer, MyPhantomReference> phantomRefs = new HashMap<>();
+ ConcurrentHashMap<Integer, MyPhantomReference> phantomRefs = new ConcurrentHashMap<>();
class FinalizableObject {
int n;
@@ -101,16 +100,14 @@
}
}
boolean inPhantomRefs(int n) {
- synchronized(phantomRefsLock) {
- MyPhantomReference ref = phantomRefs.get(n);
- if (ref == null) {
- return false;
- }
- if (ref.n != n) {
- System.out.println("phantomRef retrieval failed");
- }
- return true;
+ MyPhantomReference ref = phantomRefs.get(n);
+ if (ref == null) {
+ return false;
}
+ if (ref.n != n) {
+ System.out.println("phantomRef retrieval failed");
+ }
+ return true;
}
void CheckOKToClearWeak(int num) {
@@ -197,9 +194,7 @@
int me = nextAllocated++;
listHead = new FinalizableObject(me, listHead);
weakRefs.put(me, new MyWeakReference(listHead));
- synchronized(phantomRefsLock) {
- phantomRefs.put(me, new MyPhantomReference(listHead));
- }
+ phantomRefs.put(me, new MyPhantomReference(listHead));
}
liveObjects += n;
}