Implement custom fold for WhileSome

[Owen-CH-Leung]

#755

This PR introduces a custom fold method for WhileSome. The optimization hinges on the underlying iterator's fold:

With Custom fold: If the underlying iterator provides its own specialized fold, the performance gain depends on its implementation.
Without Custom fold: In the absence of a specialized fold for the underlying iterator, the performance remains on par with the default.

Benchmark Results:

Default fold: time: [497.09 ns 497.45 ns 497.82 ns]
Custom fold: time: [456.09 ns 458.15 ns 460.80 ns]

[jswrenn]

I think it's possible to implement this without the unwrap, using try_fold:

Suggested change

	fn fold<B, F>(self, acc: B, f: F) -> B
	where
	Self: Sized,
	F: FnMut(B, Self::Item) -> B,
	{
	self.iter
	.take_while(|opt| opt.is_some())
	.map(|item| item.unwrap())
	.fold(acc, f)
	}
	fn fold<B, F>(mut self, init: B, mut f: F) -> B
	where
	Self: Sized,
	F: FnMut(B, Self::Item) -> B,
	{
	// Replace with `ControlFlow`, once MSRV >= 1.55.0
	use core::result::Result::{Ok as Continue, Err as Break};
	let res = self.iter.try_fold(init, |acc, item| match item {
	Some(item) => Continue(f(acc, item)),
	None => Break(acc),
	});
	let (Break(res) | Continue(res)) = res;
	res
	}

[Owen-CH-Leung]

@jswrenn Thanks for your feedback! I'm a bit hesitate to override fold with try_fold though.

The names of the methods in Rust are usually semantically clear. When overriding fold, it makes sense to delegate its responsibility to the underlying iterator's fold. Similarly, try_fold should be reserved for overriding try_fold.

What do you think ?

[jswrenn]

In the Rust standard library, it's quite common for fold to be implemented in terms of try_fold. In your original code you construct a TakeWhile and call fold on it. But TakeWhile's fold implementation delegates to try_fold!

The general rule to follow is something like this:

• If you need to completely consume an iterator, use for_each or fold.
  • Use for_each if you don't need to maintain an accumulator.
  • Use fold if you do need to maintain an accumulator.
• If you need to partly consume an iterator, use try_for_each or try_fold.
  • Same rules about accumulators apply.

Here, try_fold is the perfect tool: We need to consume the iterator partially, up until the first None, with an accumulator.

[Philippe-Cholet]

Untested, I think the bench part should be

let result = data.fold(String::new(), |mut acc, ch| {
    acc.push(ch);
    acc
});
assert_eq!(result, "0123456789abcdef"); // or result.as_str() if needed

[Owen-CH-Leung]

Untested, I think the bench part should be

let result = data.fold(String::new(), |mut acc, ch| {
    acc.push(ch);
    acc
});
assert_eq!(result, "0123456789abcdef"); // or result.as_str() if needed

Interestingly, when I re-benchmark again using the revised benchmark (using both fold / try_fold), the performance gain disappears.

default: time: [59.414 ns 59.554 ns 59.697 ns]
overriden: time: [84.891 ns 85.273 ns 85.691 ns]

Seems that the overhead from calling to_string() is far more heavier than just push(ch)

[Philippe-Cholet]

Each time in fold, to_string here creates a non-empty string and therefore heap-allocates (slow) while push(ch) merely mutates the existing acc string, so yes way heavier and the reason behind my quick comment, I should have said something.
The assert_eq was probably a bit slow too for allocating to a vector while my suggestion would not.

[Philippe-Cholet]

I guess I can't merge this because an owner (jswrenn) requested changes.