Disallow missing commas between fields (#2287 #2520)
Fixes #2287 and #2520, makes sure that comma between key-value pairs is required in Json.decodeFromString
Hi @sandwwraith, could you please take a look at failed build? I believe it's not caused by this PR and I shouldn't/can't fix it in this PR
Sure, I'll restart the build
Just a quick update from me: I've reviewed the PR and I'm mostly satisfied with the changes. However, I also ran the decoding benchmarks and unfortunately, scores for them dropped for about 6-8%:
develop
====
Benchmark Mode Cnt Score Error Units
TwitterFeedBenchmark.decodeMicroTwitter thrpt 14 962.120 ± 34.369 ops/s
TwitterFeedBenchmark.decodeMicroTwitterNoAltNames thrpt 14 1005.670 ± 43.361 ops/s
TwitterFeedBenchmark.decodeMicroTwitterWithNamingStrategy thrpt 14 915.170 ± 10.127 ops/s
TwitterFeedBenchmark.decodeTwitter thrpt 14 1073.797 ± 51.738 ops/s
TwitterFeedBenchmark.decodeTwitterNoAltNames thrpt 14 1090.153 ± 3.413 ops/s
TwitterFeedBenchmark.decodeTwitterWithNamingStrategy thrpt 14 928.378 ± 3.961 ops/s
this PR
====
Benchmark Mode Cnt Score Error Units
TwitterFeedBenchmark.decodeMicroTwitter thrpt 14 885.924 ± 27.029 ops/s
TwitterFeedBenchmark.decodeMicroTwitterNoAltNames thrpt 14 968.884 ± 7.501 ops/s
TwitterFeedBenchmark.decodeMicroTwitterWithNamingStrategy thrpt 14 865.704 ± 17.647 ops/s
TwitterFeedBenchmark.decodeTwitter thrpt 14 1013.502 ± 14.582 ops/s
TwitterFeedBenchmark.decodeTwitterNoAltNames thrpt 14 983.525 ± 48.605 ops/s
TwitterFeedBenchmark.decodeTwitterWithNamingStrategy thrpt 14 871.941 ± 9.046 ops/s
I will try to investigate whether it is possible to restructure some changes to optimize it further or if we have to accept this performance hit for the sake of correctness.
Yeah I think performance hit comes from this change
https://github.com/Kotlin/kotlinx.serialization/pull/2889/files#diff-dc3d58b34387712e1b644a549a44bc8ad853ee2d1944f0da168b4c9e6ad28fc6L113-R115
https://github.com/Kotlin/kotlinx.serialization/blob/f9f160a680da9f92c3bb121ae3644c96e57ba42e/formats/json/commonMain/src/kotlinx/serialization/json/internal/StreamingJsonDecoder.kt#L113-L117
https://github.com/alisa101rs/kotlinx.serialization/blob/719acbb385679a51743801bb114a6f5da0fd3aef/formats/json/commonMain/src/kotlinx/serialization/json/internal/StreamingJsonDecoder.kt#L115