Rewrite TreeJsonDecoder main decoding loop to be identical to StreamingJsonDecoder - #3220
Rewrite TreeJsonDecoder main decoding loop to be identical to StreamingJsonDecoder#3220qwwdfsad wants to merge 1 commit into
Conversation
…ngJsonDecoder Previously, the decoding was SerialDescriptor-driven, not JsonObject keys-driven. It made us to reimplement the logic of StreamingJsonDecoder in an inverted manner (instead of mapping actual keys to indices, we mapped indices to keys), which led to all kinds of duplicated caches and reverse quadratic lookups. Now the logic is almost identical to the streaming decoder, the redundancy in implementation is gone, and throughput for the tree decoder is, for a baseline, twice as good, and for alternative names up to 10 times better, making alternative names to be on par with the regular mode. Implementation-wise, it is still tagged decoder under the hood (will be fixed separately, minor inconvenience, harder to review) and contains behavioural changes, see tests for that. The behavioural changes are now more in line with streaming behaviour. Fixes #3193
|
Benchmarks: https://gist.github.com/qwwdfsad/8523d8ff8212878b8a4b9e71c3cefe94 (decided not to commit) Before the change: After the change: TL;DR much faster, less code, one implementation instead of two, behaviour that replicates streaming decoder |
|
It would be even nicer to get rid of |
sandwwraith
left a comment
sandwwraith
left a comment
There was a problem hiding this comment.
btw, it would be nice to apply similar fix for the DynamicInput. Or try to completely unify it with JsonElement decoder
| @Test | ||
| fun testLastPropertyWinsOnAlternativeNames() { | ||
| val cases = mapOf( | ||
| """{"data":"first","foo":"second"}""" to "second", // primary then alternative |
There was a problem hiding this comment.
I'm a bit concerned about this change. Currently, @SerialName/property name has priority over the alternate one, and it is even documented. I hope no one actually relies on it, though. I'd rather prefer to try to restore the original behavior, but it looks like though it is not possible in this implementation unless we fix #1990 along the way
There was a problem hiding this comment.
This appears to be colliding with a whole different issue. Currently on all releases with @SerialName the behavior of TreeJsonDecoder versus StreamingJsonDecoder diverges.
@Serializable
data class Config(
@SerialName("foo") val a: String? = null, // also works without SerialName
@JsonNames("foo") val b: String? = null,
)
fun test() {
val input = """{"foo":"X"}"""
println(Json.decodeFromString<Config>(input))
println(Json.decodeFromJsonElement<Config>(Json.parseToJsonElement(input)))
}Config(a=X, b=null) // Streaming
Config(a=X, b=X) // Tree
On qwwdfsad/tree-json-opto it inadvertently fixes the divergence and the 2 align.
🙃
| ) | ||
| ) continue // do not read coerced value | ||
| // Pointer to the current entry of JsonObject that is being decoded | ||
| private val entries: Iterator<Map.Entry<String, JsonElement>> by lazy(LazyThreadSafetyMode.NONE) { |
There was a problem hiding this comment.
Is lazy really needed here? decodeElementIndex is always called when class is not empty, so it looks like we can strip one level of indirection. And if value.isEmpty() you can simply have null iterator or something like that
3 participants
Previously, the decoding was SerialDescriptor-driven, not JsonObject keys-driven.
It made us reimplement the logic of StreamingJsonDecoder in an inverted manner (instead of mapping actual keys to indices, we mapped indices to keys), which led to all kinds of duplicated caches and reverse quadratic lookups.
Now the logic is almost identical to the streaming decoder, the redundancy in implementation is gone, and throughput for the tree decoder is, for a baseline, twice as good, and for alternative names up to 10 times better, making alternative names to be on par with the regular mode.
Implementation-wise, it is still tagged decoder under the hood (will be fixed separately, minor inconvenience, harder to review) and contains behavioural changes, see tests for that. The behavioural changes are now more in line with streaming behaviour.