Make the segment pool lock-free on the JVM by swankjesse · Pull Request #717 · square/okio

swankjesse · 2020-05-23T23:32:29Z

Under a high contention environment we'll do more zero-fill and GC
instead of locking.

Egorand

Why not have the new implementation in commonMain? AtomicLong and AtomicReference are available in kotlin-stdlib / kotlin.native.concurrent.

Egorand · 2020-05-23T23:50:19Z

okio/src/jvmMain/kotlin/okio/SegmentPool.kt

+
+  actual fun take(): Segment {
+    return when (val first = firstRef.getAndSet(LOCK)) {
+      LOCK -> {


I wonder if when will do a ref check or an equality check here? Will this code break if it's an equality check?

I like your idea of using ===. Lemme switch to that. (They’ll do the same thing regardless, because Segment doesn’t override equals()).

Egorand · 2020-05-23T23:51:49Z

okio/src/jvmMain/kotlin/okio/SegmentPool.kt

+  }
+
+  actual fun recycle(segment: Segment) {
+    require(segment.next == null && segment.prev == null)


nit: maybe add an error message? Or even split this into two requires?

It’s more of an assert than anything. This isn’t public API. I’d prefer to not split it up because its primary purpose is documentation.

Egorand · 2020-05-23T23:52:11Z

okio/src/jvmMain/kotlin/okio/SegmentPool.kt

+    if (atomicByteCount.get() >= MAX_SIZE) return // Pool is full.
+
+    val first = firstRef.get()
+    if (first == LOCK) return // A take() is currently in progress.


first === LOCK?

Good call. Fixed.

JakeWharton

I would like to see this rolled out to a highly concurrent server as an experiment before we ship it

JakeWharton · 2020-05-23T23:53:15Z

okio/src/jvmMain/kotlin/okio/SegmentPool.kt

+  /** Total bytes in this pool. */
+  private var atomicByteCount = AtomicLong()
+
+  actual val byteCount: Long


What is this used for? The order of operations means that you can observe this being larger than what's actually available.

Just testing.

Lemme document that.

JakeWharton · 2020-05-23T23:54:15Z

okio/src/jvmMain/kotlin/okio/SegmentPool.kt

+    if (atomicByteCount.get() >= MAX_SIZE) return // Pool is full.
+
+    val first = firstRef.get()
+    if (first == LOCK) return // A take() is currently in progress.


JakeWharton · 2020-05-23T23:54:39Z

okio/src/jvmMain/kotlin/okio/SegmentPool.kt

+
+  actual fun take(): Segment {
+    return when (val first = firstRef.getAndSet(LOCK)) {
+      LOCK -> {


This will do == but we probably want ===

Yep, good call!

JakeWharton · 2020-05-23T23:57:15Z

okio/src/jvmMain/kotlin/okio/SegmentPool.kt

+internal actual object SegmentPool {
+  /** The maximum number of bytes to pool.  */
+  // TODO: Is 64 KiB a good maximum size? Do we ever have that many idle segments?
+  actual val MAX_SIZE = 64 * 1024L // 64 KiB.


@JvmStatic

JakeWharton · 2020-05-23T23:57:18Z

okio/src/jvmMain/kotlin/okio/SegmentPool.kt

+  actual val MAX_SIZE = 64 * 1024L // 64 KiB.
+
+  /** A sentinel segment to indicate that the linked list is currently being modified. */
+  private val LOCK = Segment(ByteArray(0), pos = 0, limit = 0, shared = false, owner = false)


@JvmStatic

Since it’s an object I get that for free.

Actually, only the fields, not the methods!

Fixed all to be @JvmStatic. Can’t hurt.

Why care about @JvmStatic on private fields though? To avoid calling getters internally? Won't it be @JvmField then?

I decided to go with @JvmStatic on the methods and not on the fields. That’s the least amount of code and the simplest dispatch for callers.

JakeWharton · 2020-05-23T23:58:20Z

okio/src/jvmMain/kotlin/okio/SegmentPool.kt

+  private val LOCK = Segment(ByteArray(0), pos = 0, limit = 0, shared = false, owner = false)
+
+  /** Singly-linked list of segments. */
+  private var firstRef = AtomicReference<Segment>()


@JvmStatic

JakeWharton · 2020-05-23T23:58:22Z

okio/src/jvmMain/kotlin/okio/SegmentPool.kt

+  private var firstRef = AtomicReference<Segment>()
+
+  /** Total bytes in this pool. */
+  private var atomicByteCount = AtomicLong()


@JvmStatic

JakeWharton · 2020-05-23T23:58:30Z

okio/src/jvmMain/kotlin/okio/SegmentPool.kt

+  /** Total bytes in this pool. */
+  private var atomicByteCount = AtomicLong()
+
+  actual val byteCount: Long


@JvmStatic

JakeWharton · 2020-05-23T23:58:35Z

okio/src/jvmMain/kotlin/okio/SegmentPool.kt

+  actual val byteCount: Long
+    get() = atomicByteCount.get()
+
+  actual fun take(): Segment {


@JvmStatic

JakeWharton · 2020-05-23T23:58:41Z

okio/src/jvmMain/kotlin/okio/SegmentPool.kt

+    }
+  }
+
+  actual fun recycle(segment: Segment) {


@JvmStatic

swankjesse · 2020-05-24T01:33:16Z

Agreed 100% on wanting to give this lots of exercise before shipping it to everyone!

swankjesse · 2020-05-24T01:35:16Z

@Egorand I can’t do commonMain ’cause JVM/AtomicReference and Kotlin/AtomicReference don’t have an expect definition in common. When I tried to hack one myself, KMP got angry at the type parameters on AtomicReference.

swankjesse · 2020-05-24T01:47:28Z

This branch released as 2.7.0-alpha.lockfree.1.

bnorm

Are aware of https://github.com/Kotlin/kotlinx.atomicfu? Used by kotlinx-coroutines for atomics in common code. Compile time only as well. Could help remove the expect/actual if you wanted to go down that route.

bnorm · 2020-05-24T16:41:46Z

okio/src/jvmMain/kotlin/okio/SegmentPool.kt

+
+  /** Singly-linked list of segments. */
+  @JvmStatic
+  private var firstRef = AtomicReference<Segment>()


AtomicReference<Segment?> will get you some null type safety. Otherwise firstRef.getAndSet(LOCK) and firstRef.get() will return platform types.

Oooh I like it.

Under a high contention environment we'll do more zero-fill and GC instead of locking.

swankjesse · 2020-05-25T13:19:50Z

AtomicFU looks brilliant. That’d be an excellent tool for our toolchain if/when we get serious about OkHttp in Kotlin/Native. For now I’m reluctant only because it’s experimental magic, and Okio is no place for experimental magic.

codefromthecrypt · 2020-12-07T07:42:17Z

added some notes after reviewing this incredibly late #831

codefromthecrypt · 2020-12-23T01:55:24Z

okio/src/jvmMain/kotlin/okio/SegmentPool.kt

+    val first = firstRef.get()
+    if (first === LOCK) return // A take() is currently in progress.
+
+    segment.next = first


sorry if I'm showing my lack of kotlin knowledge, but I'm wondering how these 3 changes will be guaranteed to be visible in the right order to other threads? We aren't using locks or volatile inside Segment as far as I can tell.. what am I missing? (I want to follow up with a comment to explain this later unless it is generally understood kotlin thing)

ah right. below we are using compareAndSet, not weakCompareAndSet, and that is what's allowing these changes to be safely published to the consumer of the pool (happens-before relationship) https://docs.oracle.com/javase/6/docs/api/java/util/concurrent/atomic/package-summary.html

swankjesse mentioned this pull request May 23, 2020

SegmentPool blocked #715

Closed

Egorand approved these changes May 23, 2020

View reviewed changes

Egorand reviewed May 23, 2020

View reviewed changes

JakeWharton approved these changes May 23, 2020

View reviewed changes

swankjesse force-pushed the jwilson.0523.lock_free branch from 884f38f to 52859e5 Compare May 24, 2020 01:33

bnorm approved these changes May 24, 2020

View reviewed changes

Make the segment pool lock-free on the JVM

fae6650

Under a high contention environment we'll do more zero-fill and GC instead of locking.

swankjesse force-pushed the jwilson.0523.lock_free branch from 52859e5 to fae6650 Compare May 25, 2020 13:18

swankjesse merged commit 2a2d4f8 into master May 25, 2020

satoshun mentioned this pull request Jul 26, 2020

Okio 2.7のパフォーマンス向上について AndroidDagashi/AndroidDagashi#1487

Open

oldergod deleted the jwilson.0523.lock_free branch August 18, 2020 01:16

codefromthecrypt reviewed Dec 23, 2020

View reviewed changes

This was referenced Sep 8, 2021

Different buffer pool implementations suitable for server use? #94

Closed

SegmentPool lock is a hot spot; investigate ConcurrentLinkedDeque etc. #335

Closed

Conversation

swankjesse commented May 23, 2020

Uh oh!

Egorand left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JakeWharton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swankjesse commented May 24, 2020

Uh oh!

swankjesse commented May 24, 2020

Uh oh!

swankjesse commented May 24, 2020

Uh oh!

bnorm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swankjesse commented May 25, 2020

Uh oh!

codefromthecrypt commented Dec 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!