kotlinx.serialization icon indicating copy to clipboard operation
kotlinx.serialization copied to clipboard

Disallow missing commas between fields (#2287 #2520)

Open alisa101rs opened this issue 1 year ago • 4 comments

Fixes #2287 and #2520, makes sure that comma between key-value pairs is required in Json.decodeFromString

alisa101rs avatar Dec 17 '24 04:12 alisa101rs

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

alisa101rs avatar Dec 18 '24 22:12 alisa101rs

Sure, I'll restart the build

sandwwraith avatar Dec 19 '24 10:12 sandwwraith

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.

sandwwraith avatar Jan 21 '25 13:01 sandwwraith

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

alisa101rs avatar Jan 22 '25 06:01 alisa101rs