rustdoc: avoid many `Symbol` to `String` conversions. #91948

nnethercote · 2021-12-15T01:06:15Z

Particularly when constructing file paths and fully qualified paths.
This avoids a lot of allocations, speeding things up on almost all
examples.

r? @GuillaumeGomez

nnethercote · 2021-12-15T01:07:10Z

In local measurements I see the following rustdoc improvements on rustc-perf:

instructions: up to 5% better
cycles: up to 8% better
wall-time: up to 10% better

@bors try @rust-timer queue

rust-timer · 2021-12-15T01:07:12Z

Awaiting bors try build completion.

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

bors · 2021-12-15T01:07:19Z

⌛ Trying commit ac09b579d88c408116eb91a197850a86e6fb7aed with merge c35f8646bbfb5c225d701c6fd49948b853c27202...

camelid · 2021-12-15T01:55:05Z

I guess we had the same idea at the same time: #91876 :P

I'll close that one since the tests are failing and this covers it.

src/librustdoc/clean/inline.rs

src/librustdoc/html/format.rs

src/librustdoc/html/render/print_item.rs

src/librustdoc/html/tests.rs

src/librustdoc/visit_ast.rs

bors · 2021-12-15T02:42:34Z

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

rust-timer · 2021-12-15T02:42:35Z

Queued c35f8646bbfb5c225d701c6fd49948b853c27202 with parent 2f4da62, future comparison URL.

rust-timer · 2021-12-15T04:13:47Z

Finished benchmarking commit (c35f8646bbfb5c225d701c6fd49948b853c27202): comparison url.

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

Large improvement in instruction counts (up to -3.7% on full builds of futures)

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

nnethercote · 2021-12-15T04:59:39Z

Instruction counts and cycles show clear wins, smaller than I got locally, probably because I was profiling without jemalloc enabled. Wall times show no improvement in the default view, but if you show "insignificant" changes you can see up to 7%; I guess the wall times are so noisy in general that any non-huge change is considered insignificant.

nnethercote · 2021-12-15T05:01:50Z

I have pushed an update that addresses most of the comments.

GuillaumeGomez · 2021-12-15T09:17:27Z

This is super cool, thanks! Let's wait for @camelid's confirmation as well.

camelid · 2021-12-15T20:28:07Z

I'll review this soon.

camelid

Overall, this looks good, but I have some code quality concerns.

src/librustdoc/clean/inline.rs

src/librustdoc/html/format.rs

src/librustdoc/visit_ast.rs

camelid · 2022-01-13T19:55:32Z

I just realized that I forgot to delete an outdated comment from an earlier commit (the one about 64 bytes is covers 99.9%+ of cases). Done.

@bors r=GuillaumeGomez rollup=never

bors · 2022-01-13T19:55:33Z

📌 Commit 4848d5a059a242e9d691f94b070863d76a520a24 has been approved by GuillaumeGomez

bors · 2022-01-14T06:29:46Z

☔ The latest upstream changes (presumably #92844) made this pull request unmergeable. Please resolve the merge conflicts.

camelid · 2022-01-14T19:57:00Z

@bors p=1 (this is really conflict-prone)

Particularly when constructing file paths and fully qualified paths. This avoids a lot of allocations, speeding things up on almost all examples.

I seem to recall that in general, it's best to request an allocation with a size that's a power of 2. The low estimate of 5 was probably a little too low as well.

camelid · 2022-01-14T20:07:49Z

@bors r=GuillaumeGomez rollup=never

bors · 2022-01-14T20:07:51Z

📌 Commit c7147e4 has been approved by GuillaumeGomez

bors · 2022-01-14T20:34:23Z

⌛ Testing commit c7147e4 with merge b0ec3e0...

bors · 2022-01-15T00:31:15Z

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

rust-timer · 2022-01-15T02:02:22Z

Finished benchmarking commit (b0ec3e0): comparison url.

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

Very large improvement in instruction counts (up to -5.5% on full builds of issue-46449)

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

@rustbot label: -perf-regression

rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Dec 15, 2021

rust-highfive assigned GuillaumeGomez Dec 15, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 15, 2021

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 15, 2021

camelid mentioned this pull request Dec 15, 2021

rustdoc: Replace String with Symbol in Cache.paths et al. #91876

Closed

camelid reviewed Dec 15, 2021

View reviewed changes

camelid self-assigned this Dec 15, 2021

camelid 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 Dec 15, 2021

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 Dec 15, 2021

nnethercote mentioned this pull request Dec 15, 2021

rustdoc: Add UrlPartsBuilder #91871

Merged

GuillaumeGomez approved these changes Dec 15, 2021

View reviewed changes

camelid requested changes Dec 17, 2021

View reviewed changes

camelid removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 17, 2021

camelid mentioned this pull request Dec 17, 2021

hir: Do not introduce dummy type names for extern blocks in def paths #92032

Merged

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jan 13, 2022

camelid force-pushed the rustdoc-more-Symbols branch from 970b911 to 4848d5a Compare January 13, 2022 19:54

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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 14, 2022

nnethercote and others added 7 commits January 14, 2022 11:57

rustdoc: avoid many Symbol to String conversions.

10b1c9a

Particularly when constructing file paths and fully qualified paths. This avoids a lot of allocations, speeding things up on almost all examples.

rustdoc: remove many unnecessary .as_ref() calls.

1e4637c

rustdoc: remove some unnecessary sigils.

6b19cf9

Use UrlPartsBuilder and remove join_with_slash

53f1bed

Estimate path length instead of hardcoding 64 bytes

8f59eb6

Make AVG_PART_LENGTH a power of 2

cef250d

I seem to recall that in general, it's best to request an allocation with a size that's a power of 2. The low estimate of 5 was probably a little too low as well.

Document and test UrlPartsBuilder::push_fmt

c7147e4

camelid force-pushed the rustdoc-more-Symbols branch from 4848d5a to c7147e4 Compare January 14, 2022 20:06

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 Jan 14, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 15, 2022

bors merged commit b0ec3e0 into rust-lang:master Jan 15, 2022

rustbot added this to the 1.60.0 milestone Jan 15, 2022

nnethercote deleted the rustdoc-more-Symbols branch January 15, 2022 00:41

nnethercote restored the rustdoc-more-Symbols branch January 15, 2022 00:41

bors mentioned this pull request Jan 15, 2022

Implement namespacing for doc comments IDs #92043

Closed

nnethercote deleted the rustdoc-more-Symbols branch February 25, 2022 02:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustdoc: avoid many `Symbol` to `String` conversions. #91948

rustdoc: avoid many `Symbol` to `String` conversions. #91948

nnethercote commented Dec 15, 2021 •

edited by camelid

Loading

nnethercote commented Dec 15, 2021

rust-timer commented Dec 15, 2021

bors commented Dec 15, 2021

camelid commented Dec 15, 2021

bors commented Dec 15, 2021

rust-timer commented Dec 15, 2021

rust-timer commented Dec 15, 2021

nnethercote commented Dec 15, 2021

nnethercote commented Dec 15, 2021

GuillaumeGomez commented Dec 15, 2021

camelid commented Dec 15, 2021

camelid left a comment

camelid commented Jan 13, 2022

bors commented Jan 13, 2022

bors commented Jan 14, 2022

camelid commented Jan 14, 2022

camelid commented Jan 14, 2022

bors commented Jan 14, 2022

bors commented Jan 14, 2022

bors commented Jan 15, 2022

rust-timer commented Jan 15, 2022

rustdoc: avoid many Symbol to String conversions. #91948

rustdoc: avoid many Symbol to String conversions. #91948

Conversation

nnethercote commented Dec 15, 2021 • edited by camelid Loading

nnethercote commented Dec 15, 2021

rust-timer commented Dec 15, 2021

bors commented Dec 15, 2021

camelid commented Dec 15, 2021

bors commented Dec 15, 2021

rust-timer commented Dec 15, 2021

rust-timer commented Dec 15, 2021

nnethercote commented Dec 15, 2021

nnethercote commented Dec 15, 2021

GuillaumeGomez commented Dec 15, 2021

camelid commented Dec 15, 2021

camelid left a comment

Choose a reason for hiding this comment

camelid commented Jan 13, 2022

bors commented Jan 13, 2022

bors commented Jan 14, 2022

camelid commented Jan 14, 2022

camelid commented Jan 14, 2022

bors commented Jan 14, 2022

bors commented Jan 14, 2022

bors commented Jan 15, 2022

rust-timer commented Jan 15, 2022

rustdoc: avoid many `Symbol` to `String` conversions. #91948

rustdoc: avoid many `Symbol` to `String` conversions. #91948

nnethercote commented Dec 15, 2021 •

edited by camelid

Loading