Skip to content

Continue String to Symbol conversion in rustdoc (1) #80119

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
Merged
merged 1 commit into from
Dec 18, 2020
Merged

Continue String to Symbol conversion in rustdoc (1) #80119

merged 1 commit into from
Dec 18, 2020

Conversation

GuillaumeGomez
Copy link
Member

Follow-up of #80091.

This PR is already big enough so I'll stop here before the next one.

r? @jyn514

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

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also looks great ❤️

src/librustdoc/json/conversions.rs
@@ -163,7 +163,9 @@ impl From<clean::ItemKind> for ItemEnum {
use clean::ItemKind::*;
match item {
ModuleItem(m) => ItemEnum::ModuleItem(m.into()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ItemEnum should probably be renamed at some point, but no need to do it here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe open an issue so we don't forget? (And mark it as e-easy too ;) )

@jyn514 jyn514 added C-cleanup Category: PRs that clean code up or issues documenting cleanup. I-compilemem Issue: Problems and improvements with respect to memory usage during compilation. I-compiletime Issue: Problems and improvements with respect to compile times. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Dec 17, 2020
@jyn514 jyn514 mentioned this pull request Dec 17, 2020
Merged
@GuillaumeGomez
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@bors
Copy link
Collaborator

bors commented Dec 17, 2020

⌛ Trying commit 44e226c with merge ac57a14252d79c3da055dc82c0c5e253444fbc26...

@bors
Copy link
Collaborator

bors commented Dec 17, 2020

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

@rust-timer
Copy link
Collaborator

Queued ac57a14252d79c3da055dc82c0c5e253444fbc26 with parent caeb333, 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 17, 2020
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (ac57a14252d79c3da055dc82c0c5e253444fbc26): 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 17, 2020
@jyn514
Copy link
Member

jyn514 commented Dec 18, 2020

-2.2% on instruction count, -1.5% on max-rss 🎉 🎉

@bors r+ rollup=never

@bors
Copy link
Collaborator

bors commented Dec 18, 2020

📌 Commit 44e226c has been approved by jyn514

@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 18, 2020
@jyn514 jyn514 mentioned this pull request Dec 18, 2020
Closed
@bors
Copy link
Collaborator

bors commented Dec 18, 2020

⌛ Testing commit 44e226c with merge fee693d...

@bors
Copy link
Collaborator

bors commented Dec 18, 2020

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing fee693d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 18, 2020
@bors bors merged commit fee693d into rust-lang:master Dec 18, 2020
@rustbot rustbot added this to the 1.50.0 milestone Dec 18, 2020
@rust-highfive
Copy link
Contributor

📣 Toolstate changed by #80119!

Tested on commit fee693d.
Direct link to PR: #80119

💔 rls on linux: test-pass → test-fail (cc @Xanewok).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Dec 18, 2020
@rust-highfive
📣 Toolstate changed by rust-lang/rust#80119!
5ccb3bb 
Tested on commit rust-lang/rust@fee693d.
Direct link to PR: <rust-lang/rust#80119>

💔 rls on linux: test-pass → test-fail (cc @Xanewok).
@bors bors mentioned this pull request Dec 18, 2020
Merged
@GuillaumeGomez GuillaumeGomez deleted the str-to-symbol branch December 18, 2020 09:03
@bors bors mentioned this pull request Dec 18, 2020
Merged
@GuillaumeGomez GuillaumeGomez mentioned this pull request Dec 18, 2020
Merged
@jyn514 jyn514 changed the title Continue String to Symbol conversion in rustdoc Continue String to Symbol conversion in rustdoc (1) Dec 18, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 18, 2020
@bors
Auto merge of rust-lang#80154 - GuillaumeGomez:str-to-symbol, r=jyn514
50a9097 
Continue String to Symbol conversion in rustdoc (2)

Follow-up of rust-lang#80119.

This is the last one (and I actually expected more conversions but seems like it was the last one remaining...).

r? `@jyn514`
@GuillaumeGomez GuillaumeGomez mentioned this pull request Dec 21, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 3, 2021
@bors
Auto merge of rust-lang#80261 - GuillaumeGomez:attr-rework, r=jyn514
e821a6e 
rustdoc DocFragment rework

Kind of a follow-up of rust-lang#80119.

A few things are happening in this PR. I'm not sure about the impact on perf though so I'm very interested about that too (if the perf is worse, then we can just close this PR).

The idea here is mostly about reducing the memory usage by relying even more on `Symbol` instead of `String`. The only issue is that `DocFragment` has 3 modifications performed on it:
 1. Unindenting
 2. Collapsing similar comments into one
 3. "Beautifying" (weird JS-like comments handling).

To do so, I saved the information about unindent and the "collapse" is now on-demand (which is why I'm not sure the perf will be better, it has to be run multiple times...).

r? `@jyn514`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyn514 jyn514 jyn514 left review comments

Assignees

@jyn514 jyn514

Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. 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-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.50.0
Development

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