Specialize merge_attrs in good case #76782
Conversation
rust-highfive
added
the
S-waiting-on-review
jyn514
added
the
T-rustdoc
jyn514
added
C-cleanup
src/librustdoc/clean/inline.rs
Outdated
| // NOTE: If we have additional attributes (from a re-export), | ||
| // always insert them first. This ensure that re-export | ||
| // doc comments show up before the original doc comments | ||
| // when we render them. | ||
| if let Some(a) = other_attrs { | ||
| merged_attrs.extend(a.iter().cloned()); | ||
| if let Some(inner) = other_attrs { | ||
| merged_attrs = inner.to_vec(); | ||
| } | ||
| merged_attrs.extend(attrs.to_vec()); | ||
| merged_attrs.extend_from_slice(attrs); | ||
| merged_attrs.clean(cx) |
There was a problem hiding this comment.
Ok, what about this idea, which preserve the order and never performs an unnecessary copy:
let merged_attrs = if let Some(inner) = other_attrs {
let mut both_attrs = inner.to_vec();
both_attrs.extend_from_slice(attrs);
both_attrs
} else {
attrs.to_vec()
};There was a problem hiding this comment.
The reason I want to push for this is because it's very rare to have additional documentation on a re-export, but very common to have a lot of documentation on the original item.
There was a problem hiding this comment.
I don't think it has much different since Vec::new has cap=0.
Also, your and my code reallocates when other_attrs is_some, IIUC.
There was a problem hiding this comment.
One reallocation is really cheap though, it's only reallocation in a loop that's slow (which this doesn't do).
Co-authored-by: jyn514 <[email protected]>
|
I'm curious to see whether the perf difference is noticable. @bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit 2818032 with merge 45d9a0fe6bdb703dac3cc60690e84de52ab6cf6d... |
tesuji
changed the title
|
☀️ Try build successful - checks-actions, checks-azure |
|
Queued 45d9a0fe6bdb703dac3cc60690e84de52ab6cf6d with parent f3c923a, future comparison URL. |
jyn514
added
S-waiting-on-perf
|
Finished benchmarking try commit (45d9a0fe6bdb703dac3cc60690e84de52ab6cf6d): 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 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 |
|
Mostly noise, that's about what I expected. max-rss went down quite a bit, but not on doc runs (??) so it must be noise too. Well, this code is still better than before. @bors r+ |
|
📌 Commit 2818032 has been approved by |
bors
added
the
S-waiting-on-bors
|
☀️ Test successful - checks-actions, checks-azure |
Just a non-important micro-optimization.
r? @jyn514