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

Use u32::reverse_bits to reverse bits #91

Merged
merged 1 commit into from
Jan 26, 2022

Conversation

nico-abram
Copy link
Contributor

I wanted to try running dev/cmp to bench but I ran into #89

I ran the perf example on bwv_542_fuge.ogg about 10 times with and without the change. This is a horrible way of doing this and probably meaningless, but I wanted to at least get some numbers before opening this issue. I would not really expect a perf regression from this, but just in case I feel like it should be benchmarked.

Max/min with change: 1.0420328 s/1.0198687 s
Max/min without change: 1.0231534 s/1.1075205 s

@nico-abram
Copy link
Contributor Author

ubuntu-latest seems to be running rust 1.36, reverse_bits is available since 1.37

I can update the MSRV in the readme (Which says it is 1.36) for the PR, although I don't know if changing that for the next release would actually be acceptable. If it isn't, should this PR just be left open until it is?

@est31
Copy link
Member

est31 commented Nov 22, 2020

Thanks this is a good catch! As you already found out, lewton currently has a MSRV of 1.36.0. I'm not sure I want to increase it for this small improvement alone, especially as it requires a semver breaking update. Especially there is still a sizeable chunk of downloads for versions prior to 0.10.

grafik

@nico-abram
Copy link
Contributor Author

Understandable! Should this PR be closed, or can it be left open until a time you consider bumping the MSRV?

@est31
Copy link
Member

est31 commented Nov 22, 2020

@nico-abram your choice!

@nico-abram
Copy link
Contributor Author

Leaving it open is fine by me

@est31
Copy link
Member

est31 commented Dec 7, 2020

As an alternative to leaving this PR open, one could change it to add a TODO comment, similar to what I've done in c059882

@est31 est31 force-pushed the master branch 2 times, most recently from 0631313 to 9e377e1 Compare January 20, 2021 19:31
@roderickvd
Copy link

I think Ubuntu is on Rust >= 1.47 now so this could go in?

@est31 est31 force-pushed the master branch 10 times, most recently from 1a9ca88 to f655fa2 Compare January 23, 2022 04:11
@est31 est31 merged commit fcbaf21 into RustAudio:master Jan 26, 2022
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