From 4f725387ff3b0fba7879b8cbd11bcd1a2bb91d75 Mon Sep 17 00:00:00 2001 From: Alexander Shabalin Date: Mon, 10 Aug 2020 12:04:41 +0300 Subject: [PATCH] Restore cycle detector (#4286) Reimagines cycle detector removed in "Remove obsolete leak detector. (#3819)" at aae8441b6ee4f394cdcc0184175b81f46dee45bd --- backend.native/tests/build.gradle | 6 + .../tests/runtime/memory/cycle_detector.kt | 193 +++++++++++++++ runtime/src/main/cpp/Memory.cpp | 228 ++++++++++++++++++ runtime/src/main/cpp/Types.h | 3 + .../kotlin/native/concurrent/Atomics.kt | 6 +- .../main/kotlin/kotlin/native/internal/GC.kt | 16 +- 6 files changed, 449 insertions(+), 3 deletions(-) create mode 100644 backend.native/tests/runtime/memory/cycle_detector.kt diff --git a/backend.native/tests/build.gradle b/backend.native/tests/build.gradle index 462cb8e32b4..3bd5610eb24 100644 --- a/backend.native/tests/build.gradle +++ b/backend.native/tests/build.gradle @@ -2824,6 +2824,12 @@ task memory_stable_ref_cross_thread_check(type: KonanLocalTest) { source = "runtime/memory/stable_ref_cross_thread_check.kt" } +standaloneTest("cycle_detector") { + disabled = project.globalTestArgs.contains('-opt') || (project.testTarget == 'wasm32') // Needs debug build. + flags = ['-tr', '-g'] + source = "runtime/memory/cycle_detector.kt" +} + standaloneTest("cycle_collector") { disabled = true // Needs USE_CYCLIC_GC, which is disabled. flags = ['-g'] diff --git a/backend.native/tests/runtime/memory/cycle_detector.kt b/backend.native/tests/runtime/memory/cycle_detector.kt new file mode 100644 index 00000000000..0f4113856e7 --- /dev/null +++ b/backend.native/tests/runtime/memory/cycle_detector.kt @@ -0,0 +1,193 @@ +import kotlin.native.concurrent.* +import kotlin.native.internal.GC +import kotlin.test.* + +class Holder(var other: Any?) + +class Holder2(var field1: Any?, var field2: Any?) + +val Array.description: String + get() { + val result = StringBuilder() + result.append('[') + for (elem in this) { + result.append(elem.toString()) + result.append(',') + } + result.append(']') + return result.toString() + } + +fun assertArrayEquals( + expected: Array, + actual: Array +): Unit { + val lazyMessage: () -> String? = { + "Expected <${expected.description}>, actual <${actual.description}>." + } + + asserter.assertTrue(lazyMessage, expected.size == actual.size) + for (i in expected.indices) { + asserter.assertTrue(lazyMessage, expected[i] == actual[i]) + } +} + +@Test +fun noCycles() { + val atomic1 = AtomicReference(null) + val atomic2 = AtomicReference(null) + try { + atomic1.value = atomic2 + val cycles = GC.detectCycles()!! + assertEquals(0, cycles.size) + assertNull(GC.findCycle(atomic1)); + assertNull(GC.findCycle(atomic2)); + } finally { + atomic1.value = null + atomic2.value = null + } +} + +@Test +fun oneCycle() { + val atomic = AtomicReference(null) + try { + atomic.value = atomic + val cycles = GC.detectCycles()!! + assertEquals(1, cycles.size) + assertArrayEquals(arrayOf(atomic, atomic), GC.findCycle(cycles[0])!!) + } finally { + atomic.value = null + } +} + +@Test +fun oneCycleWithHolder() { + val atomic = AtomicReference(null) + try { + atomic.value = Holder(atomic).freeze() + val cycles = GC.detectCycles()!! + assertEquals(1, cycles.size) + assertArrayEquals(arrayOf(atomic, atomic.value!!, atomic), GC.findCycle(cycles[0])!!) + assertArrayEquals(arrayOf(atomic.value!!, atomic, atomic.value!!), GC.findCycle(atomic.value!!)!!) + } finally { + atomic.value = null + } +} + +@Test +fun oneCycleWithArray() { + val array = arrayOf(AtomicReference(null), AtomicReference(null)) + try { + array[0].value = Holder(array).freeze() + val cycles = GC.detectCycles()!! + assertEquals(1, cycles.size) + assertArrayEquals(arrayOf(array[0], array[0].value!!, array, array[0]), GC.findCycle(cycles[0])!!) + } finally { + array[0].value = null + array[1].value = null + } +} + +@Test +fun oneCycleWithLongChain() { + val atomic = AtomicReference(null) + try { + val head = Holder(null) + var current = head + repeat(30) { + val next = Holder(null) + current.other = next + current = next + } + current.other = atomic + atomic.value = head.freeze() + val cycles = GC.detectCycles()!! + assertEquals(1, cycles.size) + val cycle = GC.findCycle(cycles[0])!! + assertEquals(33, cycle.size) + } finally { + atomic.value = null + } +} + +@Test +fun twoCycles() { + val atomic1 = AtomicReference(null) + val atomic2 = AtomicReference(null) + try { + atomic1.value = atomic2 + atomic2.value = atomic1 + val cycles = GC.detectCycles()!! + assertEquals(2, cycles.size) + assertArrayEquals(arrayOf(atomic2, atomic1, atomic2), GC.findCycle(cycles[0])!!) + assertArrayEquals(arrayOf(atomic1, atomic2, atomic1), GC.findCycle(cycles[1])!!) + } finally { + atomic1.value = null + atomic2.value = null + } +} + +@Test +fun twoCyclesWithHolder() { + val atomic1 = AtomicReference(null) + val atomic2 = AtomicReference(null) + try { + atomic1.value = atomic2 + atomic2.value = Holder(atomic1).freeze() + val cycles = GC.detectCycles()!! + assertEquals(2, cycles.size) + assertArrayEquals(arrayOf(atomic2, atomic2.value!!, atomic1, atomic2), GC.findCycle(cycles[0])!!) + assertArrayEquals(arrayOf(atomic1, atomic2, atomic2.value!!, atomic1), GC.findCycle(cycles[1])!!) + } finally { + atomic1.value = null + atomic2.value = null + } +} + +@Test +fun threeSeparateCycles() { + val atomic1 = AtomicReference(null) + val atomic2 = AtomicReference(null) + val atomic3 = AtomicReference(null) + try { + atomic1.value = atomic1 + atomic2.value = Holder2(atomic1, atomic2).freeze() + atomic3.value = Holder2(atomic3, atomic1).freeze() + val cycles = GC.detectCycles()!! + assertEquals(3, cycles.size) + assertArrayEquals(arrayOf(atomic3, atomic3.value!!, atomic3), GC.findCycle(cycles[0])!!) + assertArrayEquals(arrayOf(atomic2, atomic2.value!!, atomic2), GC.findCycle(cycles[1])!!) + assertArrayEquals(arrayOf(atomic1, atomic1), GC.findCycle(cycles[2])!!) + } finally { + atomic1.value = null + atomic2.value = null + atomic3.value = null + } +} + +@Test +fun noCyclesWithFreezableAtomicReference() { + val atomic = FreezableAtomicReference(null) + try { + atomic.value = atomic + val cycles = GC.detectCycles()!! + assertEquals(0, cycles.size) + } finally { + atomic.value = null + } +} + +@Test +fun oneCycleWithFrozenFreezableAtomicReference() { + val atomic = FreezableAtomicReference(null) + try { + atomic.value = atomic + atomic.freeze() + val cycles = GC.detectCycles()!! + assertEquals(1, cycles.size) + assertArrayEquals(arrayOf(atomic, atomic), GC.findCycle(cycles[0])!!) + } finally { + atomic.value = null + } +} diff --git a/runtime/src/main/cpp/Memory.cpp b/runtime/src/main/cpp/Memory.cpp index 36badfdc08f..0e6e5207c5c 100644 --- a/runtime/src/main/cpp/Memory.cpp +++ b/runtime/src/main/cpp/Memory.cpp @@ -35,6 +35,7 @@ #include "Natives.h" #include "Porting.h" #include "Runtime.h" +#include "Utils.h" #include "WorkerBoundReference.h" // If garbage collection algorithm for cyclic garbage to be used. @@ -132,6 +133,103 @@ volatile int aliveMemoryStatesCount = 0; KBoolean g_hasCyclicCollector = true; #endif // USE_CYCLIC_GC +// TODO: Consider using ObjHolder. +class ScopedRefHolder { + public: + ScopedRefHolder() = default; + + explicit ScopedRefHolder(KRef obj); + + ScopedRefHolder(const ScopedRefHolder&) = delete; + + ScopedRefHolder(ScopedRefHolder&& other) noexcept: obj_(other.obj_) { + other.obj_ = nullptr; + } + + ScopedRefHolder& operator=(const ScopedRefHolder&) = delete; + + ScopedRefHolder& operator=(ScopedRefHolder&& other) noexcept { + ScopedRefHolder tmp(std::move(other)); + swap(tmp); + return *this; + } + + ~ScopedRefHolder(); + + void swap(ScopedRefHolder& other) noexcept { + std::swap(obj_, other.obj_); + } + + private: + KRef obj_ = nullptr; +}; + +struct CycleDetectorRootset { + // Orders roots. + KStdVector roots; + // Pins a state of each root. + KStdUnorderedMap> rootToFields; + // Holding roots and their fields to avoid GC-ing them. + KStdVector heldRefs; +}; + +class CycleDetector { + public: + static void insertCandidateIfNeeded(KRef object) { + if (canBeACandidate(object)) + instance().insertCandidate(object); + } + + static void removeCandidateIfNeeded(KRef object) { + if (canBeACandidate(object)) + instance().removeCandidate(object); + } + + static CycleDetectorRootset collectRootset(); + + private: + CycleDetector() = default; + ~CycleDetector() = default; + CycleDetector(const CycleDetector&) = delete; + CycleDetector(CycleDetector&&) = delete; + CycleDetector& operator=(const CycleDetector&) = delete; + CycleDetector& operator=(CycleDetector&&) = delete; + + static CycleDetector& instance() { + // Only store a pointer to CycleDetector in .bss + static CycleDetector* result = new CycleDetector(); + return *result; + } + + static bool canBeACandidate(KRef object) { + return KonanNeedDebugInfo && + Kotlin_memoryLeakCheckerEnabled() && + (object->type_info()->flags_ & TF_LEAK_DETECTOR_CANDIDATE) != 0; + } + + void insertCandidate(KRef candidate) { + LockGuard guard(lock_); + + auto it = candidateList_.insert(candidateList_.begin(), candidate); + candidateInList_.emplace(candidate, it); + } + + void removeCandidate(KRef candidate) { + LockGuard guard(lock_); + + auto it = candidateInList_.find(candidate); + if (it == candidateInList_.end()) + return; + candidateList_.erase(it->second); + candidateInList_.erase(it); + } + + SimpleMutex lock_; + using CandidateList = KStdList; + CandidateList candidateList_; + KStdUnorderedMap candidateInList_; +}; + // TODO: can we pass this variable as an explicit argument? THREAD_LOCAL_VARIABLE MemoryState* memoryState = nullptr; THREAD_LOCAL_VARIABLE FrameOverlay* currentFrame = nullptr; @@ -967,6 +1065,7 @@ ALWAYS_INLINE void runDeallocationHooks(ContainerHeader* container) { cyclicRemoveAtomicRoot(obj); } #endif // USE_CYCLIC_GC + CycleDetector::removeCandidateIfNeeded(obj); if (obj->has_meta_object()) { ObjHeader::destroyMetaObject(&obj->typeInfoOrMeta_); } @@ -2036,6 +2135,7 @@ OBJ_GETTER(allocInstance, const TypeInfo* type_info) { makeShareable(container.header()); } #endif // USE_GC + CycleDetector::insertCandidateIfNeeded(obj); #if USE_CYCLIC_GC if ((obj->type_info()->flags_ & TF_LEAK_DETECTOR_CANDIDATE) != 0) { // Note: this should be performed after [rememberNewContainer] (above). @@ -2715,6 +2815,125 @@ void shareAny(ObjHeader* obj) { container->makeShared(); } +ScopedRefHolder::ScopedRefHolder(KRef obj): obj_(obj) { + if (obj_) { + addHeapRef(obj_); + } +} + +ScopedRefHolder::~ScopedRefHolder() { + if (obj_) { + ReleaseHeapRef(obj_); + } +} + +// static +CycleDetectorRootset CycleDetector::collectRootset() { + auto& detector = instance(); + CycleDetectorRootset rootset; + LockGuard guard(detector.lock_); + for (auto* candidate: detector.candidateList_) { + // Only frozen candidates are to be analyzed. + if (!isPermanentOrFrozen(candidate)) + continue; + rootset.roots.push_back(candidate); + rootset.heldRefs.emplace_back(candidate); + traverseReferredObjects(candidate, [&rootset, candidate](KRef field) { + rootset.rootToFields[candidate].push_back(field); + // TODO: There's currently a race here: + // some other thread might null this field and destroy it in GC before + // we put it in ScopedRefHolder. + rootset.heldRefs.emplace_back(field); + }); + } + return rootset; +} + +KStdVector findCycleWithDFS(KRef root, const CycleDetectorRootset& rootset) { + auto traverseFields = [&rootset](KRef obj, auto process) { + auto it = rootset.rootToFields.find(obj); + // If obj is in the rootset, use it's pinned state. + if (it != rootset.rootToFields.end()) { + const auto& fields = it->second; + for (KRef field: fields) { + if (field != nullptr) { + process(field); + } + } + return; + } + + traverseReferredObjects(obj, process); + }; + + KStdVector> toVisit; + auto appendFieldsToVisit = [&toVisit, &traverseFields](KRef obj, const KStdVector& currentPath) { + traverseFields(obj, [&toVisit, ¤tPath](KRef field) { + auto path = currentPath; + path.push_back(field); + toVisit.emplace_back(std::move(path)); + }); + }; + + appendFieldsToVisit(root, KRefList(1, root)); + + KStdUnorderedSet seen; + seen.insert(root); + while (!toVisit.empty()) { + KStdVector currentPath = std::move(toVisit.back()); + toVisit.pop_back(); + KRef node = currentPath[currentPath.size() - 1]; + + if (node == root) { + // Found a cycle. + return currentPath; + } + + // Already traversed this node. + if (seen.count(node) != 0) + continue; + seen.insert(node); + + appendFieldsToVisit(node, currentPath); + } + + return {}; +} + +template +OBJ_GETTER(createAndFillArray, const C& container) { + auto* result = AllocArrayInstance(theArrayTypeInfo, container.size(), OBJ_RESULT)->array(); + KRef* place = ArrayAddressOfElementAt(result, 0); + for (KRef it: container) { + UpdateHeapRef(place++, it); + } + RETURN_OBJ(result->obj()); +} + +OBJ_GETTER0(detectCyclicReferences) { + auto rootset = CycleDetector::collectRootset(); + + KStdVector cyclic; + + for (KRef root: rootset.roots) { + if (!findCycleWithDFS(root, rootset).empty()) { + cyclic.push_back(root); + } + } + + RETURN_RESULT_OF(createAndFillArray, cyclic); +} + +OBJ_GETTER(findCycle, KRef root) { + auto rootset = CycleDetector::collectRootset(); + + auto cycle = findCycleWithDFS(root, rootset); + if (cycle.empty()) { + RETURN_OBJ(nullptr); + } + RETURN_RESULT_OF(createAndFillArray, cycle); +} + } // namespace MetaObjHeader* ObjHeader::createMetaObject(TypeInfo** location) { @@ -3142,6 +3361,15 @@ KBoolean Kotlin_native_internal_GC_getTuneThreshold(KRef) { #endif } +OBJ_GETTER(Kotlin_native_internal_GC_detectCycles, KRef) { + if (!KonanNeedDebugInfo || !Kotlin_memoryLeakCheckerEnabled()) RETURN_OBJ(nullptr); + RETURN_RESULT_OF0(detectCyclicReferences); +} + +OBJ_GETTER(Kotlin_native_internal_GC_findCycle, KRef, KRef root) { + RETURN_RESULT_OF(findCycle, root); +} + KNativePtr CreateStablePointer(KRef any) { return createStablePointer(any); } diff --git a/runtime/src/main/cpp/Types.h b/runtime/src/main/cpp/Types.h index 6f64ce6ac42..a6ccd8c3468 100644 --- a/runtime/src/main/cpp/Types.h +++ b/runtime/src/main/cpp/Types.h @@ -25,6 +25,7 @@ #endif #include +#include #include #include #include @@ -78,6 +79,8 @@ template> using KStdOrderedMap = std::map>>; template using KStdVector = std::vector>; +template +using KStdList = std::list>; #ifdef __cplusplus extern "C" { diff --git a/runtime/src/main/kotlin/kotlin/native/concurrent/Atomics.kt b/runtime/src/main/kotlin/kotlin/native/concurrent/Atomics.kt index 52ba8af64a8..ed6ff354af8 100644 --- a/runtime/src/main/kotlin/kotlin/native/concurrent/Atomics.kt +++ b/runtime/src/main/kotlin/kotlin/native/concurrent/Atomics.kt @@ -216,7 +216,8 @@ private fun debugString(value: Any?): String { /** * An atomic reference to a frozen Kotlin object. Can be used in concurrent scenarious * but frequently shall be of nullable type and be zeroed out once no longer needed. - * Otherwise memory leak could happen if the atomic reference is a part of a reference cycle. + * Otherwise memory leak could happen. To detect such leaks [kotlin.native.internal.GC.detectCycles] + * in debug mode could be helpful. */ @Frozen @LeakDetectorCandidate @@ -293,7 +294,8 @@ public class AtomicReference { /** * An atomic reference to a Kotlin object. Can be used in concurrent scenarious, but must be frozen first, * otherwise behaves as regular box for the value. If frozen, shall be zeroed out once no longer needed. - * Otherwise memory leak could happen if atomic reference is a part of a reference cycle. + * Otherwise memory leak could happen. To detect such leaks [kotlin.native.internal.GC.detectCycles] + * in debug mode could be helpful. */ @NoReorderFields @LeakDetectorCandidate diff --git a/runtime/src/main/kotlin/kotlin/native/internal/GC.kt b/runtime/src/main/kotlin/kotlin/native/internal/GC.kt index 5fc746c39d3..fd52d6ceeee 100644 --- a/runtime/src/main/kotlin/kotlin/native/internal/GC.kt +++ b/runtime/src/main/kotlin/kotlin/native/internal/GC.kt @@ -100,6 +100,20 @@ object GC { get() = getCyclicCollectorEnabled() set(value) = setCyclicCollectorEnabled(value) + /** + * Detect cyclic references going via atomic references and return list of cycle-inducing objects + * or `null` if the leak detector is not available. Use [Platform.isMemoryLeakCheckerActive] to check + * leak detector availability. + */ + @SymbolName("Kotlin_native_internal_GC_detectCycles") + external fun detectCycles(): Array? + + /** + * Find a reference cycle including from the given object, `null` if no cycles detected. + */ + @SymbolName("Kotlin_native_internal_GC_findCycle") + external fun findCycle(root: Any): Array? + @SymbolName("Kotlin_native_internal_GC_getThreshold") private external fun getThreshold(): Int @@ -129,4 +143,4 @@ object GC { @SymbolName("Kotlin_native_internal_GC_setCyclicCollector") private external fun setCyclicCollectorEnabled(value: Boolean) -} \ No newline at end of file +}