From 1a2568df3845348c990dad8973e273ae8d1ca7b7 Mon Sep 17 00:00:00 2001 From: Nikolay Igotti Date: Fri, 20 Sep 2019 16:26:40 +0300 Subject: [PATCH] Fix KT-33824, ignore FreezableAtomicReference building condensation (#3365) --- .../tests/runtime/workers/freeze6.kt | 129 +++++++++++++++++- runtime/src/main/cpp/Memory.cpp | 95 +++++++++---- runtime/src/main/cpp/Types.h | 14 +- .../kotlin/native/concurrent/Atomics.kt | 2 + 4 files changed, 202 insertions(+), 38 deletions(-) diff --git a/backend.native/tests/runtime/workers/freeze6.kt b/backend.native/tests/runtime/workers/freeze6.kt index 935a2671237..e32450ea22b 100644 --- a/backend.native/tests/runtime/workers/freeze6.kt +++ b/backend.native/tests/runtime/workers/freeze6.kt @@ -7,6 +7,10 @@ package runtime.workers.freeze6 import kotlin.test.* import kotlin.native.concurrent.* +import kotlin.native.ref.* + +data class Hi(val s: String) +data class Nested(val hi: Hi) @Test fun ensureNeverFrozenNoFreezeChild(){ @@ -30,5 +34,126 @@ fun ensureNeverFrozenFailsTarget(){ println("OK") } -data class Hi(val s:String) -data class Nested(val hi:Hi) \ No newline at end of file +fun createRef1(): FreezableAtomicReference { + val ref = FreezableAtomicReference(null) + ref.value = ref + ref.freeze() + ref.value = null + return ref +} + +var global = 0 + +@Test +fun ensureFreezableHandlesCycles1() { + val ref = createRef1() + kotlin.native.internal.GC.collect() + + val obj: Any = ref + global = obj.hashCode() +} + +class Node(var ref: Any?) + +/** + * ref1 -> Node <- ref3 + * | /\ + * V | + * ref2 --- + */ +fun createRef2(): Pair, Any> { + val ref1 = FreezableAtomicReference(null) + + val node = Node(null) + val ref3 = FreezableAtomicReference(node) + val ref2 = FreezableAtomicReference(ref3) + + node.ref = ref2 + ref1.value = node + + ref1.freeze() + ref3.value = null + + assertTrue(node.isFrozen) + assertTrue(ref1.isFrozen) + assertTrue(ref2.isFrozen) + assertTrue(ref3.isFrozen) + + return ref1 to ref2 +} + +@Test +fun ensureFreezableHandlesCycles2() { + val (ref, obj) = createRef2() + kotlin.native.internal.GC.collect() + + assertTrue(obj.toString().length > 0) + global = ref.value!!.ref!!.hashCode() +} + +fun createRef3(): FreezableAtomicReference { + val ref = FreezableAtomicReference(null) + val node = Node(ref) + ref.value = node + ref.freeze() + + assertTrue(node.isFrozen) + assertTrue(ref.isFrozen) + + return ref +} + +@Test +fun ensureFreezableHandlesCycles3() { + val ref = createRef3() + ref.value = null + kotlin.native.internal.GC.collect() + + val obj: Any = ref + assertTrue(obj.toString().length > 0) + global = obj.hashCode() +} + +lateinit var weakRef: WeakReference + +fun createRef4(): FreezableAtomicReference { + val ref = FreezableAtomicReference(null) + val node = Node(ref) + weakRef = WeakReference(node) + ref.value = node + ref.freeze() + assertTrue(weakRef.get() != null) + return ref +} + +@Test +fun ensureWeakRefNotLeaks1() { + val ref = createRef4() + ref.value = null + // We cannot check weakRef.get() here, as value read will be stored in the stack slot, + // and thus hold weak reference from release. + kotlin.native.internal.GC.collect() + + assertTrue(weakRef.get() == null) +} + +lateinit var node1: Node +lateinit var weakNode2: WeakReference + +fun createRef5() { + val ref = FreezableAtomicReference(null) + node1 = Node(ref) + val node2 = Node(node1) + weakNode2 = WeakReference(node2) + ref.value = node2 + node1.freeze() + assertTrue(weakNode2.get() != null) + ref.value = null +} + +@Test +fun ensureWeakRefNotLeaks2() { + createRef5() + kotlin.native.internal.GC.collect() + assertTrue(weakNode2.get() == null) +} \ No newline at end of file diff --git a/runtime/src/main/cpp/Memory.cpp b/runtime/src/main/cpp/Memory.cpp index 896fb662b57..0fb39b167da 100644 --- a/runtime/src/main/cpp/Memory.cpp +++ b/runtime/src/main/cpp/Memory.cpp @@ -742,6 +742,16 @@ inline bool canFreeze(ContainerHeader* container) { return container != nullptr && !container->frozen(); } +inline bool isFreezableAtomic(ObjHeader* obj) { + return obj->type_info() == theFreezableAtomicReferenceTypeInfo; +} + +inline bool isFreezableAtomic(ContainerHeader* container) { + RuntimeAssert(!isAggregatingFrozenContainer(container), "Must be single object"); + ObjHeader* obj = reinterpret_cast(container + 1); + return isFreezableAtomic(obj); +} + ContainerHeader* allocContainer(MemoryState* state, size_t size) { ContainerHeader* result = nullptr; #if USE_GC @@ -943,7 +953,7 @@ void depthFirstTraversal(ContainerHeader* start, bool* hasCycles, continue; } toVisit.push_front(markAsRemoved(container)); - traverseContainerReferredObjects(container, [hasCycles, firstBlocker, &order, &toVisit](ObjHeader* obj) { + traverseContainerReferredObjects(container, [container, hasCycles, firstBlocker, &order, &toVisit](ObjHeader* obj) { if (*firstBlocker != nullptr) return; if (obj->has_meta_object() && ((obj->meta_object()->flags_ & MF_NEVER_FROZEN) != 0)) { @@ -959,7 +969,14 @@ void depthFirstTraversal(ContainerHeader* start, bool* hasCycles, if (!objContainer->seen() && !objContainer->marked()) { // Mark GRAY. objContainer->setSeen(); - toVisit.push_front(objContainer); + // Here we do rather interesting trick: when doing DFS we postpone processing references going from + // FreezableAtomic, so that in 'order' referred value will be seen as not actually belonging + // to the same SCC (unless there are other edges not going through FreezableAtomic reaching the same value). + if (isFreezableAtomic(container)) { + toVisit.push_back(objContainer); + } else { + toVisit.push_front(objContainer); + } } } }); @@ -1503,6 +1520,7 @@ void decrementStack(MemoryState* state) { while (current < end) { ObjHeader* obj = *current++; if (obj != nullptr) { + MEMORY_LOG("decrement stack %p\n", obj) auto* container = obj->container(); if (container != nullptr) enqueueDecrementRC(container); @@ -1566,6 +1584,13 @@ void garbageCollect(MemoryState* state, bool force) { } GC_LOG("GC: duration=%lld sinceLast=%lld\n", (gcEndTime - gcStartTime), gcStartTime - state->lastGcTimestamp); state->lastGcTimestamp = gcEndTime; + +#if TRACE_MEMORY + for (auto* obj: *state->toRelease) { + MEMORY_LOG("toRelease %p\n", obj) + } +#endif + GC_LOG("<<< GC: toFree %d toRelease %d\n", state->toFree->size(), state->toRelease->size()) } @@ -2262,56 +2287,65 @@ void freezeAcyclic(ContainerHeader* rootContainer, ContainerHeaderSet* newlyFroz } } -void freezeCyclic(ContainerHeader* rootContainer, +void freezeCyclic(ObjHeader* root, const KStdVector& order, ContainerHeaderSet* newlyFrozen) { KStdUnorderedMap> reversedEdges; - KStdDeque queue; - queue.push_back(rootContainer); + KStdDeque queue; + queue.push_back(root); while (!queue.empty()) { - ContainerHeader* current = queue.front(); + ObjHeader* current = queue.front(); queue.pop_front(); - current->unMark(); - reversedEdges.emplace(current, KStdVector(0)); - traverseContainerReferredObjects(current, [current, &queue, &reversedEdges](ObjHeader* obj) { + ContainerHeader* currentContainer = current->container(); + currentContainer->unMark(); + reversedEdges.emplace(currentContainer, KStdVector(0)); + traverseContainerReferredObjects(currentContainer, [current, currentContainer, &queue, &reversedEdges](ObjHeader* obj) { ContainerHeader* objContainer = obj->container(); if (canFreeze(objContainer)) { if (objContainer->marked()) - queue.push_back(objContainer); - reversedEdges.emplace(objContainer, KStdVector(0)).first->second.push_back(current); + queue.push_back(obj); + // We ignore references from FreezableAtomicsReference during condensation, to avoid KT-33824. + if (!isFreezableAtomic(current)) + reversedEdges.emplace(objContainer, KStdVector(0)). + first->second.push_back(currentContainer); } }); - } + } - KStdVector> components; - MEMORY_LOG("Condensation:\n"); - // Enumerate in the topological order. - for (auto it = order.rbegin(); it != order.rend(); ++it) { - auto* container = *it; - if (container->marked()) continue; - KStdVector component; - traverseStronglyConnectedComponent(container, &reversedEdges, &component); - MEMORY_LOG("SCC:\n"); + KStdVector> components; + MEMORY_LOG("Condensation:\n"); + // Enumerate in the topological order. + for (auto it = order.rbegin(); it != order.rend(); ++it) { + auto* container = *it; + if (container->marked()) continue; + KStdVector component; + traverseStronglyConnectedComponent(container, &reversedEdges, &component); + MEMORY_LOG("SCC:\n"); #if TRACE_MEMORY - for (auto c: component) - konan::consolePrintf(" %p\n", c); + for (auto c: component) + konan::consolePrintf(" %p\n", c); #endif - components.push_back(std::move(component)); - } + components.push_back(std::move(component)); + } - // Enumerate strongly connected components in reversed topological order. + // Enumerate strongly connected components in reversed topological order. for (auto it = components.rbegin(); it != components.rend(); ++it) { auto& component = *it; int internalRefsCount = 0; int totalCount = 0; for (auto* container : component) { + RuntimeAssert(!isAggregatingFrozenContainer(container), "Must not be called on such containers"); totalCount += container->refCount(); + if (isFreezableAtomic(container)) { + RuntimeAssert(component.size() == 1, "Must be trivial condensation"); + continue; + } traverseContainerReferredObjects(container, [&internalRefsCount](ObjHeader* obj) { auto* container = obj->container(); if (canFreeze(container)) - ++internalRefsCount; + ++internalRefsCount; }); - } + } // Freeze component. for (auto* container : component) { @@ -2327,11 +2361,12 @@ void freezeCyclic(ContainerHeader* rootContainer, // meta-object, where aggregating container is stored. container->setRefCount(0); } + // Create fictitious container for the whole component. auto superContainer = component.size() == 1 ? component[0] : allocAggregatingFrozenContainer(component); // Don't count internal references. MEMORY_LOG("Setting aggregating %p rc to %d (total %d inner %d)\n", \ - superContainer, totalCount - internalRefsCount, totalCount, internalRefsCount) + superContainer, totalCount - internalRefsCount, totalCount, internalRefsCount) superContainer->setRefCount(totalCount - internalRefsCount); newlyFrozen->insert(superContainer); } @@ -2382,7 +2417,7 @@ void freezeSubgraph(ObjHeader* root) { ContainerHeaderSet newlyFrozen; // Now unmark all marked objects, and freeze them, if no cycles detected. if (hasCycles) { - freezeCyclic(rootContainer, order, &newlyFrozen); + freezeCyclic(root, order, &newlyFrozen); } else { freezeAcyclic(rootContainer, &newlyFrozen); } diff --git a/runtime/src/main/cpp/Types.h b/runtime/src/main/cpp/Types.h index 88b61c8bfee..5ced20ab1cc 100644 --- a/runtime/src/main/cpp/Types.h +++ b/runtime/src/main/cpp/Types.h @@ -81,20 +81,22 @@ extern "C" { extern const TypeInfo* theAnyTypeInfo; extern const TypeInfo* theArrayTypeInfo; +extern const TypeInfo* theBooleanArrayTypeInfo; extern const TypeInfo* theByteArrayTypeInfo; extern const TypeInfo* theCharArrayTypeInfo; -extern const TypeInfo* theShortArrayTypeInfo; +extern const TypeInfo* theDoubleArrayTypeInfo; +extern const TypeInfo* theForeignObjCObjectTypeInfo; extern const TypeInfo* theIntArrayTypeInfo; extern const TypeInfo* theLongArrayTypeInfo; +extern const TypeInfo* theNativePtrArrayTypeInfo; extern const TypeInfo* theFloatArrayTypeInfo; -extern const TypeInfo* theDoubleArrayTypeInfo; -extern const TypeInfo* theBooleanArrayTypeInfo; +extern const TypeInfo* theForeignObjCObjectTypeInfo; +extern const TypeInfo* theFreezableAtomicReferenceTypeInfo; +extern const TypeInfo* theObjCObjectWrapperTypeInfo; +extern const TypeInfo* theShortArrayTypeInfo; extern const TypeInfo* theStringTypeInfo; extern const TypeInfo* theThrowableTypeInfo; extern const TypeInfo* theUnitTypeInfo; -extern const TypeInfo* theForeignObjCObjectTypeInfo; -extern const TypeInfo* theObjCObjectWrapperTypeInfo; -extern const TypeInfo* theNativePtrArrayTypeInfo; KBoolean IsInstance(const ObjHeader* obj, const TypeInfo* type_info) RUNTIME_PURE; void CheckCast(const ObjHeader* obj, const TypeInfo* type_info); diff --git a/runtime/src/main/kotlin/kotlin/native/concurrent/Atomics.kt b/runtime/src/main/kotlin/kotlin/native/concurrent/Atomics.kt index 71dec6ebbde..7ea41931502 100644 --- a/runtime/src/main/kotlin/kotlin/native/concurrent/Atomics.kt +++ b/runtime/src/main/kotlin/kotlin/native/concurrent/Atomics.kt @@ -5,6 +5,7 @@ package kotlin.native.concurrent +import kotlin.native.internal.ExportTypeInfo import kotlin.native.internal.Frozen import kotlin.native.internal.NoReorderFields import kotlin.native.SymbolName @@ -279,6 +280,7 @@ public class AtomicReference(private var value_: T) { * otherwise behaves as regular box for the value. */ @NoReorderFields +@ExportTypeInfo("theFreezableAtomicReferenceTypeInfo") public class FreezableAtomicReference(private var value_: T) { // A spinlock to fix potential ARC race. private var lock: Int = 0