-
Notifications
You must be signed in to change notification settings - Fork 92
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
Added Ctx compress/decompress #83
Conversation
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.
Thanks for your contribution @merlimat!
This looks good, I just added some comments around SetFinalizer vs a Close
pattern.
Thanks as well for adding the tests and benchmarks, really highlight the usefulness of this addition.
zstd_ctx.go
Outdated
} | ||
|
||
func (c *ctx) Close() error { | ||
if err := getError(int(C.ZSTD_freeCCtx(c.cctx))); err != nil { |
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 think we should probably try to free both and return the first error if any:
err1 := getError(int(C.ZSTD_freeCCtx(c.cctx)))
err2 := getError(int(C.ZSTD_freeDCtx(c.dctx)))
if err1 != nil {
return err1
}
return err2
That way you don't prevent the second context to be freed if the first fails.
This should also probably be gated by a boolean to make sure we don't call freeCCtx
twice on the same pointer.
As pointed earlier though, I think in that particular case, a finalizer might be a better fit since all we do is freeing memory, it would be more user-friendly, not end of the world if the finalizer is not called and you are sure it's only called once
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.
The only thing with finalize function, I don't know what to do with the error codes since we're not bubbling it up. Is there any convention on how to log errors?
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.
Yes that's a good point.
Usually what I've seen in the go ecosystem is libraries use the standard log package (examples: zookeeper, sarama)
But most of the time, main program use another logging system (logrus, zap) so it's hard to provide standard logs.
For the current case, the error is not very actionable (if the free fails) so I think we are ok not logging it.
Need to revisit this though if we ever need to add additional logging
Co-authored-by: Vianney Tran <vianney.tran@datadoghq.com>
@Viq111 Thanks for the feedback, I've changed to use finalizer. |
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.
Thanks for your contribution, this looks great!
I'll merge it tomorrow morning EST (since it's nearing end of business day now) The benchmark on CircleCI gives the following benchmark:
So still see the improvement but not as much since For a small payload (I took
|
Motivation
According to https://facebook.github.io/zstd/zstd_manual.html#Chapter4 when doing repeated compression/decompression operations, it's recommended to use a context object for internal state to be preserved.
While this could be done similarly through the stream API, it doesn't come natural when compressing a
[]byte
and it doesn't provide a way to provide adst
buffer for the result.Modifications
In order to expose the
ZSTD_compressCCtx()
andZSTD_decompressDCtx()
, adding aCtx
interface:Example:
Microbenchmark
The benchmark shows that reusing the context improves the overall performance: