Skip to content

Avoid temporary allocations in render_assoc_item #82855

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Merged
merged 1 commit into from
Mar 22, 2021
Merged

Avoid temporary allocations in render_assoc_item #82855

merged 1 commit into from
Mar 22, 2021

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Mar 7, 2021

render_assoc_item came up as very hot in a profile of rustdoc on
bevy. This avoids some temporary allocations just to calculate the
length of the header.

This should be a strict improvement, since all string formatting was
done twice before.

cc #82845

@jyn514 jyn514 added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. I-compiletime Issue: Problems and improvements with respect to compile times. labels Mar 7, 2021
@rust-highfive
Copy link
Contributor

r? @GuillaumeGomez

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 7, 2021
@jyn514
Copy link
Member Author

jyn514 commented Mar 7, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 7, 2021
@jyn514 jyn514 changed the title Avoid temporary allocations in render_assoc_item [WIP] Avoid temporary allocations in render_assoc_item Mar 7, 2021
@jyn514 jyn514 added S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 7, 2021
@bors
Copy link
Collaborator

bors commented Mar 7, 2021

⌛ Trying commit 7b886069c3b698f0ea8e4ced5589b1945fcaf71f with merge 30f3371c2e5ed13ec440a7e3fafea51639bf21f7...

@rust-log-analyzer

This comment has been minimized.

# to view
@bors
Copy link
Collaborator

bors commented Mar 7, 2021

☀️ Try build successful - checks-actions
Build commit: 30f3371c2e5ed13ec440a7e3fafea51639bf21f7 (30f3371c2e5ed13ec440a7e3fafea51639bf21f7)

@rust-timer
Copy link
Collaborator

Queued 30f3371c2e5ed13ec440a7e3fafea51639bf21f7 with parent dfe519b, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (30f3371c2e5ed13ec440a7e3fafea51639bf21f7): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Mar 7, 2021
@GuillaumeGomez
Copy link
Member

There is at most -0.1% instructions. In any case, good improvement, thanks!

@bors: r+

@bors
Copy link
Collaborator

bors commented Mar 7, 2021

📋 Looks like this PR is still in progress, ignoring approval.

Hint: Remove [WIP] from this PR's title when it is ready for review.

@GuillaumeGomez
Copy link
Member

And I just saw that the CI wasn't happy and that the PR was still WIP...

@bors: r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 7, 2021
@jyn514
Avoid temporary allocations in render_assoc_item
1e6d849 
`render_assoc_item` came up as very hot in a profile of rustdoc on
`bevy`.  This avoids some temporary allocations just to calculate the
length of the header.

This should be a strict improvement, since all string formatting was
done twice before.
@jyn514 jyn514 changed the title [WIP] Avoid temporary allocations in render_assoc_item Avoid temporary allocations in render_assoc_item Mar 7, 2021
@jyn514 jyn514 removed the S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. label Mar 7, 2021
@jyn514 jyn514 mentioned this pull request Mar 7, 2021
Merged
@jyn514
Copy link
Member Author

jyn514 commented Mar 22, 2021

@bors r=GuillaumeGomez

@bors
Copy link
Collaborator

bors commented Mar 22, 2021

📌 Commit 1e6d849 has been approved by GuillaumeGomez

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 22, 2021
@bors
Copy link
Collaborator

bors commented Mar 22, 2021

⌛ Testing commit 1e6d849 with merge 2b8fbe6...

@bors
Copy link
Collaborator

bors commented Mar 22, 2021

☀️ Test successful - checks-actions
Approved by: GuillaumeGomez
Pushing 2b8fbe6 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 22, 2021
@bors bors merged commit 2b8fbe6 into rust-lang:master Mar 22, 2021
@rustbot rustbot added this to the 1.53.0 milestone Mar 22, 2021
This was referenced Mar 22, 2021
Closed
Merged
@jyn514 jyn514 deleted the no-temporaries branch March 22, 2021 13:17
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@GuillaumeGomez GuillaumeGomez GuillaumeGomez approved these changes

Assignees

@GuillaumeGomez GuillaumeGomez

Labels
I-compiletime Issue: Problems and improvements with respect to compile times. 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.53.0
Development

Successfully merging this pull request may close these issues.
7 participants
@jyn514 @rust-highfive @rust-timer @bors @rust-log-analyzer @GuillaumeGomez @rustbot