Skip to content

Conversation

@glbrntt
Copy link
Collaborator

@glbrntt glbrntt commented Jan 22, 2024

Motivation:

v2 should support compression.

Modifications:

  • Add a Zlib compressor and decompressor supporting deflate and gzip.
  • This is loosely based on the code we had in v1 but refactored to fit slightly different requirements.

Result:

Can compress and decompress messages using deflate/gzip

Motivation:

v2 should support compression.

Modifications:

- Add a Zlib compressor and decompressor supporting deflate and gzip.
- This is loosely based on the code we had in v1 but refactored to fit
  slightly different requirements.

Result:

Can compress and decompress messages using deflate/gzip
@glbrntt glbrntt requested a review from gjcairo January 22, 2024 14:12
@glbrntt glbrntt added the version/v2 Relates to v2 label Jan 22, 2024
Comment on lines 40 to 42
/// This compressor is only suitable for compressing whole messages at a time. Callers
/// must ``initialize()`` the compressor before using it and must ``reset()`` it between
/// subsequent calls to ``compress(_:into:)``.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason why we couldn't simplify this API by calling stream/deflateInit in the initialiser, and doing the logic of reset() before calling deflate in compress(_:into:)? It would make it impossible to make mistakes (by forgetting to call these methods) while using the compressor.
Same question about the Decompressor below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately not, initialising in the init results in a Z_STREAM_ERROR when you call compress (same goes for the decompress). The same is true if you use a static function to create and then initialize and return the initialized compressor/decompressor. I don't understand why that's the case though. In v1 we did do this in init but the compressor/decompressor were both classes but I'd rather save the allocation.

W.r.t. reset(), that seems like a reasonable idea. I'm not sure what the performance implications are but the docs say it does not free or reallocate internal state so should be okay to do.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, that's unfortunate about the init.

W.r.t reset() - if the performance was to be affected by running it always in the defer block, we could instead add another flag (isDirty or something like that, signalling that we've already called (de)compress) and if that's set at the beginning of the reset() call, then we reset just before doing the rest of the logic.
That way we'd be saving the cost of one reset() (the one that would otherwise happen after the last (de)compression) - but well, I don't know if we care enough about this for now :D

@gjcairo gjcairo mentioned this pull request Jan 22, 2024
@glbrntt glbrntt requested a review from gjcairo January 22, 2024 16:32
@glbrntt glbrntt merged commit 91f9bd8 into grpc:main Jan 22, 2024
@glbrntt glbrntt deleted the compression branch January 22, 2024 17:18
glbrntt added a commit to glbrntt/grpc-swift that referenced this pull request Feb 5, 2024
Motivation:

v2 should support compression.

Modifications:

- Add a Zlib compressor and decompressor supporting deflate and gzip.
- This is loosely based on the code we had in v1 but refactored to fit
  slightly different requirements.

Result:

Can compress and decompress messages using deflate/gzip
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

version/v2 Relates to v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants