branchless .filter(_).count()

[llogiq]

I found that the branchless version is only slower if we have little to no branch misses, which usually isn't the case. I notice speedups between -5% (perfect prediction) and 60% (real world data).

[rust-highfive]

r? @sfackler

(rust_highfive has picked a reviewer for you, use r? to override)

[bluss]

Maybe one doesn't want to argue style for trivial things, but I find the code cramped when simple should work well:

fn count(mut self) -> usize {
    // Attempt to produce branchless count if possible
    let mut count = 0;
    for elt in self.iter {
        count += (self.predicate)(&elt) as usize;
    }
    count
}

The surrounding methods use .by_ref() which seems archaic (just &mut self.iter would be fine if mutable reference only was required)

[llogiq]

Good idea, will change shortly (perhaps also change the other methods en passant).

[bluss]

implementation looks good to me. Will wait for others to see if they agree with this special case.

[sfackler]

Thoughts @alexcrichton @aturon? I feel somewhat conflicted.

[alexcrichton]

Seems fine to me, although it'd be nice to have some tests specifically for this combinator now that we're specializing the implementation.

[BurntSushi]

Only one of the jobs failed, but the error looks related. Strange.

error[E0369]: binary operation `%` cannot be applied to type `&&{integer}`
   --> /checkout/src/libcore/../libcoretest/iter.rs:197:37
    |
197 |     assert_eq!(xs.iter().filter(|x| x % 2 == 0).count(), 5);
    |                                     ^
    |
note: an implementation of `std::ops::Rem` might be missing for `&&{integer}`
   --> /checkout/src/libcore/../libcoretest/iter.rs:197:37
    |
197 |     assert_eq!(xs.iter().filter(|x| x % 2 == 0).count(), 5);
    |       

[BurntSushi]

I think my concern with this is not the specific change, but rather, what's to stop someone from removing this specialization in the future because their particular data runs faster without it? Is a comment documenting it enough?

[bluss]

Only one job actually compiles rust and runs the tests, so that's expected.

@BurntSushi Do you mean that if this is added, that it's part of guaranteed behaviour? I don't think it should be, and it should be possible to reevaluate changes like this.

[BurntSushi]

@bluss No, I mean, what's to prevent someone, say, a year from now submitting a PR that removes this specialization for similar reasons that @llogiq provided? I'm not referring to the behavior, but rather, the state of adding/removing optimizations like this?

I suppose the best answer to my concern is an executable benchmark, but I'm not sure we have the infrastructure in place for that?

(And maybe this concern just doesn't matter much in practice.)

[alexcrichton]

@BurntSushi optimizations like this can be removed/added as we see fit, I don't think we're making a binding contract to implement this method exactly this way, so we're still open to future improvements if necessary.

In any case this looks good to go, so

@bors: r+

[bors]

📌 Commit bfabe81 has been approved by alexcrichton

[bors]

⌛ Testing commit bfabe81 with merge 9c8cdb2...