-
Notifications
You must be signed in to change notification settings - Fork 636
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce CharArray caching for InputStream decoding #2100
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
package kotlinx.serialization.json | ||
|
||
import kotlinx.coroutines.* | ||
import kotlinx.serialization.Serializable | ||
import kotlinx.serialization.builtins.* | ||
import org.junit.Test | ||
import java.io.ByteArrayInputStream | ||
import java.io.ByteArrayOutputStream | ||
import kotlin.random.* | ||
import kotlin.test.* | ||
|
||
// Stresses out that JSON decoded in parallel does not interfere (mostly via caching of various buffers) | ||
class JsonConcurrentStressTest : JsonTestBase() { | ||
private val charset = "ABCDEFGHIJKLMNOPQRSTUVWXTZabcdefghiklmnopqrstuvwxyz0123456789" | ||
|
||
@Test | ||
fun testDecodeInParallelSimpleList() = doTest(100) { mode -> | ||
val value = (1..10000).map { Random.nextDouble() } | ||
val string = Json.encodeToString(ListSerializer(Double.serializer()), value, mode) | ||
assertEquals(value, Json.decodeFromString(ListSerializer(Double.serializer()), string, mode)) | ||
} | ||
|
||
@Serializable | ||
data class Foo(val s: String, val f: Foo?) | ||
|
||
@Test | ||
fun testDecodeInParallelListOfPojo() = doTest(1_000) { mode -> | ||
val value = (1..100).map { | ||
val randomString = getRandomString() | ||
val nestedFoo = Foo("null抢\u000E鋽윝䑜厼\uF70A紲ᢨ䣠null⛾䉻嘖緝ᯧnull쎶\u0005null" + randomString, null) | ||
Foo(getRandomString(), nestedFoo) | ||
} | ||
val string = Json.encodeToString(ListSerializer(Foo.serializer()), value, mode) | ||
assertEquals(value, Json.decodeFromString(ListSerializer(Foo.serializer()), string, mode)) | ||
} | ||
|
||
@Test | ||
fun testDecodeInParallelPojo() = doTest(100_000) { mode -> | ||
val randomString = getRandomString() | ||
val nestedFoo = Foo("null抢\u000E鋽윝䑜厼\uF70A紲ᢨ䣠null⛾䉻嘖緝ᯧnull쎶\u0005null" + randomString, null) | ||
val randomFoo = Foo(getRandomString(), nestedFoo) | ||
val string = Json.encodeToString(Foo.serializer(), randomFoo, mode) | ||
assertEquals(randomFoo, Json.decodeFromString(Foo.serializer(), string, mode)) | ||
} | ||
|
||
@Test | ||
fun testDecodeInParallelSequencePojo() = runBlocking<Unit> { | ||
for (i in 1 until 1_000) { | ||
launch(Dispatchers.Default) { | ||
val values = (1..100).map { | ||
val randomString = getRandomString() | ||
val nestedFoo = Foo("null抢\u000E鋽윝䑜厼\uF70A紲ᢨ䣠null⛾䉻嘖緝ᯧnull쎶\u0005null" + randomString, null) | ||
Foo(getRandomString(), nestedFoo) | ||
} | ||
val baos = ByteArrayOutputStream() | ||
for (value in values) { | ||
Json.encodeToStream(Foo.serializer(), value, baos) | ||
} | ||
val bais = ByteArrayInputStream(baos.toByteArray()) | ||
assertEquals(values, Json.decodeToSequence(bais, Foo.serializer()).toList()) | ||
} | ||
} | ||
} | ||
|
||
private fun getRandomString() = (1..Random.nextInt(0, charset.length)).map { charset[it] }.joinToString(separator = "") | ||
|
||
private fun doTest(iterations: Int, block: (JsonTestingMode) -> Unit) { | ||
runBlocking<Unit> { | ||
for (i in 1 until iterations) { | ||
launch(Dispatchers.Default) { | ||
parametrizedTest { | ||
block(it) | ||
} | ||
} | ||
} | ||
} | ||
} | ||
} |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
/* | ||
* Copyright 2017-2022 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license. | ||
*/ | ||
package kotlinx.serialization.json.internal | ||
|
||
internal expect object CharArrayPoolBatchSize { | ||
fun take(): CharArray | ||
fun release(array: CharArray) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
/* | ||
* Copyright 2017-2022 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license. | ||
*/ | ||
package kotlinx.serialization.json.internal | ||
|
||
|
||
internal actual object CharArrayPoolBatchSize { | ||
|
||
actual fun take(): CharArray = CharArray(BATCH_SIZE) | ||
|
||
actual fun release(array: CharArray) = Unit | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,14 @@ | ||
/* | ||
* Copyright 2017-2022 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license. | ||
*/ | ||
package kotlinx.serialization.json.internal | ||
|
||
import java.util.concurrent.* | ||
|
||
internal object CharArrayPool { | ||
internal open class CharArrayPoolBase { | ||
private val arrays = ArrayDeque<CharArray>() | ||
private var charsTotal = 0 | ||
|
||
/* | ||
* Not really documented kill switch as a workaround for potential | ||
* (unlikely) problems with memory consumptions. | ||
|
@@ -13,20 +17,38 @@ internal object CharArrayPool { | |
System.getProperty("kotlinx.serialization.json.pool.size").toIntOrNull() | ||
}.getOrNull() ?: 1024 * 1024 // 2 MB seems to be a reasonable constraint, (1M of chars) | ||
|
||
fun take(): CharArray { | ||
protected fun take(size: Int): CharArray { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's better to take the size in the class constructor rather than in the function call. In fact, this is the case for each instance of the pool right now.
The API does not advise in any way that you should not do this. If you add a size to the constructor, then you can also add a check for the size of the array inside the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a good remark for any general-purpose API, but in this situation, we are dealing with well-tailored internal pools for one specific purpose. Also, it will force us to introduce a few more bytecodes -- for class with ctor, for outer class that will store the instance etc. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the byte code, only a new field with the int type will be added (the parameter will be transferred from the I think this is a good tradeoff so that if someone else refactor the code after a while, they don't add an unobvious bug. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that this is a protected method, I don't see that it is necessary for now, but the idea overall is a valid concern. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The method is indeed protected and sued as a base for the implementation. |
||
/* | ||
* Initially the pool is empty, so an instance will be allocated | ||
* and the pool will be populated in the 'release' | ||
*/ | ||
val candidate = synchronized(this) { | ||
arrays.removeLastOrNull()?.also { charsTotal -= it.size } | ||
} | ||
return candidate ?: CharArray(128) | ||
return candidate ?: CharArray(size) | ||
} | ||
|
||
fun release(array: CharArray) = synchronized(this) { | ||
protected fun releaseImpl(array: CharArray): Unit = synchronized(this) { | ||
if (charsTotal + array.size >= MAX_CHARS_IN_POOL) return@synchronized | ||
charsTotal += array.size | ||
arrays.addLast(array) | ||
} | ||
} | ||
|
||
internal object CharArrayPool : CharArrayPoolBase() { | ||
fun take(): CharArray = super.take(128) | ||
|
||
// Can release array of an arbitrary size | ||
fun release(array: CharArray) = releaseImpl(array) | ||
} | ||
|
||
// Pools char arrays of size 16K | ||
internal actual object CharArrayPoolBatchSize : CharArrayPoolBase() { | ||
|
||
actual fun take(): CharArray = super.take(BATCH_SIZE) | ||
|
||
actual fun release(array: CharArray) { | ||
require(array.size == BATCH_SIZE) { "Inconsistent internal invariant: unexpected array size ${array.size}" } | ||
releaseImpl(array) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
/* | ||
* Copyright 2017-2022 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license. | ||
*/ | ||
package kotlinx.serialization.json.internal | ||
|
||
internal actual object CharArrayPoolBatchSize { | ||
actual fun take(): CharArray = CharArray(BATCH_SIZE) | ||
actual fun release(array: CharArray) = Unit | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JsonTestingMode
seems to be not used anywhere.I'd also add test for
decodeFromStream
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is used in the test bodies, streams are tested as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I haven't noticed because it has the default name
it
. Maybe add a name for better readability?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done