Skip to content

Conversation

swankjesse
Copy link
Collaborator

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.

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.
@swankjesse
Copy link
Collaborator Author

I already ran a benchmark on this and all of my dreams have come true.

Benchmark                     (algorithm)  (sampleDataSize)  Mode  Cnt   Score    Error  Units
CompressBenchmark.compress        deflate           8388608  avgt    3  86.539 ± 14.696  ms/op
CompressBenchmark.compress           gzip           8388608  avgt    3  84.996 ± 20.841  ms/op
CompressBenchmark.compress           zstd           8388608  avgt    3  13.248 ±  1.842  ms/op
CompressBenchmark.compress           none           8388608  avgt    3   0.401 ±  0.052  ms/op
CompressBenchmark.decompress      deflate           8388608  avgt    3  11.126 ±  1.089  ms/op
CompressBenchmark.decompress         gzip           8388608  avgt    3   9.761 ±  0.920  ms/op
CompressBenchmark.decompress         zstd           8388608  avgt    3   4.339 ±  0.562  ms/op
CompressBenchmark.decompress         none           8388608  avgt    3   0.407 ±  0.023  ms/op

@@ -0,0 +1,5 @@
public final class okio/zstd/Zstd {
Copy link
Collaborator Author

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

Gradle Config Vulnerable to Dependency Confusion
Copy link
Collaborator Author

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

Gradle Config Vulnerable to Dependency Confusion
Copy link
Collaborator Author

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.
Copy link
Collaborator Author

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) {
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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)
Copy link
Collaborator Author

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()
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

import okio.buffer
import okio.sink

class ZstdDecompressSourceTest {
Copy link
Collaborator Author

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.)

@swankjesse swankjesse requested a review from yschimke July 11, 2025 21:24
Copy link
Collaborator

@yschimke yschimke left a 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

Copy link
Collaborator

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 {
Copy link
Collaborator

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)
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator

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.

@swankjesse swankjesse merged commit 6fe3fa9 into main Jul 12, 2025
6 checks passed
@swankjesse swankjesse deleted the jwilson.0711.zstd branch July 12, 2025 20:31
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.

3 participants