search

rust-lang / libz-sys

Rust crate package to link to a system libz (zlib)
Apache License 2.0
119 stars 77 forks source link

Should support alternative implementations of zlib #63

Closed joshtriplett closed 4 years ago

joshtriplett commented 4 years ago

libz-sys should have a feature to use an alternative zlib implementation, such as zlib-ng. That would reduce the problem such alternative implementations currently have, of potentially ending up with two conflicting versions of zlib in the same binary. (This could still happen if linking to a system library that links to zlib, though.)

brainstorm commented 4 years ago

Indeed, would you accept repointing of the submodule to this implementation instead?:

https://blog.cloudflare.com/cloudflare-fights-cancer/ https://github.com/cloudflare/zlib

I think this would speed up a few crates that use libz-sys significantly "for free" (see benchmark plots on the blogpost).

brainstorm commented 4 years ago

For now I'll try to use this crate: https://crates.io/crates/cloudflare-zlib-sys

joshtriplett commented 4 years ago

@brainstorm Not "repointing", no; cloudflare's zlib has portability issues. It also isn't the highest performance; see https://github.com/Byron/gitoxide/issues/1#issuecomment-672626465 for some informal benchmarks.

brainstorm commented 4 years ago

Super interesting, thanks for sharing the benchmarks! I'll try to carve some time to give zlib-ng a go (according to https://github.com/rust-lang/libz-sys/commit/754ca14ba8b562afeaf83cc4b44c32d51fc430a6) ... I hope it's not x86_64-unknown-linux-musl problematic, I've not seen the target in appveyor.yml. Thanks for your reply and insight in any case!

joshtriplett commented 4 years ago

@brainstorm It should work fine on musl. Please give it a try and let me know if it works.

joshtriplett commented 4 years ago

@brainstorm Currently fixing some issues; new version shortly, which I've now confirmed works on musl (and which will test musl in CI).

joshtriplett commented 4 years ago

Done and released in libz-sys 1.1.0.

brainstorm commented 4 years ago

@brainstorm It should work fine on musl. Please give it a try and let me know if it works.

~Getting an interesting segfault on our CI~ correction: just an assert failure about some boundary assumptions [made on bgzf.c:461 from htslib](https://github.com/samtools/htslib/blob/fd0f89554459b78c07303e2c8a42acacd6851b46/bgzf.c#L461 (our C bindgen) ) so I'll have to look closer at zlib-ng and that assert:

https://github.com/rust-bio/rust-htslib/pull/237/checks?check_run_id=1024717376#step:7:1962

Will investigate on rust-bio/rust-htslib's PR.

joshtriplett commented 4 years ago

@brainstorm Ah, interesting. Yeah, that assert isn't valid with an alternate zlib implementation; compressBound is allowed to return a different value. It's a worst-case bound, not a normal case.