kn: gzip compression #1228

lauzadis · 2025-01-29T21:05:45Z

Implements gzip compression using zlib bindings (GzipCompressor)
Deduplicates cross-module definitions of decompressGzipBytes
- Required making this function @InternalApi public, which I think is a fine tradeoff

Issue #

Description of changes

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…1199)

…n-gzip

runtime/runtime-core/native/src/aws/smithy/kotlin/runtime/compression/GzipCompressor.kt

ianbotsf · 2025-01-30T17:45:15Z

runtime/runtime-core/native/src/aws/smithy/kotlin/runtime/compression/GzipCompressor.kt

+            var finished = false
+
+            while (!finished) {
+                val outputPin = buffer.pin()
+                stream.next_out = outputPin.addressOf(0).reinterpret()
+                stream.avail_out = BUFFER_SIZE.toUInt()
+
+                val deflateResult = deflate(stream.ptr, Z_FINISH)
+                if (deflateResult != Z_STREAM_END && deflateResult != Z_OK) {
+                    throw RuntimeException("Deflate failed during finish: $deflateResult")
+                }
+
+                val bytesWritten = BUFFER_SIZE - stream.avail_out.toInt()
+                outputBuffer.addAll(buffer.take(bytesWritten))
+
+                finished = deflateResult == Z_STREAM_END
+                outputPin.unpin()
+            }


Question: Can this be replaced by a call to consume(availableForRead)?

Not directly, because the data is still inside the stream, not in the internal buffer we manage. It could possibly be replaced with update(byteArrayOf()) followed by consume(availableForRead)

Actually an empty update won't work, because we need to specify Z_FINISH rather than Z_NO_FLUSH to ensure the entire contents are flushed out

runtime/runtime-core/native/src/aws/smithy/kotlin/runtime/compression/GzipCompressor.kt

ianbotsf · 2025-01-30T17:50:01Z

runtime/runtime-core/native/src/aws/smithy/kotlin/runtime/compression/GzipCompressor.kt

+    companion object {
+        internal const val BUFFER_SIZE = 16384
+    }


Question: Why is this the right buffer size? Would we ever want to use different sizes?

There's no reason, I just chose this number. I don't think it matters too much, it just controls how often we call deflate (whether the input passed to update can fit in single deflate call or not)

runtime/runtime-core/native/src/aws/smithy/kotlin/runtime/compression/GzipCompressor.kt

runtime/runtime-core/native/src/aws/smithy/kotlin/runtime/compression/GzipNative.kt

ianbotsf · 2025-01-30T19:25:12Z

runtime/runtime-core/native/src/aws/smithy/kotlin/runtime/io/GzipByteReadChannelNative.kt

+        // If still no data is available and the channel is closed, we've hit EOF. Close the compressor and write the remaining bytes
+        if (compressor.availableForRead == 0 && channel.isClosedForRead) {
+            val terminationBytes = compressor.close()
+            sink.write(terminationBytes)
+            return terminationBytes.size.toLong()
+        }


Correctness: Is it always true that availableForRead == 0 after compressor.update(...) means EOF? Zlib's manual seems to imply that calls to deflate might result in data being internally buffered but not necessarily producing output.

No it doesn't, but if the underlying channel is closed (second half of the condition), then it's safe to flush the compressor (preparing all the internally-buffered data) and write the remaining bytes

* add telemetry provider configuration * lint * address pr reviews * add changelog

ianbotsf · 2025-01-31T17:41:38Z

runtime/runtime-core/native/src/aws/smithy/kotlin/runtime/compression/GzipCompressor.kt

-
-    internal val isClosed
-        get() = _isClosed.value
+    private val outputBuffer = SdkByteChannel()


Question: Do we have to use a channel instead of a source/sink here? This makes all of the data methods suspend but I cannot see why they need to be, conceptually.

We can't use an SdkSource or an SdkSink because those only support either read or write but not both. I'll try SdkBuffer

Refactored to use SdkBuffer

ianbotsf · 2025-01-31T17:55:09Z

runtime/runtime-core/native/src/aws/smithy/kotlin/runtime/compression/GzipNative.kt

+                runBlocking {
+                    GzipSdkSource(bytes.source()).readToByteArray().asByteStream()
                }


Question: Why do we need to invoke a suspend method here? We're pulling this all into memory and blocking until it's done so it seems like we can avoid runBlocking/suspend altogether...

Related to #1228 (comment)

github-actions · 2025-02-03T16:03:32Z

Affected Artifacts

Changed in size

Artifact	Pull Request (bytes)	Latest Release (bytes)	Delta (bytes)	Delta (percentage)
http-test-jvm.jar	61,604	58,690	2,914	4.97%
aws-signing-default-jvm.jar	53,497	51,946	1,551	2.99%
crt-util-jvm.jar	21,451	20,952	499	2.38%
runtime-core-jvm.jar	835,835	818,814	17,021	2.08%
aws-signing-tests-jvm.jar	456,687	456,627	60	0.01%
test-suite-jvm.jar	96,930	97,206	-276	-0.28%

0marperez and others added 30 commits December 16, 2024 11:30

fix: create JMESPATH flattenIfPossible functions for Lists (#1169)

a4734c7

chore: release 1.3.30

7911a94

chore: bump snapshot version to 1.3.31-SNAPSHOT

7085c8a

misc: enhance support for replayable instances of InputStream (#1197)

4575023

chore: release 1.3.31

b0a4bac

chore: bump snapshot version to 1.3.32-SNAPSHOT

80f4538

fix: CBOR protocol test assertions / blob serialization (#1198)

e9d16a9

fix: correctly serialize subset of shape's members when configured (#…

48849a1

…1199)

chore: release 1.3.32

ded3a4b

chore: bump snapshot version to 1.3.33-SNAPSHOT

d97e8ba

Initial GZIP implementation

49f464b

Remove printlns and clean up

9ff5e32

ktlint

28d46fc

chore: add *-main to branch workflows (#1209)

78f17f3

chore: add .kotlin/ to .gitignore (#1208)

d06ee0a

chore: smithy version bump (#1213)

d2e7c91

chore: release 1.3.33

5a2df8f

chore: bump snapshot version to 1.3.34-SNAPSHOT

003633b

feat: add AuthTokenGenerator (#1212)

5f5ec8f

chore: release 1.3.34

3fe1b5e

chore: bump snapshot version to 1.3.35-SNAPSHOT

0bba308

Merge branch 'kn-main' of github.com:smithy-lang/smithy-kotlin into k…

fbcb79c

…n-gzip

misc: merge v1.4 into main (#1218)

52e4439

feat: support default checksums (#1191)

e0c25d6

chore: release 1.4.0

14e0958

chore: bump snapshot version to 1.4.1-SNAPSHOT

ed95d7b

fix: add 0.9.x aws-crt-kotlin transform (#1220)

a4ace35

fix: Ensure Host header is included when signing auth tokens (#1222)

15e5f0c

chore: release 1.4.1

447ac10

chore: bump snapshot version to 1.4.2-SNAPSHOT

0f8db44

lauzadis requested a review from a team as a code owner January 29, 2025 21:05

lauzadis added the no-changelog Indicates that a changelog entry isn't required for a pull request. Use sparingly. label Jan 29, 2025

This comment has been minimized.

Sign in to view

ianbotsf requested changes Jan 30, 2025

View reviewed changes

xinsong-cui and others added 8 commits January 30, 2025 16:34

misc: add telemetry configuration to DefaultAwsSigner (#1226)

e6357f9

* add telemetry provider configuration * lint * address pr reviews * add changelog

Use check, simplify atomic with boolean

477201d

Implement Closeable

c8408a7

ByteStream.fromBytes

45316dd

const

06621fa

Update check

4b4c628

Refactor to buffer output using SdkByteChannel

b882663

ktlint

2794930

This comment has been minimized.

Sign in to view

lauzadis added 2 commits January 31, 2025 10:44

Refactor to use asByteStream

755cc4b

Update Gzip ByteStream.Buffer compression

f3b8e7f

This comment has been minimized.

Sign in to view

lauzadis added 2 commits January 31, 2025 11:10

apiDump

9eea733

Use upload-artifact@v4

a4f7ba6

This comment has been minimized.

Sign in to view

ianbotsf reviewed Jan 31, 2025

View reviewed changes

lauzadis added 2 commits February 3, 2025 10:43

Refactor to use SdkBuffer

bc3221d

Remove stray runBlocking calls

002ae42

This comment has been minimized.

Sign in to view

Merge branch 'main' of github.com:smithy-lang/smithy-kotlin into kn-gzip

be2a662

ianbotsf approved these changes Feb 3, 2025

View reviewed changes

lauzadis merged commit 99d1214 into kn-main Feb 3, 2025
20 checks passed

kn: gzip compression #1228

kn: gzip compression #1228

Uh oh!

Conversation

lauzadis commented Jan 29, 2025

Issue #

Description of changes

Uh oh!

This comment has been minimized.

This comment has been minimized.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Feb 3, 2025

Affected Artifacts

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants