Incorporate new Native memory model into kotlinx-coroutines mainline #2833

qwwdfsad · 2021-07-16T15:47:43Z

Changeset:

When the new memory model is disabled, behave the same way as before

For the new memory model:

Concurrent tests shared with JVM (disabled for old MM)
Dispatchers.Default backed by multiple workers on Linux/Windows and global_queue on OS X
Dispatchers.Main on OS X and iOS
newSingleThreadContext and newFixedThreadPoolContext on Native, shared declaration with JVM

* Dispatchers.Default backed by pool of workers on Linux and by global_queue on iOS-like * Implementation of Dispatchers.Main that uses main queue on iOS and default dispatcher on other platforms (#2858) * Introduced newSingleThreadDispatcher and newFixedThreadPoolDispatcher * Use proper reentrant locking and CoW arrays on new memory model, make TestBase _almost_ race-free * More thread-safety in Native counterpart and one more test from native-mt * Source-set sharing for tests shared between JVM and K/N * Wrap Obj-C interop into autorelease pool to avoid memory leaks

* Dispatchers.Default cannot be closed * Throwins Dispatchers.Main on Linux and Windows

* SharedImmutable where necessary * Disable MainDispatcherTest with old MM

dkhalanskyjb

This is just the first batch of comments so there's something to discuss while I check the rest of this.

gradle.properties

kotlinx-coroutines-core/common/src/CoroutineDispatcher.kt

kotlinx-coroutines-core/concurrent/test/flow/CombineStressTest.kt

dkhalanskyjb · 2021-11-10T08:15:53Z

kotlinx-coroutines-core/concurrent/test/AtomicCancellationTest.kt

@@ -85,7 +85,7 @@ class AtomicCancellationTest : TestBase() {
    }

    @Test
-    fun testSelectDeferredAwaitCancellable() = runBlocking {
+    fun testSelectDeferredAwaitCancellable() = runMtTest {


Below this test, there's another one with runBlocking. Shouldn't it also be changed?

Seems like all these tests should be indeed runBlocking

Do you mean, the one below should be runBlocking, but the other ones are fine with runMtTest?

All these tests have to be tested with runBlocking as neither of the tests implies actual multithreading

kotlinx-coroutines-core/native/test/WorkerTest.kt

dkhalanskyjb · 2021-11-10T09:41:01Z

kotlinx-coroutines-core/native/src/internal/CopyOnWriteList.kt

-
-    private class IteratorImpl<E>(private var array: Array<E>, private val size: Int) : MutableIterator<E> {
-
+    private class IteratorImpl<E>(private val array: Array<E>) : MutableIterator<E> {


Historical note: it looks like the deoptimization here was introduced because it would be difficult to support now that this data structure needs to use an atomic. This is not a huge deal, as this is only ever used to store the list of subscribers for a buffered broadcast channel, so we don't expect to be thousands of subscribers here.

Now, I have some problems with this. CopyOnWriteList is not properly lock-free. Concurrent add and remove operations are not supported. So, this abstraction is leaky, heavily relying on the way it is used without specifying it, and it only works because add and remove operations are protected by a lock and it also happens that add and remove are never called in the same critical section. If they ever were, the user could observe an inconsistent state via an iterator, which is not created under a lock.

The solution, I think, is to add a comment both in the CopyOnWriteList documentation, clarifying that it's a very specific implementation, and inside ArrayBroadcastChannel, warning that each access to subscribers should be carefully reviewed in tandem with CopyOnWriteList implementation on Native.

"Historical note" only refers to the first paragraph. Probably should have split into several comments.

Added a warning and a deprecation.

Broadcast channels are being decommissioned, so it shouldn't be a problem.

dkhalanskyjb · 2021-11-10T10:27:49Z

kotlinx-coroutines-core/jvm/test/exceptions/Exceptions.kt

@@ -15,7 +15,7 @@ import kotlin.test.*
 * but run only under JDK 1.8
 */
 @Suppress("ConflictingExtensionProperty")
-val Throwable.suppressed: Array<Throwable> get() {
+actual val Throwable.suppressed: Array<Throwable> get() {
    val method = this::class.java.getMethod("getSuppressed") ?: error("This test can only be run using JDK 1.7")


Suggested change

val method = this::class.java.getMethod("getSuppressed") ?: error("This test can only be run using JDK 1.7")

val method = this::class.java.getMethod("getSuppressed") ?: error("This test can only be run using JDK 1.7+")

Do we still support 1.6, by the way?

We're in limbo here. Let's discuss it separately

kotlinx-coroutines-core/native/src/Builders.kt

kotlinx-coroutines-core/native/src/CoroutineContext.kt

kotlinx-coroutines-core/jvm/test/ConcurrentTestUtilities.kt

dkhalanskyjb

Another batch of comments.

kotlinx-coroutines-core/jvm/test/FieldWalker.kt

kotlinx-coroutines-core/jvm/src/Builders.kt

dkhalanskyjb · 2021-11-10T11:57:40Z

kotlinx-coroutines-core/jvm/src/internal/Concurrent.kt

@@ -36,3 +36,5 @@ internal fun removeFutureOnCancel(executor: Executor): Boolean {
        return false // failed to setRemoveOnCancelPolicy, assume it does not removes future on cancel
    }
 }
+
+internal actual val multithreadingSupported: Boolean = true


Suggestion:

expect/actual multithreadingSupported in the tests.

val multithreadingEnabled on the Native.

Reasoning: these can be thought of as orthogonal, as in the tests, this controls their conditional execution, whereas, in the Native code, it for now controls whether the new memory model is enabled. I think these are separate concerns that we may want to evolve differently. For example, we may remove multithreadingEnabled in the future if we decide to enable it unconditionally, but in the tests, excluding the JS may still be useful.

Also, I don't know how to force the IDE to show me the source set in the popup listing the uses of something. Because of this, it's difficult to tell immediately if there is some common non-test code that relies on this variable.

I'd suggest doing so when we are going to evolve the code; it seems a bit premature for now

Having searched my feelings for why this bugs me, I think I have a more compelling explanation.

This variable is present in the non-test common code, even though it wasn't used anywhere other than on the Native. This, like not making a variable private when it can be, is, I think, an anti-pattern: making the scope in which something is present large is easy, reducing the scope is not.

What happened just now, in this PR, is an example of just that: this variable only concerns the behavior on Native, but, due to it being available everywhere, it is used in the common code. If we later make Native multithreaded by default, we may not notice that the variable outlived its usefulness and can be removed everywhere but in tests (and possibly even there).

it seems a bit premature for now

I'd argue the opposite: it's premature to have a broad scope of availability before it's actually needed.

These are good points!

I've made the following: made runMtTest expect function only in concurrent source-set, made multithreadingSupported native-only declaration and moved the only use of multithreadingSupported in common to Native source-set

kotlinx-coroutines-core/common/src/internal/Concurrent.common.kt

kotlinx-coroutines-core/common/test/TestBase.common.kt

kotlinx-coroutines-core/native/src/Dispatchers.kt

kotlinx-coroutines-core/concurrent/test/sync/SemaphoreStressTest.kt

kotlinx-coroutines-core/concurrent/test/sync/MutexStressTest.kt

kotlinx-coroutines-core/common/test/TestBase.common.kt

Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>

dkhalanskyjb

Second-to-last dump of the initial comments while I brace myself to read the Darwin Dispatchers.Main implementation and tests.

kotlinx-coroutines-core/jvm/test/channels/RendezvousChannelStressTest.kt

kotlinx-coroutines-core/concurrent/test/JobStructuredJoinStressTest.kt

kotlinx-coroutines-core/concurrent/test/internal/LockFreeLinkedListTest.kt

kotlinx-coroutines-core/concurrent/test/RunBlockingTest.kt

kotlinx-coroutines-core/concurrent/test/ConcurrentExceptionsStressTest.kt

kotlinx-coroutines-core/concurrent/test/StateFlowUpdateCommonTest.kt

kotlinx-coroutines-core/nativeDarwin/src/Dispatchers.kt

kotlinx-coroutines-core/nativeDarwin/test/MainDispatcherTest.kt

Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>

…urrencyTest.kt Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>

Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>

kotlinx-coroutines-core/nativeDarwin/src/Dispatchers.kt

dkhalanskyjb · 2021-11-11T08:02:30Z

kotlinx-coroutines-core/native/src/internal/CopyOnWriteList.kt

-
-    private class IteratorImpl<E>(private var array: Array<E>, private val size: Int) : MutableIterator<E> {
-
+    private class IteratorImpl<E>(private val array: Array<E>) : MutableIterator<E> {


"Historical note" only refers to the first paragraph. Probably should have split into several comments.

kotlinx-coroutines-core/native/src/Dispatchers.kt

kotlinx-coroutines-core/concurrent/test/RunBlockingTest.kt

kotlinx-coroutines-core/common/test/TestBase.common.kt

kotlinx-coroutines-core/concurrent/test/flow/StateFlowCommonStressTest.kt

kotlinx-coroutines-core/common/src/EventLoop.common.kt

Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>

dkhalanskyjb

Great job ensuring that DefaultExecutor is safe!

…otlin#2833) * Support of new K/N memory model * Dispatchers.Default backed by a pool of workers on Linux and by global_queue on iOS-like * Implementation of Dispatchers.Main that uses the main queue on iOS and default dispatcher on other platforms (Kotlin#2858) * Introduced newSingleThreadDispatcher and newFixedThreadPoolDispatcher * Use proper reentrant locking and CoW arrays on new memory model, make TestBase _almost_ race-free * More thread-safety in Native counterpart and one more test from native-mt * Source-set sharing for tests shared between JVM and K/N * Wrap Obj-C interop into autorelease pool to avoid memory leaks Fixes Kotlin#2914 Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>

mvicsokolova force-pushed the new-native-memory-model branch from aebc889 to ca3e621 Compare July 19, 2021 15:03

qwwdfsad force-pushed the new-native-memory-model branch from 7909ee6 to 16b64a8 Compare August 5, 2021 13:51

qwwdfsad force-pushed the new-native-memory-model branch 2 times, most recently from 91289bd to f96de23 Compare August 29, 2021 11:27

qwwdfsad mentioned this pull request Sep 2, 2021

Merge new Native memory model support into the mainline #2914

Closed

qwwdfsad force-pushed the new-native-memory-model branch 2 times, most recently from f96de23 to 407e0b6 Compare October 26, 2021 14:22

qwwdfsad force-pushed the new-native-memory-model branch from 407e0b6 to 8cdafed Compare October 26, 2021 14:27

qwwdfsad added 6 commits October 26, 2021 17:41

Merge branch 'develop' into new-native-memory-model

206d076

~compilation fixes

96f7dc1

~move K/N compiler configuration to core

e052ea1

~migrate to CloseableCoroutineDispatcher

b343d93

~run tests 4 times

5442a1c

~run coroutines in two modes

2c8439f

qwwdfsad force-pushed the new-native-memory-model branch from ed876b0 to 2c8439f Compare October 27, 2021 14:14

qwwdfsad changed the title ~~[BASE BRANCH] Enable new Native memory model~~ Incorporate new Native memory model into kotlinx-coroutines mainline Oct 27, 2021

qwwdfsad requested a review from dkhalanskyjb October 27, 2021 14:22

qwwdfsad added the For 1.6 label Oct 27, 2021

qwwdfsad added 2 commits October 27, 2021 17:37

~cleanup:

e8a76cf

* Dispatchers.Default cannot be closed * Throwins Dispatchers.Main on Linux and Windows

~fixup tests:

57d65da

* SharedImmutable where necessary * Disable MainDispatcherTest with old MM

dkhalanskyjb reviewed Nov 10, 2021

View reviewed changes

dkhalanskyjb self-requested a review November 10, 2021 11:26

dkhalanskyjb reviewed Nov 10, 2021

View reviewed changes

kotlinx-coroutines-core/jvm/test/ConcurrentTestUtilities.kt Show resolved Hide resolved

Merge branch 'develop' into new-native-memory-model

6ee3209

dkhalanskyjb reviewed Nov 10, 2021

View reviewed changes

Update kotlinx-coroutines-core/native/test/ConcurrentTestUtilities.kt

575b468

Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>

dkhalanskyjb reviewed Nov 10, 2021

View reviewed changes

dkhalanskyjb requested changes Nov 10, 2021

View reviewed changes

kotlinx-coroutines-core/nativeDarwin/src/Dispatchers.kt Outdated Show resolved Hide resolved

kotlinx-coroutines-core/nativeDarwin/src/Dispatchers.kt Outdated Show resolved Hide resolved

kotlinx-coroutines-core/nativeDarwin/test/MainDispatcherTest.kt Outdated Show resolved Hide resolved

Update kotlinx-coroutines-core/native/src/internal/CopyOnWriteList.kt

d87858d

Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>

qwwdfsad and others added 7 commits November 10, 2021 18:33

Update kotlinx-coroutines-core/native/src/CoroutineContext.kt

93f8867

Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>

Update kotlinx-coroutines-core/concurrent/test/AbstractDispatcherConc…

79ba184

…urrencyTest.kt Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>

Update kotlinx-coroutines-core/concurrent/test/AbstractDispatcherConc…

63877dd

…urrencyTest.kt Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>

Update kotlinx-coroutines-core/native/src/EventLoop.kt

51aa77f

Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>

~batch of small fixes

c77e852

~remove leftover

a6f2d5c

~cleanup

f8e6661

qwwdfsad requested a review from dkhalanskyjb November 10, 2021 17:11

dkhalanskyjb requested changes Nov 11, 2021

View reviewed changes

qwwdfsad and others added 7 commits November 11, 2021 15:48

Update kotlinx-coroutines-core/nativeDarwin/src/Dispatchers.kt

2da3434

Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>

Update kotlinx-coroutines-core/concurrent/test/RunBlockingTest.kt

ef2f6fc

Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>

Update kotlinx-coroutines-core/nativeDarwin/src/Dispatchers.kt

3412db3

Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>

~set of small cleanups

6ac83ee

~refactor multithreadingSupported to be native-specific

f005b54

~formatting

5564b35

~warnings about SubscriberList

d7e8d79

qwwdfsad requested a review from dkhalanskyjb November 11, 2021 14:09

dkhalanskyjb approved these changes Nov 12, 2021

View reviewed changes

qwwdfsad merged commit 0427205 into develop Nov 15, 2021

qwwdfsad deleted the new-native-memory-model branch November 15, 2021 09:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Incorporate new Native memory model into kotlinx-coroutines mainline #2833

Incorporate new Native memory model into kotlinx-coroutines mainline #2833

qwwdfsad commented Jul 16, 2021 •

edited

dkhalanskyjb left a comment

dkhalanskyjb Nov 10, 2021

qwwdfsad Nov 10, 2021

dkhalanskyjb Nov 11, 2021

qwwdfsad Nov 11, 2021 •

edited

dkhalanskyjb Nov 10, 2021

dkhalanskyjb Nov 11, 2021

qwwdfsad Nov 11, 2021

dkhalanskyjb Nov 10, 2021

qwwdfsad Nov 10, 2021

dkhalanskyjb left a comment

dkhalanskyjb Nov 10, 2021

qwwdfsad Nov 10, 2021

dkhalanskyjb Nov 11, 2021

qwwdfsad Nov 11, 2021

dkhalanskyjb left a comment

dkhalanskyjb Nov 11, 2021

dkhalanskyjb left a comment


		private class IteratorImpl<E>(private var array: Array<E>, private val size: Int) : MutableIterator<E> {

		private class IteratorImpl<E>(private val array: Array<E>) : MutableIterator<E> {

	val method = this::class.java.getMethod("getSuppressed") ?: error("This test can only be run using JDK 1.7")
	val method = this::class.java.getMethod("getSuppressed") ?: error("This test can only be run using JDK 1.7+")

Incorporate new Native memory model into kotlinx-coroutines mainline #2833

Incorporate new Native memory model into kotlinx-coroutines mainline #2833

Conversation

qwwdfsad commented Jul 16, 2021 • edited

dkhalanskyjb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qwwdfsad Nov 11, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkhalanskyjb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkhalanskyjb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkhalanskyjb left a comment

Choose a reason for hiding this comment

qwwdfsad commented Jul 16, 2021 •

edited

qwwdfsad Nov 11, 2021 •

edited