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

rewrite of shootout-fasta.rs #10933

Closed
wants to merge 1 commit into from

Conversation

TeXitoi
Copy link
Contributor

@TeXitoi TeXitoi commented Dec 12, 2013

improvements:

  • no managed box
  • no virtual calls
  • no useless copy
  • optimizations (bisect is slower, limit tests, BufferedWriter...)
  • pass shootout test
  • 10 times faster

@emberian
Copy link
Member

Please change 'self to 'a or some other name; it is no longer allowed on master.

fn new<'a>(rng: &'a mut MyRandom, aa: &[(char, f32)]) -> AAGen<'a> {
let mut data = ~[];
let mut cum = 0.0;
data.reserve(aa.len());
Copy link
Member

Choose a reason for hiding this comment

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

I think you can write this function as:

let mut cum = 0.0;
let data = aa.iter().map(|&(ch, p)| { cum += p; (cum, ch as u8) }).collect();
AAGen { rng: rng, data: data }

(with line breaking to taste.)

@TeXitoi
Copy link
Contributor Author

TeXitoi commented Dec 12, 2013

With all the comment and using integer arithmetic for a big speedup (should be about as fast as the best official implementation).

wr: &mut W, header: &str, mut it: I, mut n: uint)
{
wr.write(header.as_bytes());
let mut line = [0u8, ../*LINE_LENGTH*/ 60 + 1];
Copy link
Member

Choose a reason for hiding this comment

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

Why can't this just be LINE_LENGTH + 1?

@TeXitoi
Copy link
Contributor Author

TeXitoi commented Dec 12, 2013

The previous commit does not include the integer arithmetic, this one is better.

If we want safe code, we can remove it at the cost of 5% here.

@TeXitoi
Copy link
Contributor Author

TeXitoi commented Dec 12, 2013

Finally, unsafe_set is not visible, but the zip version by @huonw is approx 7% slower. So no unsafe code, but a quite ugly loop with indexing.

improvements:
 - no managed box
 - no virtual calls
 - no useless copy
 - optimizations (bisect is slower, limit tests, BufferedWriter...)
 - pass shootout test
 - should be as fast as the best official test

Thanks to @cmr and @eddyb for their help!
@TeXitoi
Copy link
Contributor Author

TeXitoi commented Dec 12, 2013

@huonw version discussed in the outdated diff is basically for a in line.mut_iter().zip(it.by_ref()).take(nb) {…. It is 7% slower.

@huonw
Copy link
Member

huonw commented Dec 12, 2013

(cc @alexcrichton ^, since, iirc, one doesn't get notifications from the main PR after commenting on an individual commit.)

@alexcrichton
Copy link
Member

huh, who knew!

bors added a commit that referenced this pull request Dec 14, 2013
…chton

improvements:
 - no managed box
 - no virtual calls
 - no useless copy
 - optimizations (bisect is slower, limit tests, BufferedWriter...)
 - pass shootout test
 - 10 times faster
@bors bors closed this Dec 14, 2013
@TeXitoi TeXitoi deleted the shootout-fasta-rewrite branch April 21, 2014 22:11
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jun 30, 2023
[`useless_vec`]: lint on `vec![_]` invocations that adjust to a slice

Fixes rust-lang#2262 (well, actually my PR over at rust-lang#10901 did do most of the stuff, but this PR implements the one last other case mentioned in the comments that my PR didn't fix)

Before this change, it would lint `(&vec![1]).iter().sum::<i32>()`, but not `vec![1].iter().sum::<i32>()`. This PR handles this case.
This also refactors a few things that I wanted to do in my other PR but forgot about.

changelog: [`useless_vec`]: lint on `vec![_]` invocations that adjust to a slice
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.

5 participants