diff --git a/test-app/runtime/src/main/java/com/tns/ObjectTable.java b/test-app/runtime/src/main/java/com/tns/ObjectTable.java index 4962d3488..eec847e5b 100644 --- a/test-app/runtime/src/main/java/com/tns/ObjectTable.java +++ b/test-app/runtime/src/main/java/com/tns/ObjectTable.java @@ -7,10 +7,12 @@ * holds its object strongly while the JS wrapper is reachable and through a WeakReference * after the wrapper was collected (until the object is collected too). * - * Where StampedLock exists (API 24+) any thread may read and findOrRegister: writers are - * serialized by its write lock and exact reads validate against it. Without it, every - * mutation and every exact read must come from the thread that holds the runtime's - * isolate lock. Neither table allocates per entry. + * Any thread may read and findOrRegister: every access synchronizes on the table. Neither + * table allocates per entry. + * + * A monitor rather than java.util.concurrent locks: those live in the boot classpath but + * outside the boot image, so on device they run interpreted, and an app's profile can't + * compile them. Uncontended monitor enter/exit is compiled into every caller. */ final class ObjectTable { static final int NO_ID = -1; @@ -22,116 +24,6 @@ interface IdSource { int newId(); } - // Null where StampedLock is unavailable (API < 24): findIdOptimistic then always misses. - private final Stamps stamps; - - ObjectTable() { - this(false); - } - - ObjectTable(boolean optimisticReads) { - stamps = optimisticReads ? new Stamps() : null; - } - - /** - * Writers are already serialized by the isolate lock; the write stamp only lets an - * optimistic reader detect that a write overlapped it. Kept in its own class so that - * runtimes where StampedLock is missing never load it. - */ - private static final class Stamps { - private final java.util.concurrent.locks.StampedLock lock = new java.util.concurrent.locks.StampedLock(); - - long beginWrite() { - return lock.writeLock(); - } - - void endWrite(long stamp) { - lock.unlockWrite(stamp); - } - - long beginRead() { - return lock.tryOptimisticRead(); - } - - long readLock() { - return lock.readLock(); - } - - void unlockRead(long stamp) { - lock.unlockRead(stamp); - } - - boolean validate(long stamp) { - return lock.validate(stamp); - } - } - - /** - * The id obj is registered under, from any thread, without the isolate lock. A miss - * (NO_ID) is not authoritative: it is also returned when a write overlapped the read. - * A hit was correct at the time of the read; the object may have been released since, - * which the caller must check under the isolate lock. - */ - int findIdOptimistic(Object obj) { - if (stamps == null) { - return NO_ID; - } - long stamp = stamps.beginRead(); - if (stamp == 0) { - return NO_ID; - } - int id; - try { - id = findIdUnlocked(obj); - } catch (RuntimeException e) { - // A torn view of a resize can index past an array; the stamp check below fails then. - id = NO_ID; - } - return stamps.validate(stamp) ? id : NO_ID; - } - - private int findIdUnlocked(Object obj) { - int[] index = identityIndex; - int[] hashes = identityIndexHashes; - int mask = index.length - 1; - int hash = System.identityHashCode(obj); - int i = mix(hash) & mask; - for (int probes = 0; probes <= mask; probes++) { - int entry = index[i]; - if (entry == 0) { - return NO_ID; - } - if (hashes[i] == hash && getUnlocked(entry - 1) == obj) { - return entry - 1; - } - i = (i + 1) & mask; - } - return NO_ID; - } - - // get() without the lock. Safe on a torn view: every array it reads is indexed within - // its own bounds, and a stale one only makes the caller's stamp check fail. - private Object getUnlocked(int id) { - if (id == NO_ID) { - return null; - } - Chunk[] cs = chunks; - int c = id >>> CHUNK_SHIFT; - if (c >= cs.length) { - return null; - } - Chunk chunk = cs[c]; - if (chunk == null) { - return null; - } - Leaf leaf = chunk.leaves[(id >>> LEAF_BITS) & CHUNK_MASK]; - if (leaf == null) { - return null; - } - Object value = leaf.values[id & LEAF_MASK]; - return value == null ? null : deref(value); - } - private static Object deref(Object value) { return value instanceof WeakRef ? ((WeakRef) value).get() : value; } @@ -177,15 +69,8 @@ private static final class Chunk { /** Registers obj under id, held strongly, replacing whatever id held before. */ void putStrong(int id, Object obj) { - if (stamps == null) { + synchronized (this) { putStrongLocked(id, obj); - return; - } - long stamp = stamps.beginWrite(); - try { - putStrongLocked(id, obj); - } finally { - stamps.endWrite(stamp); } } @@ -216,19 +101,13 @@ private void putStrongLocked(int id, Object obj, int hash) { identityCount++; } - boolean allowsConcurrentRegistration() { - return stamps != null; - } - /** * The id obj is registered under, registering it strongly under a new id from ids when * it has none; FRESH is set in the result when this call registered it. The lookup and - * the insert happen under one write lock, so concurrent calls agree on one id. Only - * when allowsConcurrentRegistration(). + * the insert happen under one lock, so concurrent calls agree on one id. */ long findOrRegister(Object obj, IdSource ids) { - long stamp = stamps.beginWrite(); - try { + synchronized (this) { int hash = System.identityHashCode(obj); int id = findIdLocked(obj, hash); if (id != NO_ID) { @@ -237,33 +116,17 @@ long findOrRegister(Object obj, IdSource ids) { id = ids.newId(); putStrongLocked(id, obj, hash); return (id & 0xFFFFFFFFL) | FRESH; - } finally { - stamps.endWrite(stamp); } } - /** The id obj is registered under, or NO_ID. */ + /** + * The id obj is registered under, or NO_ID. From a thread without the isolate lock, the + * object may have been released since, which the caller must check under the lock. + */ int findId(Object obj) { - if (stamps == null) { + synchronized (this) { return findIdLocked(obj); } - long stamp = stamps.beginRead(); - if (stamp != 0) { - try { - int id = findIdUnlocked(obj); - if (stamps.validate(stamp)) { - return id; - } - } catch (RuntimeException e) { - // A torn view of a resize; retried under the read lock. - } - } - stamp = stamps.readLock(); - try { - return findIdLocked(obj); - } finally { - stamps.unlockRead(stamp); - } } private int findIdLocked(Object obj) { @@ -288,25 +151,8 @@ private int findIdLocked(Object obj, int hash) { /** The object registered under id; null if there is none or it was collected. */ Object get(int id) { - if (stamps == null) { - return getLocked(id); - } - long stamp = stamps.beginRead(); - if (stamp != 0) { - try { - Object value = getUnlocked(id); - if (stamps.validate(stamp)) { - return value; - } - } catch (RuntimeException e) { - // A torn view of a resize; retried under the read lock. - } - } - stamp = stamps.readLock(); - try { + synchronized (this) { return getLocked(id); - } finally { - stamps.unlockRead(stamp); } } @@ -316,14 +162,8 @@ private Object getLocked(int id) { } boolean contains(int id) { - if (stamps == null) { - return rawLocked(id) != null; - } - long stamp = stamps.readLock(); - try { + synchronized (this) { return rawLocked(id) != null; - } finally { - stamps.unlockRead(stamp); } } @@ -332,35 +172,28 @@ boolean contains(int id) { * the object is still alive; an entry whose object was already collected is removed. */ boolean weakenAndCheckAlive(int id) { - if (stamps == null) { + synchronized (this) { return weakenAndCheckAliveLocked(id); } - long stamp = stamps.beginWrite(); - try { - return weakenAndCheckAliveLocked(id); - } finally { - stamps.endWrite(stamp); - } } /** - * weakenAndCheckAlive for ids[0..count), under one write lock; overwrites the id of - * every object no longer alive with NO_ID. + * weakenAndCheckAlive for ids[0..count), under one lock; overwrites the id of every + * object no longer alive with NO_ID. */ void weakenAndCheckAlive(int[] ids, int count) { if (count == 0) { return; } - long stamp = stamps == null ? 0 : stamps.beginWrite(); - try { - for (int i = 0; i < count; i++) { - if (!weakenAndCheckAliveLocked(ids[i])) { - ids[i] = NO_ID; - } - } - } finally { - if (stamps != null) { - stamps.endWrite(stamp); + synchronized (this) { + weakenAndCheckAliveAllLocked(ids, count); + } + } + + private void weakenAndCheckAliveAllLocked(int[] ids, int count) { + for (int i = 0; i < count; i++) { + if (!weakenAndCheckAliveLocked(ids[i])) { + ids[i] = NO_ID; } } } @@ -383,15 +216,8 @@ private boolean weakenAndCheckAliveLocked(int id) { } void remove(int id) { - if (stamps == null) { - removeLocked(id); - return; - } - long stamp = stamps.beginWrite(); - try { + synchronized (this) { removeLocked(id); - } finally { - stamps.endWrite(stamp); } } diff --git a/test-app/runtime/src/main/java/com/tns/Runtime.java b/test-app/runtime/src/main/java/com/tns/Runtime.java index 940a1997f..75ec2758c 100644 --- a/test-app/runtime/src/main/java/com/tns/Runtime.java +++ b/test-app/runtime/src/main/java/com/tns/Runtime.java @@ -181,9 +181,9 @@ public void run() { "Primitive types need to be manually wrapped in their respective Object wrappers.\n" + "If you are creating an instance of an inner class, make sure to always provide reference to the outer `this` as the first argument."; - // Any thread may read it optimistically and register new objects through findOrRegister - // (see ObjectTable); every other mutation and every exact read comes from the thread - // holding this runtime's isolate lock. + // Any thread may read it and register new objects through findOrRegister (see + // ObjectTable); every other mutation comes from the thread holding this runtime's + // isolate lock. private final ObjectTable objectTable; private final ObjectTable.IdSource objectIds = new ObjectTable.IdSource() { @@ -334,7 +334,7 @@ public Runtime(ClassResolver classResolver, GcListener gcListener, StaticConfigu } public Runtime(StaticConfiguration config, DynamicConfiguration dynamicConfiguration) { - this.objectTable = new ObjectTable(android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.N); + this.objectTable = new ObjectTable(); synchronized (Runtime.currentRuntime) { ManualInstrumentation.Frame frame = ManualInstrumentation.start("new Runtime"); try { @@ -1186,19 +1186,9 @@ private int getOrCreateJavaObjectID(Object obj) { // obj's id (low 32 bits), registering it strongly under a new id if it has none, with // ObjectTable.FRESH set then. Threads registering call arguments (packageArgs) do not - // hold the isolate lock, so where they may, the lookup and insert are one atomic step. + // hold the isolate lock, so the lookup and insert are one atomic step. private long getOrCreateObjectId(Object obj) { - // Exact: only the isolate-lock holder (this thread) removes entries. - int id = objectTable.findId(obj); - if (id != ObjectTable.NO_ID) { - return id & 0xFFFFFFFFL; - } - if (objectTable.allowsConcurrentRegistration()) { - return objectTable.findOrRegister(obj, objectIds); - } - id = generateNewObjectId(getRuntimeId()); - makeInstanceStrong(obj, id); - return (id & 0xFFFFFFFFL) | ObjectTable.FRESH; + return objectTable.findOrRegister(obj, objectIds); } public static Object callJSMethodFromPossibleNonMainThread(Object javaObject, String methodName, Class retType, Object... args) throws NativeScriptException { @@ -1383,11 +1373,8 @@ private PackagedArgs packageArgs(Object... args) { id = bound.getObjectId__ns(); } } - if (id == ObjectTable.NO_ID) { - id = objectTable.findIdOptimistic(value); - } long fresh = 0; - if (id == ObjectTable.NO_ID && objectTable.allowsConcurrentRegistration()) { + if (id == ObjectTable.NO_ID) { // Registered here rather than under the isolate lock, so threads calling // in with new objects only serialize on creating their wrappers. long found = objectTable.findOrRegister(value, objectIds); @@ -1396,8 +1383,6 @@ private PackagedArgs packageArgs(Object... args) { fresh = FRESH_ARG; } } - // Otherwise the id is a hint, resolved or registered natively under the - // isolate lock. values[i] = packIdAndClass(id, value) | fresh; } @@ -1513,7 +1498,7 @@ private Object dispatchCallJSMethodNative(final Object javaObject, int objectIdH // A hint: other threads may be writing the table, so native re-checks it under the // isolate lock and resolves the id itself on a miss. ret = callJSMethodNative(getRuntimeId(), javaObject, - objectIdHint != ObjectTable.NO_ID ? objectIdHint : objectTable.findIdOptimistic(javaObject), methodName, MethodNameIds.idOf(methodName), returnType, isConstructor, packagedArgs.count, packagedArgs.values, packagedArgs.refs, primitiveResult); + objectIdHint != ObjectTable.NO_ID ? objectIdHint : objectTable.findId(javaObject), methodName, MethodNameIds.idOf(methodName), returnType, isConstructor, packagedArgs.count, packagedArgs.values, packagedArgs.refs, primitiveResult); ret = takePrimitiveResult(ret, returnType, primitiveResult); } catch (NativeScriptException e) { // With multithreaded JS the owner can be torn down between the diff --git a/test-app/runtime/src/test/java/com/tns/ObjectTableTest.java b/test-app/runtime/src/test/java/com/tns/ObjectTableTest.java index f2d88ae41..e39fa3740 100644 --- a/test-app/runtime/src/test/java/com/tns/ObjectTableTest.java +++ b/test-app/runtime/src/test/java/com/tns/ObjectTableTest.java @@ -78,8 +78,8 @@ public void aRegisteredWeakReferenceIsNotMistakenForAWeakEntry() { } @Test - public void optimisticReadsNeverReturnAWrongIdWhileAnotherThreadWrites() throws Exception { - final ObjectTable table = new ObjectTable(true); + public void concurrentReadsFindStableEntriesWhileAnotherThreadWrites() throws Exception { + final ObjectTable table = new ObjectTable(); final int stableCount = 64; final Object[] stable = new Object[stableCount]; for (int i = 0; i < stableCount; i++) { @@ -124,11 +124,11 @@ public void run() { Random random = new Random(seed); while (!done.get()) { int expected = random.nextInt(stableCount); - int id = table.findIdOptimistic(stable[expected]); + int id = table.findId(stable[expected]); if (id == expected) { hits.incrementAndGet(); - } else if (id != ObjectTable.NO_ID) { - failure.compareAndSet(null, "expected " + expected + " or NO_ID, got " + id); + } else { + failure.compareAndSet(null, "expected " + expected + ", got " + id); } } } @@ -147,12 +147,12 @@ public void run() { } assertNull(failure.get(), failure.get()); - assertTrue("optimistic reads should mostly hit", hits.get() > 1000); + assertTrue("readers should have run", hits.get() > 1000); } @Test public void concurrentRegistrationsAgreeOnOneIdPerObject() throws Exception { - final ObjectTable table = new ObjectTable(true); + final ObjectTable table = new ObjectTable(); final java.util.concurrent.atomic.AtomicInteger nextId = new java.util.concurrent.atomic.AtomicInteger(); final ObjectTable.IdSource ids = new ObjectTable.IdSource() { @Override @@ -215,7 +215,7 @@ public void run() { @Test public void exactReadsStayExactWhileAnotherThreadRegisters() throws Exception { - final ObjectTable table = new ObjectTable(true); + final ObjectTable table = new ObjectTable(); final java.util.concurrent.atomic.AtomicInteger nextId = new java.util.concurrent.atomic.AtomicInteger(1000); final ObjectTable.IdSource ids = new ObjectTable.IdSource() { @Override @@ -309,7 +309,7 @@ public void keepsSurvivorsOfReleasedRangesAndOldIdsRegisteredLate() { @Test public void idsPastIntMaxAreUnsigned() { - ObjectTable table = new ObjectTable(true); + ObjectTable table = new ObjectTable(); Object a = new Object(); Object b = new Object(); table.putStrong(Integer.MIN_VALUE + 3, a); @@ -335,14 +335,6 @@ public int newId() { assertEquals(1, table.size()); } - @Test - public void optimisticReadsMissWhenDisabled() { - ObjectTable table = new ObjectTable(false); - Object a = new Object(); - table.putStrong(1, a); - assertEquals(ObjectTable.NO_ID, table.findIdOptimistic(a)); - } - @Test public void matchesReferenceMapsUnderRandomOperations() { Random random = new Random(42);