rustdoc: cleanups relating to allocations #141573
Conversation
This reverts part of rust-lang#91948, going back to returning a `UrlPartsBuilder`. It makes the code simpler, and also avoids some allocations.
Currently it is passed an `fqp` slice which it calls `to_vec` on and returns. This is a bit odd. It's better to let the call site clone if necessary. (One call site does, one does not).
To avoids the early return and duplication of `Ok((url_parts, shortty, fqp))`.
Most of the methods returning `impl Display` have `print` in their name. This commit renames a few that didn't follow that convention.
It never fails, so it doesn't need to return `Result`. And the `ItemType` in the result is just a copy of the one passed in via the `shortty` arg, so it can also be removed.
Instead of a `Vec`, to avoid some allocations.
rustbot
added
S-waiting-on-review
|
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
rustdoc: cleanups relating to allocations These commits generally clean up the code a bit and also reduce allocation rates a bit. r? `@camelid`
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (9656ba7): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking 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 may lead to changes in compiler perf. @bors rollup=never Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary -1.2%, secondary -2.7%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (primary -2.8%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeResults (primary -1.1%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 776.359s -> 777.253s (0.12%) |
rustbot
removed
the
S-waiting-on-perf
src/librustdoc/html/format.rs
Outdated
| @@ -1419,7 +1415,7 @@ pub(crate) fn visibility_print_with_space(item: &clean::Item, cx: &Context<'_>) | |||
| debug!("path={path:?}"); | |||
| // modified from `resolved_path()` to work with `DefPathData` | |||
| let last_name = path.data.last().unwrap().data.get_opt_name().unwrap(); | |||
| let anchor = anchor(vis_did, last_name, cx); | |||
| let anchor = print_anchor(vis_did, last_name, cx); | |||
There was a problem hiding this comment.
Do you think it would be worth it to use fmt::from_fn in visibility_print_with_space to eliminate another allocation? This function does get called fairly often.
There was a problem hiding this comment.
It does use fmt::from_fn, towards the bottom.
There was a problem hiding this comment.
yes, but do you think it would be worth it to move the main function body into the fmt::from_fn call? it should save at least an allocation, no?
There was a problem hiding this comment.
Not worth it from a perf perspective: this function doesn't show up as hot in profiles (I've been using profiles to guide all of this work) and I think it will only allocate in the else branch containing s.
But: worth it from a code cleanliness perspective, because it makes the code shorter and nicer. I've added a new commit doing it, thanks for the suggestion.
| let trait_ = impl_ | ||
| .inner_impl() | ||
| .trait_ | ||
| .as_ref() | ||
| .map(|trait_| format!("{:#}", trait_.print(cx))); | ||
| ret = Some(AliasSerializableImpl { |
There was a problem hiding this comment.
We should probably document what the alternate display flag does (disables html escaping of < and >) and why we are using it here.
There was a problem hiding this comment.
I have added a comment explaining the "what", but not the "why" (because I don't know why).
There was a problem hiding this comment.
disables html escaping of < and >
That's not really the core of it. The alternate format prints things (in this case, the trait's path) as plain text rather than as rich HTML. So as part of that, it doesn't use HTML escaping. But the main thing is that here we're generating plaintext rather than HTML.
I'm not exactly sure of the reason, though likely we're doing some kind of matching against the path in JS to know which aliased impl to load. Anyway, it's beside the point of this PR.
| @@ -682,7 +680,7 @@ impl TypeAliasPart { | |||
| )); | |||
|
|
|||
| let part = OrderedJson::array_sorted( | |||
| impls.iter().map(OrderedJson::serialize).collect::<Result<Vec<_>, _>>().unwrap(), | |||
| impls.map(OrderedJson::serialize).collect::<Result<Vec<_>, _>>().unwrap(), | |||
There was a problem hiding this comment.
Just FYI that this collect is also redundant,
this will work too:
| impls.map(OrderedJson::serialize).collect::<Result<Vec<_>, _>>().unwrap(), | |
| impls.map(|impl_| OrderedJson::serialize(impl_).unwrap()), |
(though doesn't seem to affect perf on my local tests)
There was a problem hiding this comment.
Nice suggestion! I have applied it.
- `ret` only ever gets at most one entry, so it can be an `Option` instead of a `Vec`. - Which means we can use `filter_map` instead of `flat_map`. - Move `trait_` next to the `ret` assignment, which can only happen once. - No need for `impls` to be a `Vec`, it can remain an iterator. - Avoid `Result` when collecting `impls`.
nnethercote
force-pushed
the
rustdoc-alloc-cleanups
branch
from
c51f167 to
8dde6d5
Compare
Moving the visibility stuff into the `from_fn` avoids the `Cow` and makes the code a little shorter and simpler.
|
BTW, this is a case where selecting "Show non-relevant results" in the perf results is worthwhile. There are a number of sub-threshold improvements that I think are real, because they are so consistent. |
|
Thanks! Looks good at a first glance, but will do a proper review by tomorrow or the next day. |
| let trait_ = impl_ | ||
| .inner_impl() | ||
| .trait_ | ||
| .as_ref() | ||
| .map(|trait_| format!("{:#}", trait_.print(cx))); | ||
| ret = Some(AliasSerializableImpl { |
There was a problem hiding this comment.
disables html escaping of < and >
That's not really the core of it. The alternate format prints things (in this case, the trait's path) as plain text rather than as rich HTML. So as part of that, it doesn't use HTML escaping. But the main thing is that here we're generating plaintext rather than HTML.
I'm not exactly sure of the reason, though likely we're doing some kind of matching against the path in JS to know which aliased impl to load. Anyway, it's beside the point of this PR.
camelid
removed
the
S-waiting-on-review
camelid
added
the
S-waiting-on-author
rustbot
added
S-waiting-on-review
|
@bors r+ |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 6de3a73 (parent) -> e6152cd (this PR) Test differencesShow 33327 test diffsStage 1
Stage 2
(and 16606 additional test diffs) Additionally, 16621 doctest diffs were found. These are ignored, as they are noisy. Job group index Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard e6152cdf5b31bd844a4cc1049433859d54863602 --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (e6152cd): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary -2.6%, secondary 0.1%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (secondary 0.8%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: missing data |
These commits generally clean up the code a bit and also reduce allocation rates a bit.
r? @camelid