-
Notifications
You must be signed in to change notification settings - Fork 7.3k
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
Add streaming deserialization support for kotlinx.serialization.json #4166
base: trunk
Are you sure you want to change the base?
Conversation
@ExperimentalSerializationApi | ||
class SerializerFromJson(override val format: Json) : Serializer() { | ||
override fun <T> fromResponseBody(loader: DeserializationStrategy<T>, body: ResponseBody): T { | ||
val stream = body.byteStream() |
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.
This incurs a lot of copies. It's probably better to depend on the Okio version of the json artifact and use the Okio-based steaming APIs.
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.
Migrated to the Okio-base APIs. Could you please re-check?
@@ -12,7 +12,7 @@ import okhttp3.MediaType | |||
import okhttp3.RequestBody | |||
import okhttp3.ResponseBody | |||
|
|||
internal sealed class Serializer { | |||
abstract class Serializer { |
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.
Keep this and Factory
internal to this module. We don't need to share anything—the implementation isn't very big and we don't need as much indirection.
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.
Ok, created module-specific Factory and converters implementations.
80bf187
to
0941c81
Compare
@JakeWharton is there anything else I can do to proceed with the PR? |
Sorry I haven't forgotten about this. I was waffling on what I wanted to do based on this comment: Kotlin/kotlinx.serialization#253 (comment). Specifically,
|
I guess in that case we can simply deprecate this converter and add the streaming to the normal one, assuming the APIs make themselves available on the high-level "format"-suffixed types... |
Sounds reasonable. Until then we could use this Json-specific implementation. |
Yep I'll get it in here soon. Probably Monday |
Any news? This hasn't been merged, nor is the streaming API in the regular KotlinX Serialization Converter. |
@JakeWharton is there plans to merge this? |
The Okio-based API of Kotlinx Serialization has terrible performance (more than 2 times slower than String last time I checked) because it basically decodes one codepoint at a time; I would advise not to use it and use the java |
Streaming is always slower than buffered, yes. The point of using the streaming APIs is that you get to amortize the cost of serialization and deserialization across the cost of the network transfer, which is orders of magnitude slower. So instead of paying 100ms to serialize and 1000ms to transfer you effectively only pay 1000ms with streaming. Whether serialization took 100ms, 200ms, or 500ms becomes irrelevant because the limiting factor is always the network. |
Why not pay even less, especially when reading the response back from the disk cache? |
Okio costs less than |
It costs less when performing comparable operations. But last time I checked, it takes much more CPU time to decode chars one by one from a See related issue in the Kotlin Serialization project: Kotlin/kotlinx.serialization#2743 |
Follows up JakeWharton/retrofit2-kotlinx-serialization-converter#43 by introducing a new ConverterFactory specific for the Json format.
It uses Okio-based streaming API to deserialize responses.