-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add Brotli support #810
Add Brotli support #810
Conversation
New Functions: CompressHandlerBrotliLevel(h RequestHandler, brotliLevel, otherLevel int) RequestHandler Request.BodyUnbrotli() ([]byte, error) Response.BodyUnbrotli() ([]byte, error) AppendBrotliBytesLevel(dst, src []byte, level int) []byte WriteBrotliLevel(w io.Writer, p []byte, level int) (int, error) WriteBrotli(w io.Writer, p []byte) (int, error) AppendBrotliBytes(dst, src []byte) []byte WriteUnbrotli(w io.Writer, p []byte) (int, error) AppendUnbrotliBytes(dst, src []byte) ([]byte, error) New Constants: CompressBrotliNoCompression CompressBrotliBestSpeed CompressBrotliBestCompression CompressBrotliDefaultCompression Brotli compression levels are different from gzip/flate. Because of this we have separate level constants and CompressHandlerBrotliLevel takes 2 levels. I didn't add Brotli support to CompressHandler as this could cause a spike in CPU usage when users upgrade fasthttp. fasthttp.CompressBrotliDefaultCompression is not the same as brotli.DefaultCompression. brotli.DefaultCompression is more than twice as slow as fasthttp.CompressBrotliDefaultCompression which I thought was unreasonable as default.
// * CompressBestCompression | ||
// * CompressDefaultCompression | ||
// * CompressHuffmanOnly | ||
func CompressHandlerBrotliLevel(h RequestHandler, brotliLevel, otherLevel int) RequestHandler { |
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 tried to find a shorter, and cleaner option for this, but still can't find a better way.
what I see here is that now, you would have to write
h = fasthttp.CompressHandlerBrotliLevel(h, fasthttp.CompressBrotliDefaultCompression, fasthttp.CompressDefaultCompression)
or
h = fasthttp.CompressHandlerBrotliLevel(h, 4, 6)
first is barely readable, and the second isn't obvious without IDE reminding you the order. maybe, you have some ideas on that?
otherwise, this looks good to me
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.
How is the first not readable? It's very readable as it spells our exactly what it is. Yeah the line is a bit long but who works in an editor with 80 characters max with these days...
Or if you think it's too long you can just write it like
h = fasthttp.CompressHandlerBrotliLevel(
h,
fasthttp.CompressBrotliDefaultCompression,
fasthttp.CompressDefaultCompression,
)
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 like to comment if the ”Compress...” is not need since it’s understood that Brotli is a compressed format or:
fasthttp.SetHandlerBrotliLevel(...)
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.
We have CompressHandler
and CompressHandlerLevel
. It would be super weird to call this similar method SetHandlerBrotliLevel
all of sudden. It's better to keep names similar.
New Functions:
New Constants:
Brotli compression levels are different from gzip/flate. Because of this we have separate level constants and
CompressHandlerBrotliLevel
takes 2 levels.I didn't add Brotli support to
CompressHandler
as this could cause a spike in CPU usage when users upgrade fasthttp.fasthttp.CompressBrotliDefaultCompression
is not the same asbrotli.DefaultCompression. brotli.DefaultCompression
is more than twiceas slow as
fasthttp.CompressBrotliDefaultCompression
which I thought wasunreasonable as default.