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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃搨 Add option for gzip_compression_level for Rack::Deflater. #1734

Closed

Conversation

bayleedev
Copy link

This would be useful on a system with smaller memory to be set to a lower number, and a system with higher compression needs to compress up to level 9.

:gzip_compression_level :: An integer from 0 to 9 controlling the
level of compression; 1 is fastest and produces the least compression,
and 9 is slowest and produces the most compression. 0 is no compression.
The default is 6. This is only needed for the gzip type encoding.

`:gzip_compression_level ::` An integer from 0 to 9 controlling the
level of compression; 1 is fastest and produces the least compression,
and 9 is slowest and produces the most compression. 0 is no compression.
The default is 6. This is only needed for the `gzip` type encoding.
@bayleedev bayleedev changed the title 馃搨 Add option for gzip_compression_level. 馃搨 Add option for gzip_compression_level for Rack::Deflater. Jan 27, 2021
@bayleedev bayleedev force-pushed the feature/gzip-compression-level branch from 2bc699e to 572fe62 Compare January 27, 2021 03:05
@bayleedev
Copy link
Author

@ioquatix
Copy link
Member

This looks good to me.

Do you think gzip_compression_level is too specific, or maybe compression_level is sufficient?

@jeremyevans
Copy link
Contributor

This also looks good to me. Currently, only gzip encoding is supported, but I could see br possibly being supported in the future, so gzip_compression_level may be fine as an option name. @ioquatix are you OK with the current option name?

@ioquatix
Copy link
Member

ioquatix commented Jan 27, 2021

I think if the compression algorithm could be different, which is possible and likely in the future, we should not embed the name of the compression algorithm in the option.

@bayleedev
Copy link
Author

bayleedev commented Jan 27, 2021

@ioquatix what were you thinking? gzip_options: { compression_level: 3 } ? Making it compression_level could make its use ambiguous with multiple compression algorithms.

EDIT:
gzip_options would require some refactoring as well - sync is a gzip specific option, hinting back at compression_level.

EDIT EDIT:
Personally, I think gzip_compression_level is the way to go. Happy to go either way on this though. Thank you!

@nateberkopec
Copy link

Just want to point out that brotli has a 10 compression level, zlib only goes up to 9 (insert spinal tap reference here)

@ioquatix
Copy link
Member

ioquatix commented Jan 27, 2021

I took a look at how this is implemented in other places.

In protocol-http (falcon) we use Zlib::Deflate which is non-algorithm-specific.

It would be nice to set options in a generic way.

I checked node.js and they expose gzip, deflate, brotli all through the same interface, but there is a way to set options. https://nodejs.org/api/zlib.html#zlib_brotli_constants

My suggestion would be something like:

deflater = Rack::Deflater.new(..., **options)
  # in initialize:
  @options = options

# In the deflater:
Zlib::Deflate.new(..., **options)

This is just pseudo code. This means we are not interpreting the value of the options which means we don't worry about naming or anything else.

@bayleedev
Copy link
Author

I love the idea. I'm unsure how to implement that idea. Since ZLib::Deflate and Zlib::GzipWriter both take level as an unnamed positional argument, not options. Thoughts?

@bayleedev
Copy link
Author

@ioquatix @nateberkopec any update on this? Happy to change the code. I'm mostly looking for the functionality to exist more than how I want the code to be structured. Let me know. [:

@bayleedev bayleedev closed this Apr 11, 2021
@bayleedev bayleedev deleted the feature/gzip-compression-level branch April 11, 2021 04:48
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.

None yet

4 participants