Skip to content
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

Scaffolding for direct benchmarking of crate::filter::unfilter. #413

Merged
merged 3 commits into from
Sep 23, 2023

Conversation

anforowicz
Copy link
Contributor

@anforowicz anforowicz commented Sep 21, 2023

Can you PTAL? The new benchmarks from this PR should help with work on performance improvements of fn unfilter.

The real motivation for this PR is that it is a prelude to a series of commits at bf2c26b...1c78a3f that results in a significant decoding speed-up (measured on Intel). Special thanks to @veluca93 for help with various SIMD questions that I had. Results that I got from running the benchmarks:

  • decode/kodim02.png: -26.6% time
  • decode/kodim17.png: -25.7% time
  • decode/kodim07.png: -27.1% time
  • decode/kodim23.png: -21.8% time
  • unfilter/filter=Sub/bpp=3: -71.5% time
  • unfilter/filter=Sub/bpp=6: -86.1% time
  • unfilter/filter=Avg/bpp=3: -71.8% time
  • unfilter/filter=Avg/bpp=6: -70.6% time
  • unfilter/filter=Paeth/bpp=3: -51.4% time
  • unfilter/filter=Paeth/bpp=6: -62.9% time
  • AFAICT other decode/... and unfilter/... benchmarks are unchanged

I note that the speed-up above depends on std::simd which hasn't yet been stabilized, so I thought that it would be best to have a separate PR for just the benchmarks. (i.e. the new benchmarks seem desirable on their own, even if we decide not to pursue the std::simd-based improvements)

/cc @calebzulawski as FYI, since the improvements are possible thanks to std::simd and rust-lang/rust#86656.

@calebzulawski
Copy link

That looks great! Feel free to file any issues or let me know if you see any cases where performance is worse than you'd expect.

@anforowicz
Copy link
Contributor Author

FWIW, I assume that the mips_cross CI failure is unrelated to this PR. Please shout if you think this is not the case.

@calebzulawski
Copy link

I don't think it's related (but we ran into similar). I believe mips was demoted to tier 3, so might not have up to date toolchains

@fintelia
Copy link
Contributor

Yeah, the mips failure is unrelated. In the main image crate we switched to powerpc for big endian testing (image-rs/image#1987), so probably just need to do the same thing here.

I should hopefully be able to take a look at the rest of the PR later tonight

src/common.rs Outdated
@@ -695,6 +687,18 @@ impl Info<'_> {
}

impl BytesPerPixel {
pub(crate) fn for_prediction(bpp: usize) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you call this from_usize or similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I assume that you plan to squash the last 2 small commits (cargo fmt + this rename) when merging?

@fintelia
Copy link
Contributor

Really excited to see the progress to enable using SIMD from safe Rust code! Avoiding, or ideally forbidding, unsafe code is one of the big differentiators that we can provide over the C libraries that are generally used for image decoding. But that's often meant that we either have to forgo potential performance improvements or tolerate some unsafe in order to use SIMD

@fintelia fintelia merged commit e11786e into image-rs:master Sep 23, 2023
18 of 19 checks passed
@anforowicz anforowicz deleted the unfilter-benchmarks branch September 28, 2023 00:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants