Specialize flattening iterators with only one inner item #121204

cuviper · 2024-02-16T21:57:08Z

For iterators like Once and option::IntoIter that only ever have a
single item at most, the front and back iterator states in FlatMap and
Flatten are a waste, as they're always consumed already. We can use
specialization for these types to simplify the iterator methods.

It's a somewhat common pattern to use flatten() for options and
results, even recommended by multiple clippy lints. The
implementation is more efficient with filter_map, as mentioned in
clippy#9377, but this new specialization should close some of that
gap for existing code that flattens.

For iterators like `Once` and `option::IntoIter` that only ever have a single item at most, the front and back iterator states in `FlatMap` and `Flatten` are a waste, as they're always consumed already. We can use specialization for these types to simplify the iterator methods. It's a somewhat common pattern to use `flatten()` for options and results, even recommended by [multiple][1] [clippy][2] [lints][3]. The implementation is more efficient with `filter_map`, as mentioned in [clippy#9377], but this new specialization should close some of that gap for existing code that flattens. [1]: https://rust-lang.github.io/rust-clippy/master/#filter_map_identity [2]: https://rust-lang.github.io/rust-clippy/master/#option_filter_map [3]: https://rust-lang.github.io/rust-clippy/master/#result_filter_map [clippy#9377]: rust-lang/rust-clippy#9377

rustbot · 2024-02-16T21:57:16Z

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

cuviper · 2024-02-16T21:57:46Z

@bors try @rust-timer queue

Specialize flattening iterators with only one inner item For iterators like `Once` and `option::IntoIter` that only ever have a single item at most, the front and back iterator states in `FlatMap` and `Flatten` are a waste, as they're always consumed already. We can use specialization for these types to simplify the iterator methods. It's a somewhat common pattern to use `flatten()` for options and results, even recommended by [multiple][1] [clippy][2] [lints][3]. The implementation is more efficient with `filter_map`, as mentioned in [clippy#9377], but this new specialization should close some of that gap for existing code that flattens. [1]: https://rust-lang.github.io/rust-clippy/master/#filter_map_identity [2]: https://rust-lang.github.io/rust-clippy/master/#option_filter_map [3]: https://rust-lang.github.io/rust-clippy/master/#result_filter_map [clippy#9377]: rust-lang/rust-clippy#9377

bors · 2024-02-16T21:58:56Z

⌛ Trying commit 974bc45 with merge 4b67ea2...

cuviper · 2024-02-16T22:02:55Z

I have also been thinking about the opposite pattern with the "one-shot" on the outside, like an Option<I> where you call .into_iter().flatten(). I think this is also a candidate for streamlining, but it could be met with a new Option method for a simplified iterator wrapper.

the8472

Neat optimization.

I think next_chunk could also do what filter_map does here. But that optimization is a bit gnarly unsafe, so that can wait until we're closer to stabilizing the chunking iterators.

library/core/src/iter/adapters/flatten.rs

the8472 · 2024-02-16T22:41:50Z

library/core/src/iter/adapters/flatten.rs

+// These are always empty, which is fine to optimize too.
+impl<T> OneShot for Empty<T> {}
+impl<T> OneShot for array::IntoIter<T, 0> {}


I wonder if something flattening over those exists in the wild, but sure, why not.

Yeah, it would be weird, but maybe in some generic or macro code?
I wouldn't bother with this if it weren't trivial, for sure.

library/core/src/iter/adapters/flatten.rs

bors · 2024-02-16T23:27:09Z

☀️ Try build successful - checks-actions
Build commit: 4b67ea2 (4b67ea2e652830fe0a05868f9a698180a18295fa)

cuviper · 2024-02-17T00:06:45Z

It doesn't appear to be in the queue? Let's try again:

@rust-timer queue

the8472 · 2024-02-17T00:14:49Z

looks like it didn't pick up the try build.

@rust-timer build 4b67ea2

rust-timer · 2024-02-17T01:29:49Z

Finished benchmarking commit (4b67ea2): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.6%	[-0.6%, -0.6%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.6%	[-0.6%, -0.6%]	1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	4.9%	[4.9%, 4.9%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.6%	[-1.6%, -1.6%]	1
All ❌✅ (primary)	4.9%	[4.9%, 4.9%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.1%	[0.0%, 0.2%]	4
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.1%	[-0.2%, -0.1%]	7
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.1%	[-0.2%, 0.2%]	11

Bootstrap: 638.862s -> 638.174s (-0.11%)
Artifact size: 306.32 MiB -> 306.35 MiB (0.01%)

cuviper · 2024-02-17T02:25:22Z

Of course I hoped for more positive results, but neutral is ok. Maybe the compiler is already sufficiently avoiding such flat-maps in its own hot loops. I can definitely show wins in microbenchmarks, so IMO it's still a potential boost for user code.

Kobzol · 2024-02-17T08:06:01Z

Usually no-op changes are half/half green/red when you click on the "Show non-relevant results", this one is mostly green. So there might be some tiny win. In any case, it doesn't seem to be a regression :)

the8472 · 2024-02-17T10:09:35Z

@bors r+

bors · 2024-02-17T10:09:37Z

📌 Commit c36ae93 has been approved by the8472

It is now in the queue for this repository.

bors · 2024-02-17T20:18:57Z

⌛ Testing commit c36ae93 with merge 6672c16...

bors · 2024-02-17T22:16:14Z

☀️ Test successful - checks-actions
Approved by: the8472
Pushing 6672c16 to master...

rust-timer · 2024-02-18T00:29:07Z

Finished benchmarking commit (6672c16): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.6%	[0.6%, 0.6%]	1
Improvements ✅ (primary)	-0.6%	[-0.6%, -0.6%]	1
Improvements ✅ (secondary)	-2.6%	[-2.6%, -2.6%]	1
All ❌✅ (primary)	-0.6%	[-0.6%, -0.6%]	1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.6%	[1.6%, 1.6%]	1
Improvements ✅ (primary)	-2.9%	[-3.7%, -2.2%]	5
Improvements ✅ (secondary)	-2.6%	[-3.7%, -1.6%]	16
All ❌✅ (primary)	-2.9%	[-3.7%, -2.2%]	5

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.4%	[-1.4%, -1.4%]	1
All ❌✅ (primary)	-	-	0

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.1%	[0.0%, 0.2%]	6
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.1%	[-0.1%, -0.1%]	7
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.0%	[-0.1%, 0.2%]	13

Bootstrap: 640.998s -> 641.933s (0.15%)
Artifact size: 306.40 MiB -> 306.43 MiB (0.01%)

rustbot assigned Mark-Simulacrum Feb 16, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 16, 2024

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 16, 2024

the8472 assigned the8472 and unassigned Mark-Simulacrum Feb 16, 2024

This comment has been minimized.

Sign in to view

the8472 reviewed Feb 16, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

Clarify the flatten specialization comment

c36ae93

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 17, 2024

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 17, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 17, 2024

bors merged commit 6672c16 into rust-lang:master Feb 17, 2024
12 checks passed

rustbot added this to the 1.78.0 milestone Feb 17, 2024

cuviper deleted the flatten-one-shot branch March 10, 2024 21:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Specialize flattening iterators with only one inner item #121204

Specialize flattening iterators with only one inner item #121204

cuviper commented Feb 16, 2024

rustbot commented Feb 16, 2024

cuviper commented Feb 16, 2024

This comment has been minimized.

bors commented Feb 16, 2024

cuviper commented Feb 16, 2024

This comment has been minimized.

the8472 left a comment •

edited

Loading

the8472 Feb 16, 2024

cuviper Feb 16, 2024

bors commented Feb 16, 2024

cuviper commented Feb 17, 2024

This comment has been minimized.

the8472 commented Feb 17, 2024

This comment has been minimized.

rust-timer commented Feb 17, 2024

cuviper commented Feb 17, 2024

Kobzol commented Feb 17, 2024

the8472 commented Feb 17, 2024

bors commented Feb 17, 2024

bors commented Feb 17, 2024

bors commented Feb 17, 2024

rust-timer commented Feb 18, 2024

Specialize flattening iterators with only one inner item #121204

Specialize flattening iterators with only one inner item #121204

Conversation

cuviper commented Feb 16, 2024

rustbot commented Feb 16, 2024

cuviper commented Feb 16, 2024

This comment has been minimized.

bors commented Feb 16, 2024

cuviper commented Feb 16, 2024

This comment has been minimized.

the8472 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Feb 16, 2024

cuviper commented Feb 17, 2024

This comment has been minimized.

the8472 commented Feb 17, 2024

This comment has been minimized.

rust-timer commented Feb 17, 2024

Overall result: ✅ improvements - no action needed

cuviper commented Feb 17, 2024

Kobzol commented Feb 17, 2024

the8472 commented Feb 17, 2024

bors commented Feb 17, 2024

bors commented Feb 17, 2024

bors commented Feb 17, 2024

rust-timer commented Feb 18, 2024

Overall result: ❌✅ regressions and improvements - no action needed

the8472 left a comment •

edited

Loading