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 94.67%. Comparing base (6814180) to head (f798d5a).
Report is 111 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #968      +/-   ##
==========================================
+ Coverage   94.38%   94.67%   +0.28%     
==========================================
  Files          48       49       +1     
  Lines        6665     7232     +567     
==========================================
+ Hits         6291     6847     +556     
- Misses        374      385      +11     

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

[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.)