-
Notifications
You must be signed in to change notification settings - Fork 13.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
librustdoc: create a helper for separating elements of an iterator instead of implementing it multiple times #136244
Conversation
rustbot has assigned @GuillaumeGomez. Use |
Seems very promising, thanks! Please tell me when you want to start the perf run. |
Thanks! I'll ping you once CI checks are done (or can it be queued before they're finished? if so - go ahead please) |
Unfortunately it's not queue, it's done on a separate server. So waiting for your ping then! |
@GuillaumeGomez ping 😁 |
Let's go! @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
[WIP: perf] attempt to prevent regressions in that originally happened in rust-lang#135494 This implements something similar to [`Itertools::format`](https://docs.rs/itertools/latest/itertools/trait.Itertools.html#method.format), but on `Fn`s returning iterators instead of directly on iterators, to allow implementing `Display` without the use of a `Cell` (to handle the possibility of `fmt` being called multiple times while receiving `&self`). This is WIP, I just want to get a perf run first to see if the regression I saw in rust-lang#135494 is fixed
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (f5cefbb): comparison URL. Overall result: ❌ regressions - please read the text belowBenchmarking 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. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (secondary -1.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.
CyclesResults (secondary -2.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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 775.532s -> 776.62s (0.14%) |
Still regresses 😓 |
Please ping me whenever you want to give it another try. 😉 |
Ping 🙏 |
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
[WIP: perf] attempt to prevent regressions in that originally happened in rust-lang#135494 This implements something similar to [`Itertools::format`](https://docs.rs/itertools/latest/itertools/trait.Itertools.html#method.format), but on `Fn`s returning iterators instead of directly on iterators, to allow implementing `Display` without the use of a `Cell` (to handle the possibility of `fmt` being called multiple times while receiving `&self`). This is WIP, I just want to get a perf run first to see if the regression I saw in rust-lang#135494 is fixed
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (e38cd65): comparison URL. Overall result: ❌ regressions - please read the text belowBenchmarking 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. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary 2.8%, secondary -1.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.
CyclesResults (secondary 9.2%)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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 776.896s -> 776.333s (-0.07%) |
Thanks! @bors r+ rollup=iffy |
☔ The latest upstream changes (presumably #136507) made this pull request unmergeable. Please resolve the merge conflicts. |
…stead of implementing it multiple times
eb2635e
to
cb028dc
Compare
@rustbot review |
This comment was marked as resolved.
This comment was marked as resolved.
Lol, oops. @bors r=GuillaumeGomez |
☀️ Test successful - checks-actions |
Finished benchmarking commit (8df89d1): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary 0.2%)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.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 777.31s -> 776.632s (-0.09%) |
…<try> librustdoc: more usages of `Joined::joined` Some missed opportunities from rust-lang#136244 r? `@GuillaumeGomez` since you reviewed the last one (feel free to re-assign, of course 😊) First two commits are just drive-by cleanups
…r=GuillaumeGomez librustdoc: more usages of `Joined::joined` Some missed opportunities from rust-lang#136244 r? `@GuillaumeGomez` since you reviewed the last one (feel free to re-assign, of course 😊) First two commits are just drive-by cleanups
…r=GuillaumeGomez librustdoc: more usages of `Joined::joined` Some missed opportunities from rust-lang#136244 r? ``@GuillaumeGomez`` since you reviewed the last one (feel free to re-assign, of course 😊) First two commits are just drive-by cleanups
…r=GuillaumeGomez librustdoc: more usages of `Joined::joined` Some missed opportunities from rust-lang#136244 r? ```@GuillaumeGomez``` since you reviewed the last one (feel free to re-assign, of course 😊) First two commits are just drive-by cleanups
Rollup merge of rust-lang#136599 - yotamofek:pr/rustdoc-more-joined, r=GuillaumeGomez librustdoc: more usages of `Joined::joined` Some missed opportunities from rust-lang#136244 r? ```@GuillaumeGomez``` since you reviewed the last one (feel free to re-assign, of course 😊) First two commits are just drive-by cleanups
This implements something similar to
Itertools::format
, but onFn
s returning iterators instead of directly on iterators, to allow implementingDisplay
without the use of aCell
(to handle the possibility offmt
being called multiple times while receiving&self
).This is WIP, I just want to get a perf run first to see if the regression I saw in #135494 is fixedThis was originally part of #135494 , but originally caused a perf regression that was since fixed:
rust/src/librustdoc/html/format.rs
Line 507 in 7d5ae18