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: Add UrlPartsBuilder #91871

Merged
merged 1 commit into from
Dec 19, 2021
Merged

rustdoc: Add UrlPartsBuilder #91871

merged 1 commit into from
Dec 19, 2021

Conversation

camelid
Copy link
Member

@camelid camelid commented Dec 13, 2021

This is a type for efficiently and easily constructing the part of a URL
after the domain: nightly/core/str/struct.Bytes.html.

It allows simplifying some code and avoiding some allocations in the
href_* functions.

It will also allow making Cache.paths et al. use Symbol without
having to allocate Strings in the href_* functions. Strings would
be necessary otherwise because Symbol::as_str() returns SymbolStr,
whose Deref<Target = str> impl requires the str to not outlive it.
This is the primary motivation for the addition of UrlPartsBuilder.

@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Dec 13, 2021
@rust-highfive
Copy link
Collaborator

r? @ollie27

(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 Dec 13, 2021
@camelid
Copy link
Member Author

camelid commented Dec 13, 2021

r? @GuillaumeGomez

@camelid camelid 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. labels Dec 13, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@camelid
Copy link
Member Author

camelid commented Dec 13, 2021

Tests pass locally now.

@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 Dec 13, 2021
@bors
Copy link
Contributor

bors commented Dec 13, 2021

⌛ Trying commit 273a6269426722889715a79e5908570ca6993e34 with merge 24bb1f2a257c0c98879ec6d209134da5dffca55a...

camelid
camelid commented Dec 13, 2021
View reviewed changes
src/librustdoc/html/url_parts_builder.rs
///
/// Basic usage:
///
/// ```ignore (private-type)
Copy link
Member Author

Choose a reason for hiding this comment

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

Is there any way to run doctests for private code?

Copy link
Member

Choose a reason for hiding this comment

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

Not that I know of...

@camelid camelid mentioned this pull request Dec 13, 2021
Closed
@camelid
rustdoc: Add UrlPartsBuilder
4bac09f 
This is a type for efficiently and easily constructing the part of a URL
after the domain: `nightly/core/str/struct.Bytes.html`.

It allows simplifying some code and avoiding some allocations in the
`href_*` functions.

It will also allow making `Cache.paths` et al. use `Symbol` without
having to allocate `String`s in the `href_*` functions. `String`s would
be necessary otherwise because `Symbol::as_str()` returns `SymbolStr`,
whose `Deref<Target = str>` impl requires the `str` to not outlive it.
This is the primary motivation for the addition of `UrlPartsBuilder`.
@bors
Copy link
Contributor

bors commented Dec 13, 2021

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

@rust-timer
Copy link
Collaborator

Queued 24bb1f2a257c0c98879ec6d209134da5dffca55a with parent 1796de7, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (24bb1f2a257c0c98879ec6d209134da5dffca55a): comparison url.

Summary: This change led to moderate relevant improvements 🎉 in compiler performance.

  • Moderate improvement in instruction counts (up to -1.4% on incr-unchanged builds of wg-grammar)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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 led to changes in compiler perf.

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

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 14, 2021
@camelid
Copy link
Member Author

camelid commented Dec 14, 2021 •
edited

Instructions count improvements of up to -0.83%, including -0.74% on webrender and close to that on several other real-world crates!

max-rss is a bit mixed, but overall looks like an improvement. The regressions seem to be mainly for artificial benchmarks rather than real-world code.

@GuillaumeGomez
Copy link
Member

Looks great, thanks! Just one nit to fix and it's good to go. r=me once updated.

@camelid
Copy link
Member Author

camelid commented Dec 14, 2021

Thanks for the speedy review :)

@bors r=GuillaumeGomez rollup=never

@bors
Copy link
Contributor

bors commented Dec 14, 2021

📌 Commit 4bac09f 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-review Status: Awaiting review from the assignee but also interested parties. labels Dec 14, 2021
@camelid camelid mentioned this pull request Dec 15, 2021
Merged
@nnethercote
Copy link
Contributor

Is it still worth merging this given that #91948 doesn't need it? It's going to cause conflicts if it does merge.

@camelid
Copy link
Member Author

camelid commented Dec 15, 2021

Yes, it is. It improves code quality and performance on its own.

@bors
Copy link
Contributor

bors commented Dec 19, 2021

⌛ Testing commit 4bac09f with merge 8f54061...

@bors
Copy link
Contributor

bors commented Dec 19, 2021

☀️ Test successful - checks-actions
Approved by: GuillaumeGomez
Pushing 8f54061 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 19, 2021
@bors bors merged commit 8f54061 into rust-lang:master Dec 19, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 19, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (8f54061): comparison url.

Summary: This change led to moderate relevant mixed results 🤷 in compiler performance.

  • Moderate improvement in instruction counts (up to -3.6% on incr-patched: println builds of regression-31157)
  • Small regression in instruction counts (up to 0.4% on incr-unchanged builds of helloworld)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

@rustbot rustbot added the perf-regression Performance regression. label Dec 19, 2021
@camelid camelid deleted the urlpartsbuilder branch December 19, 2021 19:10
@camelid
Copy link
Member Author

camelid commented Dec 19, 2021

Changes reported in the summary comment are spurious rustc changes. Doc benchmarks show small improvements.

@rylev
Copy link
Member

rylev commented Dec 21, 2021

Given the comment above and the additional concern about noise as a result of the issue tracked in rust-lang/rustc-perf#1126 I'm marking as triaged, not something we should address directly.

@rustbot label +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Dec 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuillaumeGomez GuillaumeGomez GuillaumeGomez left review comments

Assignees

@GuillaumeGomez GuillaumeGomez

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. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. 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.59.0
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@camelid @rust-highfive @rust-log-analyzer @rust-timer @bors @GuillaumeGomez @nnethercote @rylev @ollie27 @rustbot