search

Frommi / miniz_oxide

Rust replacement for miniz
MIT License
168 stars 48 forks source link

Switch to SIMD adler32 #101

Closed Ben-Lichtman closed 3 years ago

Ben-Lichtman commented 3 years ago

There's a new simd adler32 crate in town so I figured it would be of use to switch to it.

https://github.com/mcountryman/simd-adler32

mcountryman commented 3 years ago

I'm working on a PR for this. Local benchmarks of compress_to_vec and decompress_to_vec show 30-60% speed improvements.

oyvindln commented 3 years ago

That seems quite a lot, I presume it would have the most effect on low compression levels.

Would prefer if it was added as an opt-in feature for now though, as the simd-adler32 crate contains a fair bit of unsafe code.

mcountryman commented 3 years ago

That seems quite a lot, I presume it would have the most effect on low compression levels.

Would prefer if it was added as an opt-in feature for now though, as the simd-adler32 crate contains a fair bit of unsafe code.

I took a closer look and my benchmarks were on a 1MiB array of ones at compression level 1 with the methods compress_to_vec_zlib and decompress_to_vec_zlib. This isn't really realistic so I tried testing with the data supplied in the data dir. Generally I saw ~10% improvements for compression levels 1 and 6.

Additionally, I did some profiling with different compression levels on a 1MiB array of 0xaf. And you are correct. High compression levels the performance benefit isn't really noticeable until you start dealing with GBs of data or many many iterations as seen below.

Profiling was done on a Windows 10 Pro - Intel i5-8300H @ 2.30GHz, with Intel VTune profiler.

mcountryman commented 3 years ago

Good news is that it looks like cargo-geiger excludes optional crates.

❯ cargo geiger
   Compiling autocfg v1.0.1
   Compiling libc v0.2.93
   Compiling cc v1.0.67
   Compiling crc32fast v1.2.1
    Checking adler v1.0.2
    Checking cfg-if v1.0.0
   Compiling miniz_oxide v0.4.4 (/Users/mcountryman/Development/miniz_oxide/miniz_oxide)
   Compiling miniz_oxide_c_api v0.2.5 (/Users/mcountryman/Development/miniz_oxide)
    Finished dev [unoptimized + debuginfo] target(s) in 3.13s
Failed to parse file: /Users/mcountryman/Development/miniz_oxide/src/lib_oxide.rs, Syn(Error("expected `:`"), "/Users/mcountryman/Development/miniz_oxide/src/lib_oxide.rs")                                                  
    Scanning done

Metric output format: x/y
    x = unsafe code used by the build
    y = total unsafe code found in the crate

Symbols: 
    🔒  = No `unsafe` usage found, declares #![forbid(unsafe_code)]
    ❓  = No `unsafe` usage found, missing #![forbid(unsafe_code)]
    ☢️   = `unsafe` usage found

Functions  Expressions  Impls  Traits  Methods  Dependency

4/4        118/121      0/0    0/0     2/2      ☢️  miniz_oxide_c_api 0.2.5
5/6        108/156      0/0    0/0     0/0      ☢️  ├── crc32fast 1.2.1
0/0        0/0          0/0    0/0     0/0      ❓ │   └── cfg-if 1.0.0
0/19       10/311       0/0    0/0     5/27     ☢️  ├── libc 0.2.93
0/0        0/0          0/0    0/0     0/0      🔒 └── miniz_oxide 0.4.4
0/0        0/0          0/0    0/0     0/0      🔒     └── adler 1.0.2
oyvindln commented 3 years ago

Feature for it was added in #104

oyvindln commented 2 years ago

@mcountryman I noticed now that the PR added 'simd-adler32/std' to the rustc-dep-of-std. As I'm not really sure how to test if that's working correctly I wanted to ask if you have tested it. Also ping @alexcrichton if it's acceptable to add it. (If either of you have any suggestions for CI testing with that feature that would also be nice.)

I presume the compiler pins the release it uses and I'm bumping minor version so there shouldn't be any danger of breaking stuff, but if there should be any issues with the change here it ought to be looked at.

alexcrichton commented 2 years ago

AFAIK everything should work out alright, eventually someone will want to upgrade this in libstd and presumably the crate will be reviewed by the necessary folks at that time.