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

Use Kotlinx.IO #212

Open
mani1232 opened this issue Jul 17, 2024 · 8 comments
Open

Use Kotlinx.IO #212

mani1232 opened this issue Jul 17, 2024 · 8 comments

Comments

@mani1232
Copy link

mani1232 commented Jul 17, 2024

https://github.com/Kotlin/kotlinx-io

@krzema12
Copy link
Owner

At some point, I'd love to, but for now it's marked as "alpha" and "incubator" - doesn't look stable. Let's wait until it's at least beta?

@aSemy
Copy link
Collaborator

aSemy commented Jul 18, 2024

I haven't looked at kotlinx-io at all, but I strongly suspect (since it's in alpha) it doesn't support all of the features that we use... yet.

@OptimumCode
Copy link
Collaborator

OptimumCode commented Jul 29, 2024

I think it would be great to do something similar to what is done in kotlinx-serialization-json for okio and kotlin-io. They have a few abstractions like InternalJsonWriter and InternalJsonReaderCodePointImpl in kotlinx-json library and different implementations for okio and kotlin-io in separate modules. Probably, the same can be done here to support both okio and kotlin-io.

In this case, we would be able to create independent implementations for okio and kotlin-io (which can be marked as experimental) and people can choose what to use.

@OptimumCode
Copy link
Collaborator

OptimumCode commented Jul 29, 2024

I haven't looked at kotlinx-io at all, but I strongly suspect (since it's in alpha) it doesn't support all of the features that we use... yet.

I might be missing something but from my point of view the only things used right now from okio are:

  • BufferedSource.readUtf8() it/krzeminski/snakeyaml/engine/kmp/scanner/StreamReader.kt:189
  • okio.ByteString.Companion.encodeUtf8 it/krzeminski/snakeyaml/engine/kmp/scanner/StreamReader.kt:89
  • Buffer.writeUtf8 several places (it/krzeminski/snakeyaml/engine/kmp/api/lowlevel/ComposeCommon.kt:34)
  • okio.ByteString.Companion.decodeHex it/krzeminski/snakeyaml/engine/kmp/api/YamlUnicodeReader.kt:117

From a first look, all of that can be found in kotlinx-io (with some different naming but it can be found)

@aSemy
Copy link
Collaborator

aSemy commented Jul 31, 2024

think it would be great to do something similar to what is done in kotlinx-serialization-json for okio and kotlin-io.

I do like this idea, but I think it makes sense for Kaml to do this, not SnakeKMP. Maybe SnakeKMP does need to abstract all of the IO stuff and rely on Kaml converting Okio/kotlinx-io types into the abstractions.

From a first look, all of that can be found in kotlinx-io (with some different naming but it can be found)

My suspicion could be wrong :) But from looking at the kotlinx-io PR for Kaml, it requires using InternalIoApi. We definitely need to avoid that.

@OptimumCode
Copy link
Collaborator

OptimumCode commented Jul 31, 2024

But from looking at the kotlinx-io PR for Kaml, it requires using InternalIoApi. We definitely need to avoid that.

It looks like a mistake in PR. The buffer from kotlinx-io is both Sink and Source when in okio the buffer() method returns BufferedSource.
But in any case, using library's internal api should be avoided as much as possible.

Maybe SnakeKMP does need to abstract all of the IO stuff and rely on Kaml converting Okio/kotlinx-io types into the abstractions.

This idea sounds great. Creating an abstractions from a particular IO library will give more flexibility for end users. And this definitely should be done as the first step to support different IO libraries.
But I still think that it would be better to have integrations with okio and kotlinx-io maintained in this repo. This would give us more insight on how good the SnakeKMP integrates with those IO libraries (by performing tests and benchmarks for everything together) and ability to define some internal API to optimize the integration.

@aSemy
Copy link
Collaborator

aSemy commented Aug 30, 2024

But I still think that it would be better to have integrations with okio and kotlinx-io maintained in this repo. This would give us more insight on how good the SnakeKMP integrates with those IO libraries (by performing tests and benchmarks for everything together) and ability to define some internal API to optimize the integration.

Don't get me wrong, I'm not disagreeing: better support for Okio and kotlinx-io would certainly be a good thing, and I'm totally on board. However, I see SnakeKMP is the supporting, utility focused backend, and Kaml is the friendly, user facing frontend. SnakeKMP should focus on performance and correctness, while Kaml should focus on usability. (Ideally SnakeKMP would have zero dependencies, but the Kotlin multiplatform stdlib doesn't have enough IO stuff yet.)

So, when it comes to SnakeKMP I think the best strategy is to pick the IO library that works best for performance and integration with Kaml. Currently I think Okio is best suited to support that, since it's older and has more support. But definitely when kotlinx-io becomes stable it makes sense to use it instead, because it's official.

And then it's up to Kaml to provide Okio/kotlinx-io/java.io/whatever-else integrations.

At least, that's how I see it!

@OptimumCode
Copy link
Collaborator

I think I got your point and I agree with you, especially with

Ideally SnakeKMP would have zero dependencies

Maybe it is worth parking this question until Kotlin has standardized IO interfaces...

But if SnakeKMP had an extension point (like kotlinx-serizalization-json does), it would allow to do something about the above right now (for Kaml or any other library/application).
Of course, if this extension point causes a big performance degradation that would not be worth it.

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

No branches or pull requests

4 participants