Skip to content
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.

Sign up for GitHub

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

Jump to bottom

rustdoc: reduce allocs in FnDecl::inner_full_print #109011

Merged
merged 1 commit into from
Mar 13, 2023
Merged

rustdoc: reduce allocs in FnDecl::inner_full_print #109011

merged 1 commit into from
Mar 13, 2023

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Mar 11, 2023

Instead of maintaining parallel buffers for both HTML and non-HTML output, follow the idiom from the rest of format.rs that f.alternate() == true means textual output. Also, add an argument to control line wrapping explicitly.

This allows the caller to render once with textual output and no line wrapping, to decide whether line wrapping should be applied in the final HTML output.

Also, remove some format! and " ".repeat calls, and remove a dependency on calling String::replace to switch from newlines to spaces.

This coincidentally fixes some minor bugs where the old code was undercounting the number of characters for a declaration in text mode.

@rustbot
Copy link
Collaborator

rustbot commented Mar 11, 2023

r? @notriddle

(rustbot has picked a reviewer for you, use r? to override)

@rustbot 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 Mar 11, 2023
@jsha jsha force-pushed the reduce-allocations-inner-full-print branch from 0a9069e to a5e2a28 Compare March 11, 2023 18:31
notriddle
notriddle reviewed Mar 11, 2023
View reviewed changes
src/librustdoc/html/format.rs Outdated Show resolved Hide resolved
src/librustdoc/html/format.rs Outdated Show resolved Hide resolved
src/librustdoc/html/format.rs Outdated Show resolved Hide resolved
@jsha
rustdoc: reduce allocs in FnDecl::inner_full_print
121ae1d 
Instead of maintaining parallel buffers for both HTML and non-HTML output,
follow the idiom from the rest of format.rs that f.alternate() == true means
textual output. Also, add an argument to control line wrapping explicitly.

This allows the caller to render once with textual output and no line wrapping,
to decide whether line wrapping should be applied in the final HTML output.

Also, remove some format! and " ".repeat calls, and remove a dependency on
calling `String::replace` to switch from newlines to spaces.

This coincidentally fixes some minor bugs where the old code was undercounting
the number of characters for a declaration in text mode.
@jsha jsha force-pushed the reduce-allocations-inner-full-print branch from a5e2a28 to 121ae1d Compare March 11, 2023 23:37
@notriddle
Copy link
Contributor

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 12, 2023
@bors
Copy link
Contributor

bors commented Mar 12, 2023

⌛ Trying commit 121ae1d with merge 42cd0bed30baeb285c5e39fa396f9ff30d7bb8b6...

@bors
Copy link
Contributor

bors commented Mar 12, 2023

☀️ Try build successful - checks-actions
Build commit: 42cd0bed30baeb285c5e39fa396f9ff30d7bb8b6 (42cd0bed30baeb285c5e39fa396f9ff30d7bb8b6)

1 similar comment
@bors
Copy link
Contributor

bors commented Mar 12, 2023

☀️ Try build successful - checks-actions
Build commit: 42cd0bed30baeb285c5e39fa396f9ff30d7bb8b6 (42cd0bed30baeb285c5e39fa396f9ff30d7bb8b6)

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (42cd0bed30baeb285c5e39fa396f9ff30d7bb8b6): 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.5% [-0.8%, -0.3%] 4
Improvements ✅
(secondary)		 -2.2% [-2.2%, -2.2%] 1
All ❌✅ (primary) -0.5% [-0.8%, -0.3%] 4

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.9% [1.9%, 1.9%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -2.5% [-2.5%, -2.5%] 1
All ❌✅ (primary) - - 0

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)		 -2.6% [-2.6%, -2.6%] 1
All ❌✅ (primary) - - 0

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 12, 2023
@notriddle
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Mar 12, 2023

📌 Commit 121ae1d has been approved by notriddle

It is now in the queue for this repository.

@bors 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 12, 2023
@bors
Copy link
Contributor

bors commented Mar 13, 2023

⌛ Testing commit 121ae1d with merge d610b0c...

@bors
Copy link
Contributor

bors commented Mar 13, 2023

☀️ Test successful - checks-actions
Approved by: notriddle
Pushing d610b0c to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 13, 2023
@bors bors merged commit d610b0c into rust-lang:master Mar 13, 2023
@rustbot rustbot added this to the 1.70.0 milestone Mar 13, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (d610b0c): comparison URL.

Overall result: ✅ 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
Improvements ✅
(primary)		 -0.7% [-1.0%, -0.4%] 7
Improvements ✅
(secondary)		 -0.7% [-2.3%, -0.3%] 8
All ❌✅ (primary) -0.7% [-1.0%, -0.4%] 7

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)		 2.4% [2.4%, 2.4%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

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)		 -2.6% [-2.6%, -2.6%] 1
All ❌✅ (primary) - - 0

@fmease fmease mentioned this pull request Jun 24, 2023
Open
@fmease fmease mentioned this pull request Jun 24, 2023
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 25, 2023
@matthiaskrgr
Rollup merge of rust-lang#113013 - fmease:rustdoc-decl-line-wrapping-…
32995d8 
…slim-arg-list, r=camelid

rustdoc: get rid of extra line when line-wrapping fn decls with empty arg list

Fixes bevyengine/bevy#8898 (comment):

![Screenshot 2023-06-24 at 23-42-53 any_with_component in bevy_ecs schedule common_conditions - Rust](https://github.com/rust-lang/rust/assets/14913065/4646eba6-b186-4d78-96d9-aad716a4ef5d)

It now prints as shown below (which conforms to the style guide):

```rs
pub fn any_with_component<T: Component>(
) -> impl FnMut(Query<'_, '_, (), With<T>>) -> bool + Clone
```

The bug was introduced in rust-lang#109011.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@notriddle notriddle notriddle left review comments

Assignees

@notriddle notriddle

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.70.0
Development

Successfully merging this pull request may close these issues.
5 participants
@jsha @rustbot @notriddle @rust-timer @bors