Skip to content
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

IO Streams #1901

Merged
merged 27 commits into from Jun 30, 2022
Merged

IO Streams #1901

merged 27 commits into from Jun 30, 2022

Conversation

shanshin
Copy link
Contributor

@shanshin shanshin commented Apr 13, 2022

No description provided.

@shanshin
Copy link
Contributor Author

shanshin commented May 19, 2022

This implementation is not suitable for release: since json-tests contains a dependency on okio, this reduces the number of native platforms on which tests can be run.

@sandwwraith
Copy link
Member

sandwwraith commented Jun 9, 2022

I see that currently there's an issue with targets that we do support but okio doesn't: linuxArm32Hfp, linuxArm64, iosArm32.

I think we should simply ignore them in our tests too. Here are my thoughts:

  • We can't delete these targets, as there's certain demand (Support for linuxArm32Hfp target #535). Maybe we can further research demand, but grep.app doesn't allow to 'dump' all repositories by the query
  • These targets are actually not tested even now. We don't have linuxArm agents. Regular linux agents test only linuxX64 target.
  • We can always ask Square to add these targets to okio and add them later

The only alternative way I see is to move tests to a separate source set instead of a module. To avoid confusion with IDEA, such source set would be included only in -json module for local development, but into both -json and -json-okio modules during the build.

WDYT?

@shanshin
Copy link
Contributor Author

shanshin commented Jun 22, 2022

I see that currently there's an issue with targets that we do support but okio doesn't: linuxArm32Hfp, linuxArm64, iosArm32.

I think we should simply ignore them in our tests too. Here are my thoughts:

We can't delete these targets, as there's certain demand (#535). Maybe we can further research demand, but grep.app doesn't allow to 'dump' all repositories by the query
These targets are actually not tested even now. We don't have linuxArm agents. Regular linux agents test only linuxX64 target.
We can always ask Square to add these targets to okio and add them later
The only alternative way I see is to move tests to a separate source set instead of a module. To avoid confusion with IDEA, such source set would be included only in -json module for local development, but into both -json and -json-okio modules during the build.

WDYT?

Targets linux ARM 32 HFP, linux ARM 64, iOS ARM 32, minGW x86 was disabled for project kotlinx-serialization-json-tests

build.gradle Outdated Show resolved Hide resolved
formats/json-okio/api/kotlinx-serialization-json-okio.api Outdated Show resolved Hide resolved
formats/json-okio/api/kotlinx-serialization-json-okio.api Outdated Show resolved Hide resolved
formats/json-okio/build.gradle.kts Outdated Show resolved Hide resolved
shanshin and others added 2 commits Jun 28, 2022
…io/OkioStreams.kt

Co-authored-by: Leonid Startsev <sandwwraith@users.noreply.github.com>
@shanshin shanshin requested a review from sandwwraith Jun 28, 2022
@shanshin
Copy link
Contributor Author

shanshin commented Jun 29, 2022

Benchmark comparison for implementation from PR and dev

Test   PR (ops/ms) dev (ops/ms) change
JacksonComparisonBenchmark.jacksonFromString   363.431 368.535 -1.38%
JacksonComparisonBenchmark.jacksonSmallToString   5882.674 5923.969 -0.70%
JacksonComparisonBenchmark.jacksonToString   1142.508 1125.109 +1.55%
JacksonComparisonBenchmark.jacksonToStringWithEscapes 885.894 887.015 -0.13%
JacksonComparisonBenchmark.kotlinFromString   929.297 885.595 +4.93%
JacksonComparisonBenchmark.kotlinSmallToOkio   5363.857    
JacksonComparisonBenchmark.kotlinSmallToStream   6527.822 51.192 +12 651.64%
JacksonComparisonBenchmark.kotlinSmallToString   11594.878 11588.499 +0.06%
JacksonComparisonBenchmark.kotlinToOkio   692.897    
JacksonComparisonBenchmark.kotlinToStream   753.526 49.273 +1 429.29%
JacksonComparisonBenchmark.kotlinToString   1633.087 1622.768 +0.64%
JacksonComparisonBenchmark.kotlinToStringWithEscapes 1103.148 1138.291 -3.09%

Copy link
Member

@sandwwraith sandwwraith left a comment

Great work!

@sandwwraith sandwwraith merged commit 5e8ccad into dev Jun 30, 2022
3 checks passed
/**
* Serializes the [value] with [serializer] into a [target] using JSON format and UTF-8 encoding.
*
* If [target] is not a [BufferedSink] then called [Sink.buffer] for it to create buffered wrapper.
Copy link
Contributor

@martinbonnin martinbonnin Jun 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I'm late to the party! Why not making the "buffer" requirement in the API itself?

public fun <T> Json.encodeToSink(
    serializer: SerializationStrategy<T>,
    value: T,
    target: BufferedSink
)

The current API fails if someone expects to be able to resume reading the stream after decoding an element.

See #1789 (comment) for a related discussion.

Copy link
Contributor

@JakeWharton JakeWharton Jun 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For encoding this isn't a problem. You are always free to call buffer() and then emit() as implementation details.

The problem you are referring to, however, is for decoding. Decoding absolutely should take a BufferedSource and not a Source. This is what Moshi and Wire do as well.

https://github.com/square/moshi/blob/moshi-parent-1.13.0/moshi/src/main/java/com/squareup/moshi/JsonAdapter.java#L57

https://github.com/square/wire/blob/4.4.0/wire-library/wire-runtime/src/commonMain/kotlin/com/squareup/wire/ProtoAdapter.kt#L145

Copy link
Contributor

@martinbonnin martinbonnin Jun 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right! Sorry for the confusion, I never realised how asymmetric this was 👍 . Maybe it's worth removing the "BufferSink" mention from the kdoc then? Since it's an implementation detail, the caller doesn't really need to know about it?

And in all cases, make the BufferedSource mandatory for decodeFromSource:

public fun <T> Json.decodeFromSource(
    deserializer: DeserializationStrategy<T>,
    source: BufferedSource
): T

Copy link
Contributor

@JakeWharton JakeWharton Jun 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree on both points!

Copy link
Member

@sandwwraith sandwwraith Jul 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your suggestions! Looks like we probably need to change existing functions working with Java streams applying the same logic.

However, this may lead to some asymmetry in the API. What do you think would be better — change both encodeToSink/decodeFromSource so they accept buffered versions or leave encodeToSink intact?

Copy link
Contributor

@martinbonnin martinbonnin Jul 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change both encodeToSink/decodeFromSource so they accept buffered versions or leave encodeToSink intact?

No strong opinion from me. Both Moshi and Wire use BufferedSink so I'd lean towards BufferedSink here as well for consistency/aesthetics reasons.

Since the sink is ending up being buffered in all cases, it's pretty easy to let the caller call .buffer() if required. Plus it might even save an emit() when writing multiple Json to the same BufferedSink (So tiny performance gain maybe...). But if there's a use case for passing a raw Sink, that works with me too.

}

override fun release() {
target.flush()
Copy link
Contributor

@JakeWharton JakeWharton Jun 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tempted to say that this should be a call to emit() which only pushes buffered data into the wrapped Sink allowing it to choose whether to write further down the chain. flush(), however, forces all of the bytes through the whole stack.

More here: https://jakewharton.com/forcing-bytes-downward-in-okio/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants