-
Notifications
You must be signed in to change notification settings - Fork 314
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
refactor: Prefer KxS's string API over the stream API #8902
Conversation
The string API is generally considered to be faster, see e.g. [1] and [2]. [1]: Kotlin/kotlinx.serialization#2186 [2]: Kotlin/kotlinx.serialization#2657 (comment) Signed-off-by: Sebastian Schuberth <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8902 +/- ##
=========================================
Coverage 67.63% 67.63%
Complexity 1168 1168
=========================================
Files 244 244
Lines 7781 7781
Branches 867 867
=========================================
Hits 5263 5263
Misses 2161 2161
Partials 357 357
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
I find the linked discussions not convincing, the first one closes with the question if some performance improvements in version 1.5.0 changed the situation, but this was never answered. We are now at version 1.7.1, so it's possible that more optimizations have been implemented in the meantime. Also, the benchmark was done on an Android device which could be different to the JVM we use. As the memory usage should always be higher when first reading to a string, I would like to see an actual benchmark that compares both for the current version of kotlinx.serialization before doing the switch. |
True, but that probably does not matter much for the size of the data we're dealing with in the changed code places. Performance for the same reason does not improve much either, but the string API is also a bit easier to use as it requires less code. |
IMO it would be nice to add our own |
And we could even select the API dynamically on the file size. I'll look into that. |
As it would be a bit unclear where to put such a function (I do not want |
The string API is generally considered to be faster, see e.g. 1 and 2.