Skip to content

Rework beautify_doc_string so that it returns a Symbol instead of a String #80295

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

Merged
merged 1 commit into from
Dec 24, 2020

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Dec 22, 2020

This commit comes from #80261, the goal here is to inspect the impact on performance of this change on its own.

The idea of rewriting beautify_doc_string is to not go through String if we don't need to update the doc comment to be able to keep the original Symbol and also to have better performance.

r? @jyn514

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 22, 2020
@jyn514
Copy link
Member

jyn514 commented Dec 22, 2020

r? @petrochenkov

@rust-highfive rust-highfive assigned petrochenkov and unassigned jyn514 Dec 22, 2020
@jyn514 jyn514 added I-compilemem Issue: Problems and improvements with respect to memory usage during compilation. I-compiletime Issue: Problems and improvements with respect to compile times. labels Dec 22, 2020
@rust-log-analyzer

This comment has been minimized.

@jyn514 jyn514 added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 22, 2020
@jyn514
Copy link
Member

jyn514 commented Dec 22, 2020

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@bors
Copy link
Collaborator

bors commented Dec 22, 2020

⌛ Trying commit 64afded with merge bfedb49624a98ca1cdf9865df9704c425f540444...

@bors
Copy link
Collaborator

bors commented Dec 22, 2020

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

@rust-timer
Copy link
Collaborator

Queued bfedb49624a98ca1cdf9865df9704c425f540444 with parent 75e1acb, future comparison URL.

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

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 22, 2020
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (bfedb49624a98ca1cdf9865df9704c425f540444): 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 removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 22, 2020
@jyn514
Copy link
Member

jyn514 commented Dec 22, 2020

Perf look like noise, probably any improvements here are being lost because we immediately call to_string(). I think this would be worthwhile as part of #80261 though.

@GuillaumeGomez
Copy link
Member Author

Just like I said. As is, it's pretty useless unfortunately. So I suggest to merge this so I can integrate it into #80261.

@GuillaumeGomez
Copy link
Member Author

We need someone from the compiler team. Ping @oli-obk :)

@GuillaumeGomez
Copy link
Member Author

Oh and ping @petrochenkov too. :)

@petrochenkov
Copy link
Contributor

So, the goal is to not just return a Symbol instead of String, the goal is to return the original Symbol without going through String at all if nothing changed?
This is a good idea since this optimization should cover the common case.
I only wish this was written in the PR description, because I initially assumed that the goal is keeping a Symbol in DocFragment, but that doesn't require changing beautify_doc_string - the interning can be done on rustdoc side.

@GuillaumeGomez
Copy link
Member Author

Sorry, I opened it to make the review of #80261 simpler for @jyn514. I'll add a more extensive description.

@GuillaumeGomez
Copy link
Member Author

Done! Anything that needs to be updated here @petrochenkov (is the new description enough too?)?

@GuillaumeGomez
Copy link
Member Author

Ok, let's move on then!

@bors: r=petrochenkov

@bors
Copy link
Collaborator

bors commented Dec 24, 2020

📌 Commit 64afded has been approved by petrochenkov

@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 Dec 24, 2020
@bors
Copy link
Collaborator

bors commented Dec 24, 2020

⌛ Testing commit 64afded with merge 2acf6ee...

@bors
Copy link
Collaborator

bors commented Dec 24, 2020

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 2acf6ee to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 24, 2020
@bors bors merged commit 2acf6ee into rust-lang:master Dec 24, 2020
@rustbot rustbot added this to the 1.50.0 milestone Dec 24, 2020
@GuillaumeGomez GuillaumeGomez deleted the beautify-rework branch December 24, 2020 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-compilemem Issue: Problems and improvements with respect to memory usage during compilation. 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-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants