-
Notifications
You must be signed in to change notification settings - Fork 3
Initial commit of Okio ZStd #1
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
Conversation
This initial commit supports only the JVM. But it is structured to make it straightforward to support Kotlin/Multiplatform. Build infrastructure is all derived from Zipline.
I already ran a benchmark on this and all of my dreams have come true.
|
@@ -0,0 +1,5 @@ | |||
public final class okio/zstd/Zstd { |
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.
Very minimal API to start. I think we might later want to expose a mechanism to configure the compressor, but that’s not urgent at all.
build.gradle.kts
Dismissed
|
||
buildscript { | ||
repositories { | ||
mavenCentral() |
Check failure
Code scanning / Semgrep OSS
Gradle Config Vulnerable to Dependency Confusion Error
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.
The Gradle Repositories configuration was identifed as containing references to an external package registry. Block requires that we only pull down Artifacts from our internal registries and/or mirrors. All references to mavenCentral(), google(), jcenter() or ivy() must be removed
I don’t think this rule holds for open source projects.
build.gradle.kts
Dismissed
version = project.property("VERSION_NAME") as String | ||
|
||
repositories { | ||
mavenCentral() |
Check failure
Code scanning / Semgrep OSS
Gradle Config Vulnerable to Dependency Confusion Error
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.
The Gradle Repositories configuration was identifed as containing references to an external package registry. Block requires that we only pull down Artifacts from our internal registries and/or mirrors. All references to mavenCentral(), google(), jcenter() or ivy() must be removed
I don’t think this rule holds for open source projects.
lib.addIncludePath(b.path("../zstd/lib")); | ||
|
||
lib.linkLibC(); | ||
// TODO Tree-walk these two dirs for all C files. |
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 is the part of the PR where I hold the most shame. I just have to learn how to Zig instead of copy-pasting
} | ||
|
||
extern "C" JNIEXPORT jlong JNICALL | ||
Java_okio_zstd_JniZstdCompressor_compressStream2(JNIEnv* env, jobject type, jlong jniZstdPointer, jlong cctxPointer, jobject output, jobject input, jint mode) { |
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.
The 2
here is 'cause that’s the Zstd function I’m calling. I’m tempted to rename to just compress
internal expect fun zstdCompressor(): ZstdCompressor | ||
|
||
/** Returns a new decompressor. The caller must close it. */ | ||
internal expect fun zstdDecompressor(): ZstdDecompressor |
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.
These are the three 3 functions to implement on Kotlin/Native !
ZSTD_e_continue -> inputRemaining == 0L | ||
else -> inputRemaining == 0L && result == 0L | ||
} | ||
} while (!finished) |
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 whole loop should approximately mirror the Zstd sample:
https://github.com/facebook/zstd/blob/dev/examples/multiple_streaming_compression.c#L80
|
||
lastDecompressResult = result | ||
result.checkError() | ||
} |
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 whole loop should track another Zstd sample:
https://github.com/facebook/zstd/blob/dev/examples/streaming_decompression.c#L39
import okio.buffer | ||
import okio.sink | ||
|
||
class ZstdDecompressSourceTest { |
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.
These tests are currently in jvmTest
and not commonTest
cause I’m confirming interop with com.github.luben.zstd.ZstdOutputStream
.
I wanna add some more golden file tests, and then change these to interop with our own compressor. (And vice-versa for the decompression test.)
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.
LGTM - might be a fun project for a fuzzer
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.
TIL json5
* | ||
* Pass a pointer to this to all JNI functions that operate on JVM objects. | ||
*/ | ||
class JniZstd { |
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.
I'm not much use on this part of the review.
override fun write(source: Buffer, byteCount: Long) { | ||
check(!closed) { "closed" } | ||
|
||
inputBuffer.write(source, byteCount) |
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.
If this fails, do you still expect the caller to be a boy scout and clean up by calling close?
Since this is native code, should this have some fallback like a Cleaner task if caller doesn't release the native resources?
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.
Nothing for now. I’m mixed on whether to build that in.
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.
I feel like native resources you need to. Not cleaning up java classes eventually gets GCed. But the native code becomes a slow process death.
I guess there is leakcanary etc, to show unreleased resources.
This initial commit supports only the JVM. But it is structured to make it straightforward to support
Kotlin/Multiplatform.
Build infrastructure is all derived from Zipline.