refactor `notable_traits_button` to use iterator combinators instead of for loop #136816

yotamofek · 2025-02-10T12:28:34Z

Small cleanup.
Use Iterator::any instead of for loop with predicate = true;.
I think this makes the code more readable... and also has the additional benefit of short-circuiting the iterator when a notable trait is found (a break statement was missing in the for loop version, I think). Probably won't be significant enough to show on perf results, though.

Three commits, each attempting to optimize notable_trait_buttons by a little bit.

rustbot · 2025-02-10T12:28:43Z

r? @GuillaumeGomez

rustbot has assigned @GuillaumeGomez.
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

yotamofek · 2025-02-10T12:56:27Z

(speculating out loud to no one in particular)
Is there any reason why Context::types_with_notable_traits is an IndexSet instead of a HashSet?
Only time it's ever iterated over is in notable_traits_json, where it's immediately sorted, so I don't see the value of preserving insertion order with IndexSet. HashMap should be a little faster and smaller in memory footprint.

src/librustdoc/html/render/mod.rs

GuillaumeGomez · 2025-02-10T19:44:49Z

I think it might have an impact on performance, we'll see. I'll run a perf check once suggestion has been applied.

I personally tend to prefer for loops but I don't see a reason not to switch to iterator combinators here.

yotamofek · 2025-02-10T19:52:12Z

I personally tend to prefer for loops but I don't see a reason not to switch to iterator combinators here.

It's a matter of style and personal preference, I guess. I'm the opposite - I usually prefer "functional" style. Especially with stuff like .any(), like in this case. I think it makes the purpose of the loop clearer, i.e. this loop is looking for at least one case of element-that-satisfies-some-predicate.
But yeah, just a matter of taste. :)

GuillaumeGomez · 2025-02-10T19:53:48Z

Personal taste and stuff. I see improvement, personal taste comes second. 😉

Let's check perf.

@bors try @rust-timer queue

…leanup, r=<try> Refactor `notable_trait_buttons` to use iterator combinators instead of `for` loop Small cleanup. Use `Iterator::any` instead of `for` loop with `predicate = true;`. I think this makes the code more readable... and also has the additional benefit of short-circuiting the iterator when a notable trait is found (a `break` statement was missing in the `for` loop version, I think). Probably won't be significant enough to show on perf results, though.

bors · 2025-02-10T19:55:00Z

⌛ Trying commit fee4729 with merge b30f3e7...

yotamofek · 2025-02-10T19:56:18Z

Oh BTW, if we're talking perf,
if we're already populating a cache of types_with_notable_traits here, couldn't we just check if a type is already there first?

GuillaumeGomez · 2025-02-10T19:57:08Z

Sounds like a great idea!

yotamofek · 2025-02-10T20:07:10Z

Ok, added another commit with that.
And just noticed that when I (manually) applied your suggestion earlier, I accidentally left the old .filter() in. 😓
I guess most chances are the compiler will be smart enough to not perform the if twice, but if not then it might skew the perf run.

yotamofek · 2025-02-10T22:42:40Z

@GuillaumeGomez rustc-perf hasn't noticed the try build finished (again),
but in the meanwhile I added a 3rd commit changing the FxIndexSet into a FxHashSet.
In local perf runs, none of the 3 commits showed any improvements. We can try to see if it makes any difference on CI, but if it stays neutral then maybe this PR can just be closed.

aDotInTheVoid · 2025-02-11T00:46:00Z

src/librustdoc/html/render/print_item.rs

-            buf,
-            r#"<script type="text/json" id="notable-traits-data">{}</script>"#,
+        write!(buf, r#"<script type="text/json" id="notable-traits-data">{}</script>"#, {
+            #[allow(rustc::potential_query_instability)]


This is technically ok, as notable_traits_json sorts the items, but just looking at this code, that's not clear why.

Given that this is the only callsite, it'd be better to pass a FxHashMap (instead of an opaque impl Iterator, so the allow can be next to a comment explaining why it's ok. That way all that reasoning about why this is correct stays local.

I agree, fixed.
(I used the #[expect] attribute because it's cool and I hadn't had a chance to use it yet, not sure if it should just be #[allow])

GuillaumeGomez · 2025-02-11T13:28:04Z

I wonder if it's because you pushed? Well, try not to push before this one runs then. ;)

@bors try @rust-timer queue

…leanup, r=<try> Optimize `notable_trait_buttons` ~Small cleanup. Use `Iterator::any` instead of `for` loop with `predicate = true;`. I think this makes the code more readable... and also has the additional benefit of short-circuiting the iterator when a notable trait is found (a `break` statement was missing in the `for` loop version, I think). Probably won't be significant enough to show on perf results, though.~ Three commits, each attempting to optimize `notable_trait_buttons` by a little bit.

bors · 2025-02-11T13:29:16Z

⌛ Trying commit afdf48d with merge 613e9cd...

bors · 2025-02-11T15:24:58Z

☀️ Try build successful - checks-actions
Build commit: 613e9cd (613e9cd968ec0f2cc5541e9d3c9eb9bfb8d739a7)

rust-timer · 2025-02-11T16:58:03Z

Finished benchmarking commit (613e9cd): comparison URL.

Overall result: no relevant changes - 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 benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary -0.9%, secondary -0.3%)

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)	1.0%	[1.0%, 1.0%]	1
Regressions ❌ (secondary)	6.3%	[6.3%, 6.3%]	1
Improvements ✅ (primary)	-2.8%	[-2.8%, -2.8%]	1
Improvements ✅ (secondary)	-7.0%	[-7.0%, -7.0%]	1
All ❌✅ (primary)	-0.9%	[-2.8%, 1.0%]	2

Cycles

Results (primary -14.4%)

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)	-14.4%	[-19.0%, -11.1%]	13
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-14.4%	[-19.0%, -11.1%]	13

Binary size

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

Bootstrap: 787s -> 785.515s (-0.19%)
Artifact size: 348.27 MiB -> 348.30 MiB (0.01%)

bors · 2025-02-15T15:53:14Z

☔ The latest upstream changes (presumably #136828) made this pull request unmergeable. Please resolve the merge conflicts.

yotamofek · 2025-02-15T21:19:59Z

Woops, wrong button

aDotInTheVoid · 2025-02-15T22:01:32Z

We can try to see if it makes any difference on CI, but if it stays neutral then maybe this PR can just be closed.

Do you think this is worth merging given the perf results?

yotamofek · 2025-02-15T22:08:02Z

I think 0da8b61 is beneficial even with no improvements to perf, it makes the code more readable (IMHO) and a bit more correct (because it short-circuits when it finds a notable trait impl).
The second commit just makes the code messier, and the third commit I'm ambivalent about.
So whatever you think is fine by me 😊

bors · 2025-03-02T16:40:21Z

☔ The latest upstream changes (presumably #137425) made this pull request unmergeable. Please resolve the merge conflicts.

…of for loop

yotamofek · 2025-03-15T13:56:29Z

@aDotInTheVoid rebased and dropped the last two commits, leaving just one which I think is worthwhile (for correctness' sake), even if it doesn't affect perf. LMK if you agree, otherwise I can just close this one for now.

aDotInTheVoid · 2025-03-16T21:44:51Z

Yeah, the new version is nicer to read, even if it doesn't give us any more perf.

@bors r+ rollup

bors · 2025-03-16T21:44:54Z

📌 Commit 9d9bac0 has been approved by aDotInTheVoid

It is now in the queue for this repository.

…-cleanup, r=aDotInTheVoid refactor `notable_traits_button` to use iterator combinators instead of for loop ~Small cleanup. Use `Iterator::any` instead of `for` loop with `predicate = true;`. I think this makes the code more readable... and also has the additional benefit of short-circuiting the iterator when a notable trait is found (a `break` statement was missing in the `for` loop version, I think). Probably won't be significant enough to show on perf results, though.~ Three commits, each attempting to optimize `notable_trait_buttons` by a little bit.

Rollup of 7 pull requests Successful merges: - rust-lang#136293 (document capacity for ZST as example) - rust-lang#136355 (Add `*_value` methods to proc_macro lib) - rust-lang#136359 (doc all differences of ptr:copy(_nonoverlapping) with memcpy and memmove) - rust-lang#136816 (refactor `notable_traits_button` to use iterator combinators instead of for loop) - rust-lang#138363 (Add `From<{integer}>` for `f16`/`f128` impls) - rust-lang#138552 (Misc print request handling cleanups + a centralized test for print request stability gating) - rust-lang#138573 (Make `_Unwind_Action` a type alias, not enum) r? `@ghost` `@rustbot` modify labels: rollup

Rollup of 5 pull requests Successful merges: - rust-lang#136293 (document capacity for ZST as example) - rust-lang#136359 (doc all differences of ptr:copy(_nonoverlapping) with memcpy and memmove) - rust-lang#136816 (refactor `notable_traits_button` to use iterator combinators instead of for loop) - rust-lang#138552 (Misc print request handling cleanups + a centralized test for print request stability gating) - rust-lang#138573 (Make `_Unwind_Action` a type alias, not enum) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#136816 - yotamofek:pr/notable-traits-button-cleanup, r=aDotInTheVoid refactor `notable_traits_button` to use iterator combinators instead of for loop ~Small cleanup. Use `Iterator::any` instead of `for` loop with `predicate = true;`. I think this makes the code more readable... and also has the additional benefit of short-circuiting the iterator when a notable trait is found (a `break` statement was missing in the `for` loop version, I think). Probably won't be significant enough to show on perf results, though.~ Three commits, each attempting to optimize `notable_trait_buttons` by a little bit.

Rollup of 5 pull requests Successful merges: - rust-lang#136293 (document capacity for ZST as example) - rust-lang#136359 (doc all differences of ptr:copy(_nonoverlapping) with memcpy and memmove) - rust-lang#136816 (refactor `notable_traits_button` to use iterator combinators instead of for loop) - rust-lang#138552 (Misc print request handling cleanups + a centralized test for print request stability gating) - rust-lang#138573 (Make `_Unwind_Action` a type alias, not enum) r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned GuillaumeGomez Feb 10, 2025

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

GuillaumeGomez reviewed Feb 10, 2025

View reviewed changes

src/librustdoc/html/render/mod.rs Outdated Show resolved Hide resolved

yotamofek force-pushed the pr/notable-traits-button-cleanup branch from 2f0f308 to fee4729 Compare February 10, 2025 19:49

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 10, 2025

yotamofek force-pushed the pr/notable-traits-button-cleanup branch from 1c8bb6e to 6fea3a4 Compare February 10, 2025 22:37

yotamofek changed the title ~~Refactor notable_trait_buttons to use iterator combinators instead of for loop~~ Optimize notable_trait_buttons Feb 10, 2025

aDotInTheVoid reviewed Feb 11, 2025

View reviewed changes

yotamofek force-pushed the pr/notable-traits-button-cleanup branch from 6fea3a4 to afdf48d Compare February 11, 2025 12:38

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 11, 2025

yotamofek force-pushed the pr/notable-traits-button-cleanup branch from 5ab9abf to 9dd6561 Compare February 15, 2025 15:59

rustbot added the T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output. label Feb 15, 2025

yotamofek closed this Feb 15, 2025

yotamofek deleted the pr/notable-traits-button-cleanup branch February 15, 2025 21:19

yotamofek restored the pr/notable-traits-button-cleanup branch February 15, 2025 21:19

yotamofek reopened this Feb 15, 2025

refactor notable_traits_button to use iterator combinators instead …

9d9bac0

…of for loop

yotamofek force-pushed the pr/notable-traits-button-cleanup branch from 9dd6561 to 9d9bac0 Compare March 15, 2025 13:54

aDotInTheVoid changed the title ~~Optimize notable_trait_buttons~~ refactor notable_traits_button to use iterator combinators instead of for loop Mar 16, 2025

aDotInTheVoid self-assigned this Mar 16, 2025

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 Mar 16, 2025

jhpratt mentioned this pull request Mar 16, 2025

Rollup of 7 pull requests #138578

Closed

jhpratt mentioned this pull request Mar 17, 2025

Rollup of 5 pull requests #138583

Merged

bors merged commit 04032ab into rust-lang:master Mar 17, 2025
6 checks passed

rustbot added this to the 1.87.0 milestone Mar 17, 2025

yotamofek deleted the pr/notable-traits-button-cleanup branch March 17, 2025 17:48

refactor notable_traits_button to use iterator combinators instead of for loop #136816

refactor notable_traits_button to use iterator combinators instead of for loop #136816

Uh oh!

Conversation

yotamofek commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Feb 10, 2025

Uh oh!

yotamofek commented Feb 10, 2025

Uh oh!

Uh oh!

GuillaumeGomez commented Feb 10, 2025

Uh oh!

yotamofek commented Feb 10, 2025

Uh oh!

GuillaumeGomez commented Feb 10, 2025

Uh oh!

This comment has been minimized.

bors commented Feb 10, 2025

Uh oh!

yotamofek commented Feb 10, 2025

Uh oh!

GuillaumeGomez commented Feb 10, 2025

Uh oh!

yotamofek commented Feb 10, 2025

Uh oh!

yotamofek commented Feb 10, 2025

Uh oh!

aDotInTheVoid Feb 11, 2025

Choose a reason for hiding this comment

Uh oh!

yotamofek Feb 11, 2025

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez commented Feb 11, 2025

Uh oh!

This comment has been minimized.

bors commented Feb 11, 2025

Uh oh!

bors commented Feb 11, 2025

Uh oh!

This comment has been minimized.

rust-timer commented Feb 11, 2025

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Uh oh!

bors commented Feb 15, 2025

Uh oh!

yotamofek commented Feb 15, 2025

Uh oh!

aDotInTheVoid commented Feb 15, 2025

Uh oh!

yotamofek commented Feb 15, 2025

Uh oh!

bors commented Mar 2, 2025

Uh oh!

yotamofek commented Mar 15, 2025

Uh oh!

aDotInTheVoid commented Mar 16, 2025

Uh oh!

bors commented Mar 16, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

refactor `notable_traits_button` to use iterator combinators instead of for loop #136816

refactor `notable_traits_button` to use iterator combinators instead of for loop #136816

yotamofek commented Feb 10, 2025 •

edited

Loading