-
-
Notifications
You must be signed in to change notification settings - Fork 41
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
bam: Use libdeflate instead of the standard compress/{gzip,flate} #129
Conversation
It results in 2x to 4x performance improvements. github.com/biogo/hts/bgzf Before: BenchmarkWrite-56 1 2087464279 ns/op BenchmarkRead-56 1 4452965285 ns/op After: BenchmarkWrite-56 1 1069021742 ns/op BenchmarkRead-56 1 1195341566 ns/op Hardware: 56 * Xeon E5-2690@2.60GHz, 256GiB memory.
It addresses the issue (Most of the work was done by Chris, by the way) |
I will be able to have a look at this in a few days. |
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.
This can't go in as it is. The condition of adding a Cgo dependency was that it was conditional on build tags and off by default. This is always on.
Maybe you can use pgzip, I use it a lot.
|
Yes, that's a possibility, though bgzf already uses parallelism. The alternative is "github.com/klauspost/compress/gzip" which has optimisations that I don't believe are in std. |
On Sat, Dec 15, 2018 at 6:54 PM Dan Kortschak ***@***.***> wrote:
***@***.**** commented on this pull request.
This can't go in as it is. The condition of adding a Cgo dependency was
that it was conditional on build tags and off by default
<#124 (comment)>. This is
always on.
libdeflate itself is conditional, so it builds on both cgo and pure
environments (in latter, it just uses compress/flate). Does that not
suffice?
… —
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#129 (review)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAhVsw52SQrIFyfJuiA11jOpGOqx1F7Vks5u5bXNgaJpZM4ZQP7c>
.
--
Yasushi Saito
|
On Sat, Dec 15, 2018 at 9:32 PM Dan Kortschak ***@***.***> wrote:
Yes, that's a possibility, though bgzf already uses parallelism. The
alternative is "github.com/klauspost/compress/gzip" which has
optimisations that I don't believe are in std.
We've used klauspost/compress internally but it provides only minor
improvements over the standard packages - most of its improvements have
been upstreamed already.
… —
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#129 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAhVs1w5y7zEMCTuTWqPpTL-rOvgSa4Rks5u5drtgaJpZM4ZQP7c>
.
--
Yasushi Saito
|
OK. So I'll have to look at libdeflate. That will take some time. From a quick look though, the fact that libdeflate is part of grailbio/base means that all those other packages are pulled just to get libdeflate. I'd be happier if it were in its own repo. Also, there should be additional tests elements in the traivs matrix to check the Cgo and pure-Go implementation and documentation in bgzf to explain how to get the different builds. Finally, I see that you have code that is derivative of Klaus' code in libdeflate.go, this should have headers showing Go Authors and Klaus' copyrights on that in addition to Grail's (also the Go and Klaus' LICENSE files).
|
Closing due to absence of a response. |
It results in 2x to 4x performance improvements.
github.com/biogo/hts/bgzf
Before:
BenchmarkWrite-56 1 2087464279 ns/op
BenchmarkRead-56 1 4452965285 ns/op
After:
BenchmarkWrite-56 1 1069021742 ns/op
BenchmarkRead-56 1 1195341566 ns/op
Hardware: 56 * Xeon E5-2690@2.60GHz, 256GiB memory.