Faster minmax by using fold

[Philippe-Cholet]

No description provided.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.76%. Comparing base (6814180) to head (7cee79a).
⚠️ Report is 173 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #968      +/-   ##
==========================================
- Coverage   94.38%   93.76%   -0.62%     
==========================================
  Files          48       50       +2     
  Lines        6665     6323     -342     
==========================================
- Hits         6291     5929     -362     
- Misses        374      394      +20     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[phimuemue]

Thanks. I think I get the idea. fold pairwise and see if there's one excess element that has to be processed manually.

Do you have some numbers that show that the proposed implementation indeed is faster? (My gut thought: For a slice, the existing implementation can just call next twice, whereas your proposal has to match acc twice, even though that seems unneeded.)

I'll approve this so that you can merge if you want. But I'd be happy about benchmarks that substantiate that we're taking the right direction.

[gendx]

I've also been exploring another approach, using the .tuples() adaptor to always fetch elements two-by-two in the main loop, and handle the last element separately if the size was odd: https://play.rust-lang.org/?version=stable&mode=release&edition=2024&gist=92a14c034e9fe662267ae892ee1029b5

This gives a concise and elegant implementation (feel free to pick it under the Itertools Apache/MIT license):

fn minmax(x: &[u32]) -> MinMaxResult<&u32> {
    let mut tuples = x.iter().tuples();
    let result = tuples
        .by_ref()
        .map(|(a, b)| if a < b { (a, b) } else { (b, a) })
        .reduce(|(a, b), (c, d)| (a.min(c), b.max(d)));

    // Handle the last element, if any.
    match (result, tuples.into_buffer().next()) {
        (None, None) => MinMaxResult::NoElements,
        (None, Some(x)) => MinMaxResult::OneElement(x),
        (Some((min, max)), None) => MinMaxResult::MinMax(min, max),
        (Some((min, max)), Some(x)) => MinMaxResult::MinMax(min.min(x), max.max(x)),
    }
}

Intuitively, this should play nice with the amount of branches in the hot loop, and indeed the generated assembly is shorter than the vanilla itertools implementation: https://play.rust-lang.org/?version=stable&mode=release&edition=2024&gist=d7f89e74389221fdb6c89b5a4dfc5b2c

Note that I haven't tested nor benchmarked any of these approaches.

[scottmcm]

That two-phase phrasing it pretty nice 👍

Might want to double-check the edge cases, though. I think

.map(|(a, b)| if a < b { (a, b) } else { (b, a) })

should probably be

.map(|(a, b)| if a > b { (b, a) } else { (a, b) })

because if they're ord-equivalent-but-not-actually-identical you want them to stay in the original order.

(If there's not already, should have some tests for .minmax_by_key(|pair| pair.0) or something to ensure this.)

[gendx]

because if they're ord-equivalent-but-not-actually-identical you want them to stay in the original order.

Good point! I missed that: the minmax_by(_key) documentation specifies this behavior, but .minmax() doesn't.