From 66767ec3fd3d19895c6a54d5b49fbd6d82ce973a Mon Sep 17 00:00:00 2001 From: Nikolay Igotti Date: Fri, 17 Aug 2018 17:47:35 +0300 Subject: [PATCH] Rework frozen lazy to explicitly prevent cyclic garbage. (#1894) --- backend.native/tests/build.gradle | 6 ++ backend.native/tests/runtime/workers/lazy1.kt | 17 +++++ runtime/src/main/cpp/Memory.cpp | 44 +++++++++++- .../main/kotlin/kotlin/native/worker/Lazy.kt | 62 ++++++++--------- .../main/kotlin/kotlin/native/worker/Lock.kt | 69 +++++++++++++++++++ 5 files changed, 165 insertions(+), 33 deletions(-) create mode 100644 backend.native/tests/runtime/workers/lazy1.kt create mode 100644 runtime/src/main/kotlin/kotlin/native/worker/Lock.kt diff --git a/backend.native/tests/build.gradle b/backend.native/tests/build.gradle index 02e1dcdd8c1..b44df4c7fea 100644 --- a/backend.native/tests/build.gradle +++ b/backend.native/tests/build.gradle @@ -724,6 +724,12 @@ task lazy0(type: RunKonanTest) { source = "runtime/workers/lazy0.kt" } +task lazy1(type: RunKonanTest) { + disabled = (project.testTarget == 'wasm32') // Need exceptions. + goldValue = "OK\n" + source = "runtime/workers/lazy1.kt" +} + task enumIdentity(type: RunKonanTest) { disabled = (project.testTarget == 'wasm32') // Workers need pthreads. goldValue = "true\n" diff --git a/backend.native/tests/runtime/workers/lazy1.kt b/backend.native/tests/runtime/workers/lazy1.kt new file mode 100644 index 00000000000..f46c2824e09 --- /dev/null +++ b/backend.native/tests/runtime/workers/lazy1.kt @@ -0,0 +1,17 @@ +package runtime.workers.lazy1 + +import kotlin.test.* + +import kotlin.native.worker.* + +class Leak { + val leak by lazy { this } +} + +@Test fun runTest() { + assertFailsWith { + for (i in 1..100) + Leak().freeze().leak + } + println("OK") +} diff --git a/runtime/src/main/cpp/Memory.cpp b/runtime/src/main/cpp/Memory.cpp index 6dd9a0c6f4e..e8c2e1bb0e1 100644 --- a/runtime/src/main/cpp/Memory.cpp +++ b/runtime/src/main/cpp/Memory.cpp @@ -447,6 +447,7 @@ namespace { template inline void traverseContainerObjectFields(ContainerHeader* container, func process) { + RuntimeAssert(!isAggregatingFrozenContainer(container), "Must not be called on such containers"); ObjHeader* obj = reinterpret_cast(container + 1); for (int object = 0; object < container->objectCount(); object++) { const TypeInfo* typeInfo = obj->type_info(); @@ -1753,7 +1754,7 @@ OBJ_GETTER(ReadRefLocked, ObjHeader** location, int32_t* spinlock) { return value; } -void EnsureNeverFrozen(KRef object) { +void EnsureNeverFrozen(ObjHeader* object) { if (object->container()->frozen()) ThrowFreezingException(object, object); // TODO: note, that this API could not not be called on frozen objects, so no need to care much about concurrency, @@ -1761,4 +1762,45 @@ void EnsureNeverFrozen(KRef object) { object->meta_object()->flags_ |= MF_NEVER_FROZEN; } +KBoolean Konan_ensureAcyclicAndSet(ObjHeader* where, KInt index, ObjHeader* what) { + RuntimeAssert(where->container()->frozen(), "Must be used on frozen objects only"); + RuntimeAssert(what == nullptr || what->container()->permanentOrFrozen(), + "Must be used with an immutable value"); + if (what != nullptr) { + // Now we check that `where` is not reachable from `what`. + // As we cannot modify objects while traversing, instead we remember all seen objects in a set. + KStdUnorderedSet seen; + KStdDeque queue; + queue.push_back(what->container()); + bool acyclic = true; + while (!queue.empty() && acyclic) { + ContainerHeader* current = queue.front(); + queue.pop_front(); + seen.insert(current); + if (isAggregatingFrozenContainer(current)) { + ContainerHeader** subContainer = reinterpret_cast(current + 1); + for (int i = 0; i < current->objectCount(); ++i) { + if (seen.count(*subContainer) == 0) + queue.push_back(*subContainer++); + } + } else { + traverseContainerReferredObjects(current, [where, &queue, &acyclic, &seen](ObjHeader* obj) { + if (obj == where) { + acyclic = false; + } else { + auto* objContainer = obj->container(); + if (seen.count(objContainer) == 0) + queue.push_back(objContainer); + } + }); + } + } + if (!acyclic) return false; + } + UpdateRef(reinterpret_cast( + reinterpret_cast(where + 1) + where->type_info()->objOffsets_[index]), what); + // Fence on updated location? + return true; +} + } // extern "C" diff --git a/runtime/src/main/kotlin/kotlin/native/worker/Lazy.kt b/runtime/src/main/kotlin/kotlin/native/worker/Lazy.kt index b154fe5bdad..0ff3b29e6fd 100644 --- a/runtime/src/main/kotlin/kotlin/native/worker/Lazy.kt +++ b/runtime/src/main/kotlin/kotlin/native/worker/Lazy.kt @@ -16,49 +16,47 @@ package kotlin.native.worker +import kotlin.native.internal.NoReorderFields + +@SymbolName("Konan_ensureAcyclicAndSet") +private external fun ensureAcyclicAndSet(where: Any, index: Int, what: Any?): Boolean + +@NoReorderFields internal class FreezeAwareLazyImpl(initializer: () -> T) : Lazy { - private var initializer_: (() -> T)? = initializer + // IMPORTANT: due to simplified ensureAcyclicAndSet() semantics fields here must be ordered like this, + // as an ordinal is used to refer a field. private var value_: Any? = UNINITIALIZED - // Objects are not frozen by default on single-threaded targets, shall they? - private val valueFrozen_ = AtomicReference(UNINITIALIZED.freeze()) + private var initializer_: (() -> T)? = initializer + private val lock_ = Lock() override val value: T get() { - var result: Any? = value_ if (isFrozen) { - if (result !== UNINITIALIZED) { - assert(result !== INITIALIZING) - @Suppress("UNCHECKED_CAST") - return result as T - } - // Barrier. - // Note that recursive lazy initializers will hang here. - do { - result = valueFrozen_.get() - } while (result === INITIALIZING) - - if (result !== UNINITIALIZED) { - @Suppress("UNCHECKED_CAST") - return result as T - } - // TODO: maybe release initializer in frozen case. - if (valueFrozen_.compareAndSwap(UNINITIALIZED, INITIALIZING) === UNINITIALIZED) { + locked(lock_) { + var result: Any? = value_ + if (result !== UNINITIALIZED) { + assert(result !== INITIALIZING) + @Suppress("UNCHECKED_CAST") + return result as T + } + // Set value_ to INITIALIZING. + ensureAcyclicAndSet(this, 0, INITIALIZING) result = initializer_!!().freeze() - val old = valueFrozen_.compareAndSwap(INITIALIZING, result) - assert(old === INITIALIZING) - } else { - do { - result = valueFrozen_.get() - } while (result === INITIALIZING) + // Set value_. + if (!ensureAcyclicAndSet(this, 0, result)) { + throw InvalidMutabilityException("Setting cyclic data via lazy in $this: $result") + } + // Clear initializer_ reference. + ensureAcyclicAndSet(this, 1, null) + @Suppress("UNCHECKED_CAST") + return result as T } - assert(result !== UNINITIALIZED && result !== INITIALIZING) - @Suppress("UNCHECKED_CAST") - return result as T } else { + var result: Any? = value_ if (result === UNINITIALIZED) { result = initializer_!!() if (isFrozen) - throw InvalidMutabilityException(this) + throw InvalidMutabilityException("$this got frozen during lazy evaluation" ) value_ = result initializer_ = null } @@ -68,7 +66,7 @@ internal class FreezeAwareLazyImpl(initializer: () -> T) : Lazy { } // Racy! - override fun isInitialized(): Boolean = (value_ !== UNINITIALIZED) || (valueFrozen_.get() !== UNINITIALIZED) + override fun isInitialized(): Boolean = (value_ !== UNINITIALIZED) && (value_ !== INITIALIZING) override fun toString(): String = if (isInitialized()) value.toString() else "Lazy value not initialized yet." diff --git a/runtime/src/main/kotlin/kotlin/native/worker/Lock.kt b/runtime/src/main/kotlin/kotlin/native/worker/Lock.kt new file mode 100644 index 00000000000..60e9d7bff27 --- /dev/null +++ b/runtime/src/main/kotlin/kotlin/native/worker/Lock.kt @@ -0,0 +1,69 @@ +/* + * Copyright 2010-2018 JetBrains s.r.o. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package kotlin.native.worker + +import kotlin.native.internal.Frozen + +@ThreadLocal +private object CurrentThread { + val id = Any().freeze() +} + +@Frozen +internal class Lock { + private val locker_ = AtomicInt(0) + private val reenterCount_ = AtomicInt(0) + + // TODO: make it properly reschedule instead of spinning. + fun lock() { + val lockData = CurrentThread.id.hashCode() + loop@ do { + val old = locker_.compareAndSwap(0, lockData) + when (old) { + lockData -> { + // Was locked by us already. + reenterCount_.increment() + break@loop + } + 0 -> { + // We just got the lock. + assert(reenterCount_.get() == 0) + break@loop + } + } + } while (true) + } + + fun unlock() { + if (reenterCount_.get() > 0) { + reenterCount_.decrement() + } else { + val lockData = CurrentThread.id.hashCode() + val old = locker_.compareAndSwap(lockData, 0) + assert(old == lockData) + } + } +} + +internal inline fun locked(lock: Lock, block: () -> R): R { + lock.lock() + try { + return block() + } finally { + lock.unlock() + } +} \ No newline at end of file