From 1df27a7884ede4a2e2893743c076704fa6e59001 Mon Sep 17 00:00:00 2001 From: Nikolay Igotti Date: Wed, 20 Jun 2018 14:36:16 +0300 Subject: [PATCH] Tweaks to atomics implementation. (#1703) --- backend.native/tests/build.gradle | 2 +- .../tests/runtime/workers/atomic0.kt | 17 +++- runtime/src/main/cpp/Atomic.cpp | 25 +++++- runtime/src/main/cpp/Memory.cpp | 67 +++++++++++++-- runtime/src/main/cpp/Memory.h | 5 ++ .../src/main/kotlin/konan/worker/Atomics.kt | 86 +++---------------- 6 files changed, 115 insertions(+), 87 deletions(-) diff --git a/backend.native/tests/build.gradle b/backend.native/tests/build.gradle index 9d2bd1744e3..0829f730b7c 100644 --- a/backend.native/tests/build.gradle +++ b/backend.native/tests/build.gradle @@ -673,7 +673,7 @@ task freeze2(type: RunKonanTest) { task atomic0(type: RunKonanTest) { disabled = (project.testTarget == 'wasm32') // Workers need pthreads. - goldValue = "35\n" + "20\n" + goldValue = "35\n" + "20\n" + "OK\n" source = "runtime/workers/atomic0.kt" } diff --git a/backend.native/tests/runtime/workers/atomic0.kt b/backend.native/tests/runtime/workers/atomic0.kt index 685d009fc7e..84b36b39c25 100644 --- a/backend.native/tests/runtime/workers/atomic0.kt +++ b/backend.native/tests/runtime/workers/atomic0.kt @@ -18,13 +18,19 @@ fun test1(workers: Array) { } fun test2(workers: Array) { - val atomic = AtomicInt(0) + val atomic = AtomicInt(1) val counter = AtomicInt(0) val futures = Array(workers.size, { workerIndex -> workers[workerIndex].schedule(TransferMode.CHECKED, { Triple(atomic, workerIndex, counter) }) { (place, index, result) -> - while (place.compareAndSwap(index, index + 1) != index) {} - result.increment() == index + 1 + // Here we simulate mutex using [place] location to store tag of the current worker. + // When it is negative - worker executes exclusively. + val tag = index + 1 + while (place.compareAndSwap(tag, -tag) != tag) {} + val ok1 = result.increment() == index + 1 + // Now, let the next worker run. + val ok2 = place.compareAndSwap(-tag, tag + 1) == -tag + ok1 && ok2 } }) futures.forEach { @@ -80,4 +86,9 @@ fun test4() { test2(workers) test3(workers) test4() + + workers.forEach { + it.requestTermination().consume { _ -> } + } + println("OK") } \ No newline at end of file diff --git a/runtime/src/main/cpp/Atomic.cpp b/runtime/src/main/cpp/Atomic.cpp index 0fd8941a3a0..c67d52ff222 100644 --- a/runtime/src/main/cpp/Atomic.cpp +++ b/runtime/src/main/cpp/Atomic.cpp @@ -16,10 +16,16 @@ #include "Atomic.h" #include "Common.h" +#include "Memory.h" #include "Types.h" namespace { +struct AtomicReferenceLayout { + KRef value_; + KInt lock_; +}; + template T addAndGetImpl(KRef thiz, T delta) { volatile T* location = reinterpret_cast(thiz + 1); return atomicAdd(location, delta); @@ -30,6 +36,10 @@ template T compareAndSwapImpl(KRef thiz, T expectedValue, T newValu return compareAndSwap(location, expectedValue, newValue); } +inline AtomicReferenceLayout* asAtomicReference(KRef thiz) { + return reinterpret_cast(thiz + 1); +} + } // namespace extern "C" { @@ -62,9 +72,20 @@ void Kotlin_AtomicReference_checkIfFrozen(KRef value) { } } -KRef Kotlin_AtomicReference_compareAndSwap(KRef thiz, KRef expectedValue, KRef newValue) { +OBJ_GETTER(Kotlin_AtomicReference_compareAndSwap, KRef thiz, KRef expectedValue, KRef newValue) { Kotlin_AtomicReference_checkIfFrozen(newValue); - return compareAndSwapImpl(thiz, expectedValue, newValue); + // See Kotlin_AtomicReference_get() for explanations, why locking is needed. + AtomicReferenceLayout* ref = asAtomicReference(thiz); + RETURN_RESULT_OF(SwapRefLocked, &ref->value_, expectedValue, newValue, &ref->lock_); +} + +OBJ_GETTER(Kotlin_AtomicReference_get, KRef thiz) { + // Here we must take a lock to prevent race when value, while taken here, is CASed and immediately + // destroyed by an another thread. AtomicReference no longer holds such an object, so if we got + // rescheduled unluckily, between the moment value is read from the field and RC is incremented, + // object may go away. + AtomicReferenceLayout* ref = asAtomicReference(thiz); + RETURN_RESULT_OF(ReadRefLocked, &ref->value_, &ref->lock_); } } // extern "C" \ No newline at end of file diff --git a/runtime/src/main/cpp/Memory.cpp b/runtime/src/main/cpp/Memory.cpp index 71362f8d100..2722444ff60 100644 --- a/runtime/src/main/cpp/Memory.cpp +++ b/runtime/src/main/cpp/Memory.cpp @@ -21,12 +21,12 @@ #include "Alloc.h" #include "Assert.h" +#include "Atomic.h" #include "Exceptions.h" #include "Memory.h" #include "MemoryPrivate.hpp" #include "Natives.h" #include "Porting.h" -#include "Atomic.h" // If garbage collection algorithm for cyclic garbage to be used. // We are using the Bacon's algorithm for GC, see @@ -397,6 +397,15 @@ inline FrameOverlay* asFrameOverlay(ObjHeader** slot) { inline bool isRefCounted(KConstRef object) { return isFreeable(object->container()); } + +inline void lock(KInt* spinlock) { + while (compareAndSwap(spinlock, 0, 1) != 0) {} +} + +inline void unlock(KInt* spinlock) { + RuntimeCheck(compareAndSwap(spinlock, 1, 0) == 1, "Must succeed"); +} + } // namespace extern "C" { @@ -1243,14 +1252,27 @@ void UpdateRef(ObjHeader** location, const ObjHeader* object) { } } -void UpdateReturnRef(ObjHeader** returnSlot, const ObjHeader* object) { - if (isArenaSlot(returnSlot)) { +inline ObjHeader** slotAddressFor(ObjHeader** returnSlot, const ObjHeader* value) { + if (!isArenaSlot(returnSlot)) return returnSlot; // Not a subject of reference counting. - if (object == nullptr || !isRefCounted(object)) return; - auto arena = initedArena(asArenaSlot(returnSlot)); - returnSlot = arena->getSlot(); + if (value == nullptr || !isRefCounted(value)) return nullptr; + return initedArena(asArenaSlot(returnSlot))->getSlot(); +} + +inline void updateReturnRefAdded(ObjHeader** returnSlot, const ObjHeader* value) { + returnSlot = slotAddressFor(returnSlot, value); + if (returnSlot == nullptr) return; + ObjHeader* old = *returnSlot; + *const_cast(returnSlot) = value; + if (old != nullptr) { + ReleaseRef(old); } - UpdateRef(returnSlot, object); +} + +void UpdateReturnRef(ObjHeader** returnSlot, const ObjHeader* value) { + returnSlot = slotAddressFor(returnSlot, value); + if (returnSlot == nullptr) return; + UpdateRef(returnSlot, value); } void UpdateRefIfNull(ObjHeader** location, const ObjHeader* object) { @@ -1652,4 +1674,35 @@ void MutationCheck(ObjHeader* obj) { if (obj->container()->frozen()) ThrowInvalidMutabilityException(); } +OBJ_GETTER(SwapRefLocked, + ObjHeader** location, ObjHeader* expectedValue, ObjHeader* newValue, int32_t* spinlock) { + lock(spinlock); + ObjHeader* oldValue = *location; + // We do not use UpdateRef() here to avoid having ReleaseRef() on return slot under lock. + if (oldValue == expectedValue) { + SetRef(location, newValue); + } else { + // We create an additional reference to the [oldValue] in the return slot. + if (oldValue != nullptr && isRefCounted(oldValue)) { + AddRef(oldValue); + } + } + unlock(spinlock); + // [oldValue] ownership was either transferred from *location to return slot if CAS succeeded, or + // we explicitly added a new reference if CAS failed. + updateReturnRefAdded(OBJ_RESULT, oldValue); + return oldValue; +} + +OBJ_GETTER(ReadRefLocked, ObjHeader** location, int32_t* spinlock) { + lock(spinlock); + ObjHeader* value = *location; + // We do not use UpdateRef() here to avoid having ReleaseRef() on return slot under lock. + if (value != nullptr) + AddRef(value); + unlock(spinlock); + updateReturnRefAdded(OBJ_RESULT, value); + return value; +} + } // extern "C" diff --git a/runtime/src/main/cpp/Memory.h b/runtime/src/main/cpp/Memory.h index e0c6d25f203..60c80f7b7f3 100644 --- a/runtime/src/main/cpp/Memory.h +++ b/runtime/src/main/cpp/Memory.h @@ -434,6 +434,11 @@ void UpdateRef(ObjHeader** location, const ObjHeader* object) RUNTIME_NOTHROW; void UpdateRefIfNull(ObjHeader** location, const ObjHeader* object) RUNTIME_NOTHROW; // Updates reference in return slot. void UpdateReturnRef(ObjHeader** returnSlot, const ObjHeader* object) RUNTIME_NOTHROW; +// Compares and swaps reference with taken lock. +OBJ_GETTER(SwapRefLocked, + ObjHeader** location, ObjHeader* expectedValue, ObjHeader* newValue, int32_t* spinlock) RUNTIME_NOTHROW; +// Reads reference with taken lock. +OBJ_GETTER(ReadRefLocked, ObjHeader** location, int32_t* spinlock) RUNTIME_NOTHROW; // Optimization: release all references in range. void ReleaseRefs(ObjHeader** start, int count) RUNTIME_NOTHROW; // Called on frame enter, if it has object slots. diff --git a/runtime/src/main/kotlin/konan/worker/Atomics.kt b/runtime/src/main/kotlin/konan/worker/Atomics.kt index 081b432ed12..9584a6ddb37 100644 --- a/runtime/src/main/kotlin/konan/worker/Atomics.kt +++ b/runtime/src/main/kotlin/konan/worker/Atomics.kt @@ -21,8 +21,7 @@ import konan.SymbolName import kotlinx.cinterop.NativePtr @Frozen -class AtomicInt(private var value: Int = 0) : Number() { - /* Atomic operations. */ +class AtomicInt(private var value: Int = 0) { /** * Increments the value by [delta] and returns the new value. @@ -52,47 +51,14 @@ class AtomicInt(private var value: Int = 0) : Number() { */ fun get(): Int = value - /* Operations from Number. */ - /** - * Returns the value of this number as a [Double], which may involve rounding. + * Returns the string representation of this object. */ - public override fun toDouble(): Double = value.toDouble() - - /** - * Returns the value of this number as a [Float], which may involve rounding. - */ - public override fun toFloat(): Float = value.toFloat() - - /** - * Returns the value of this number as a [Long], which may involve rounding or truncation. - */ - public override fun toLong(): Long = value.toLong() - - /** - * Returns the value of this number as an [Int], which may involve rounding or truncation. - */ - public override fun toInt(): Int = value.toInt() - - /** - * Returns the [Char] with the numeric value equal to this number, truncated to 16 bits if appropriate. - */ - public override fun toChar(): Char = value.toChar() - - /** - * Returns the value of this number as a [Short], which may involve rounding or truncation. - */ - public override fun toShort(): Short = value.toShort() - - /** - * Returns the value of this number as a [Byte], which may involve rounding or truncation. - */ - public override fun toByte(): Byte = value.toByte() + public override fun toString(): String = "AtomicInt $value" } @Frozen -class AtomicLong(private var value: Long = 0) : Number() { - /* Atomic operations. */ +class AtomicLong(private var value: Long = 0) { /** * Increments the value by [delta] and returns the new value. @@ -127,42 +93,10 @@ class AtomicLong(private var value: Long = 0) : Number() { */ fun get(): Long = value - /* Operations from Number. */ - /** - * Returns the value of this number as a [Double], which may involve rounding. + * Returns the string representation of this object. */ - public override fun toDouble(): Double = value.toDouble() - - /** - * Returns the value of this number as a [Float], which may involve rounding. - */ - public override fun toFloat(): Float = value.toFloat() - - /** - * Returns the value of this number as a [Long], which may involve rounding or truncation. - */ - public override fun toLong(): Long = value.toLong() - - /** - * Returns the value of this number as an [Int], which may involve rounding or truncation. - */ - public override fun toInt(): Int = value.toInt() - - /** - * Returns the [Char] with the numeric value equal to this number, truncated to 16 bits if appropriate. - */ - public override fun toChar(): Char = value.toChar() - - /** - * Returns the value of this number as a [Short], which may involve rounding or truncation. - */ - public override fun toShort(): Short = value.toShort() - - /** - * Returns the value of this number as a [Byte], which may involve rounding or truncation. - */ - public override fun toByte(): Byte = value.toByte() + public override fun toString(): String = "AtomicLong $value" } @Frozen @@ -185,6 +119,9 @@ external private fun checkIfFrozen(ref: Any?) @Frozen class AtomicReference(private var value: T? = null) { + // A spinlock to fix potential ARC race. Not an AtomicInt just for the effeciency sake. + private var lock: Int = 0 + /** * Creates a new atomic reference pointing to given [ref]. If reference is not frozen, * @InvalidMutabilityException is thrown. @@ -200,10 +137,11 @@ class AtomicReference(private var value: T? = null) { * Returns the old value. */ @SymbolName("Kotlin_AtomicReference_compareAndSwap") - external fun compareAndSwap(expected: T?, new: T?): T? + external public fun compareAndSwap(expected: T?, new: T?): T? /** * Returns the current value. */ - public fun get(): T? = value + @SymbolName("Kotlin_AtomicReference_get") + external public fun get(): T? } \ No newline at end of file