Skip to content

Commit ce8bf91

Browse files
camelidcamelid
committed
rustdoc: Cache Attributes::doc_value
Locally, this appears to improve wall-time performance on the stm32f4 benchmark by about 2%. Also, it makes intuitive sense that we don't want to recompute this over and over. I also made the `doc_strings` field private since any modifications to it will mean the `doc_value_cache` becomes stale.
1 parent c78a294 commit ce8bf91

11 files changed

Lines changed: 82 additions & 61 deletions

File tree

‎src/librustdoc/clean/types.rs‎

Lines changed: 34 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
use std::fmt::Write;
22
use std::hash::Hash;
33
use std::path::PathBuf;
4-
use std::sync::{Arc, OnceLock as OnceCell};
4+
use std::sync::{Arc, OnceLock};
55
use std::{fmt, iter};
66

77
use arrayvec::ArrayVec;
@@ -483,22 +483,22 @@ impl Item {
483483
.iter()
484484
.filter_map(|attr| attr.deprecation_note().map(|note| note.span));
485485

486-
span_of_fragments(&self.attrs.doc_strings)
486+
span_of_fragments(&self.attrs.doc_strings())
487487
.into_iter()
488488
.chain(deprecation_notes)
489489
.reduce(|a, b| a.to(b))
490490
.unwrap_or_else(|| self.span(tcx).map_or(DUMMY_SP, |span| span.inner()))
491491
}
492492

493493
/// Combine all doc strings into a single value handling indentation and newlines as needed.
494-
pub(crate) fn doc_value(&self) -> String {
494+
pub(crate) fn doc_value(&self) -> &str {
495495
self.attrs.doc_value()
496496
}
497497

498498
/// Combine all doc strings into a single value handling indentation and newlines as needed.
499499
/// Returns `None` is there's no documentation at all, and `Some("")` if there is some
500500
/// documentation but it is empty (e.g. `#[doc = ""]`).
501-
pub(crate) fn opt_doc_value(&self) -> Option<String> {
501+
pub(crate) fn opt_doc_value(&self) -> Option<&str> {
502502
self.attrs.opt_doc_value()
503503
}
504504

@@ -550,7 +550,7 @@ impl Item {
550550
pub(crate) fn item_or_reexport_id(&self) -> ItemId {
551551
// added documentation on a reexport is always prepended.
552552
self.attrs
553-
.doc_strings
553+
.doc_strings()
554554
.first()
555555
.map(|x| x.item_id)
556556
.flatten()
@@ -1004,11 +1004,21 @@ pub struct RenderedLink {
10041004
/// as well as doc comments.
10051005
#[derive(Clone, Debug, Default)]
10061006
pub(crate) struct Attributes {
1007-
pub(crate) doc_strings: Vec<DocFragment>,
1007+
/// IMPORTANT! This should not be mutated since then `doc_value_cache` will be invalid.
1008+
doc_strings: Vec<DocFragment>,
10081009
pub(crate) other_attrs: ThinVec<hir::Attribute>,
1010+
doc_value_cache: OnceLock<Option<String>>,
10091011
}
10101012

10111013
impl Attributes {
1014+
fn new(doc_strings: Vec<DocFragment>, other_attrs: ThinVec<hir::Attribute>) -> Self {
1015+
Self { doc_strings, other_attrs, doc_value_cache: OnceLock::new() }
1016+
}
1017+
1018+
pub(crate) fn doc_strings(&self) -> &[DocFragment] {
1019+
&self.doc_strings
1020+
}
1021+
10121022
pub(crate) fn has_doc_flag<F: Fn(&DocAttribute) -> bool>(&self, callback: F) -> bool {
10131023
find_attr!(&self.other_attrs, Doc(d) if callback(d))
10141024
}
@@ -1036,26 +1046,30 @@ impl Attributes {
10361046
doc_only: bool,
10371047
) -> Attributes {
10381048
let (doc_strings, other_attrs) = attrs_to_doc_fragments(attrs, doc_only);
1039-
Attributes { doc_strings, other_attrs }
1049+
Attributes::new(doc_strings, other_attrs)
10401050
}
10411051

10421052
/// Combine all doc strings into a single value handling indentation and newlines as needed.
1043-
pub(crate) fn doc_value(&self) -> String {
1053+
pub(crate) fn doc_value(&self) -> &str {
10441054
self.opt_doc_value().unwrap_or_default()
10451055
}
10461056

10471057
/// Combine all doc strings into a single value handling indentation and newlines as needed.
10481058
/// Returns `None` is there's no documentation at all, and `Some("")` if there is some
10491059
/// documentation but it is empty (e.g. `#[doc = ""]`).
1050-
pub(crate) fn opt_doc_value(&self) -> Option<String> {
1051-
(!self.doc_strings.is_empty()).then(|| {
1052-
let mut res = String::new();
1053-
for frag in &self.doc_strings {
1054-
add_doc_fragment(&mut res, frag);
1055-
}
1056-
res.pop();
1057-
res
1058-
})
1060+
pub(crate) fn opt_doc_value(&self) -> Option<&str> {
1061+
self.doc_value_cache
1062+
.get_or_init(|| {
1063+
(!self.doc_strings.is_empty()).then(|| {
1064+
let mut res = String::new();
1065+
for frag in &self.doc_strings {
1066+
add_doc_fragment(&mut res, frag);
1067+
}
1068+
res.pop();
1069+
res
1070+
})
1071+
})
1072+
.as_deref()
10591073
}
10601074

10611075
pub(crate) fn get_doc_aliases(&self) -> Box<[Symbol]> {
@@ -1673,7 +1687,7 @@ impl PrimitiveType {
16731687
pub(crate) fn simplified_types() -> &'static SimplifiedTypes {
16741688
use PrimitiveType::*;
16751689
use ty::{FloatTy, IntTy, UintTy};
1676-
static CELL: OnceCell<SimplifiedTypes> = OnceCell::new();
1690+
static CELL: OnceLock<SimplifiedTypes> = OnceLock::new();
16771691

16781692
let single = |x| iter::once(x).collect();
16791693
CELL.get_or_init(move || {
@@ -1779,7 +1793,7 @@ impl PrimitiveType {
17791793
/// `rustc_doc_primitive`, then it's entirely random whether `std` or the other crate is picked.
17801794
/// (no_std crates are usually fine unless multiple dependencies define a primitive.)
17811795
pub(crate) fn primitive_locations(tcx: TyCtxt<'_>) -> &FxIndexMap<PrimitiveType, DefId> {
1782-
static PRIMITIVE_LOCATIONS: OnceCell<FxIndexMap<PrimitiveType, DefId>> = OnceCell::new();
1796+
static PRIMITIVE_LOCATIONS: OnceLock<FxIndexMap<PrimitiveType, DefId>> = OnceLock::new();
17831797
PRIMITIVE_LOCATIONS.get_or_init(|| {
17841798
let mut primitive_locations = FxIndexMap::default();
17851799
// NOTE: technically this misses crates that are only passed with `--extern` and not loaded when checking the crate.
@@ -2418,7 +2432,7 @@ mod size_asserts {
24182432
static_assert_size!(GenericParamDef, 40);
24192433
static_assert_size!(Generics, 16);
24202434
static_assert_size!(Item, 8);
2421-
static_assert_size!(ItemInner, 144);
2435+
static_assert_size!(ItemInner, 176);
24222436
static_assert_size!(ItemKind, 48);
24232437
static_assert_size!(PathSegment, 32);
24242438
static_assert_size!(Type, 32);

‎src/librustdoc/clean/types/tests.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ fn run_test(input: &str, expected: &str) {
2020
create_default_session_globals_then(|| {
2121
let mut s = create_doc_fragment(input);
2222
unindent_doc_fragments(&mut s);
23-
let attrs = Attributes { doc_strings: s, other_attrs: Default::default() };
23+
let attrs = Attributes::new(s, Default::default());
2424
assert_eq!(attrs.doc_value(), expected);
2525
});
2626
}

‎src/librustdoc/doctest/rust.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,7 @@ impl HirCollector<'_> {
181181
// anything else, this will combine them for us.
182182
let attrs = Attributes::from_hir(ast_attrs);
183183
if let Some(doc) = attrs.opt_doc_value() {
184-
let span = span_of_fragments(&attrs.doc_strings).unwrap_or(sp);
184+
let span = span_of_fragments(&attrs.doc_strings()).unwrap_or(sp);
185185
self.collector.position = if span.edition().at_least_rust_2024() {
186186
span
187187
} else {

‎src/librustdoc/json/conversions.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ impl JsonRenderer<'_> {
4141
(String::from(&**link), self.id_from_item_default(id.into()))
4242
})
4343
.collect();
44-
let docs = item.opt_doc_value();
44+
let docs = item.opt_doc_value().map(|s| s.to_owned());
4545
let attrs = item
4646
.attrs
4747
.other_attrs

‎src/librustdoc/passes/calculate_doc_coverage.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -211,7 +211,7 @@ impl DocVisitor<'_> for CoverageCalculator<'_, '_> {
211211
// doesn't make sense, as all methods on a type are in one single impl block
212212
clean::ImplItem(_) => {}
213213
_ => {
214-
let has_docs = !i.attrs.doc_strings.is_empty();
214+
let has_docs = !i.attrs.doc_strings().is_empty();
215215
let mut tests = Tests { found_tests: 0 };
216216

217217
find_testable_code(&i.doc_value(), &mut tests, ErrorCodes::No, None);

‎src/librustdoc/passes/collect_intra_doc_links.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1095,7 +1095,7 @@ impl LinkCollector<'_, '_> {
10951095
// In the presence of re-exports, this is not the same as the module of the item.
10961096
// Rather than merging all documentation into one, resolve it one attribute at a time
10971097
// so we know which module it came from.
1098-
for (item_id, doc) in prepare_to_doc_link_resolution(&item.attrs.doc_strings) {
1098+
for (item_id, doc) in prepare_to_doc_link_resolution(&item.attrs.doc_strings()) {
10991099
if !may_have_doc_links(&doc) {
11001100
continue;
11011101
}
@@ -1464,7 +1464,7 @@ impl LinkCollector<'_, '_> {
14641464
self.cx.tcx,
14651465
dox,
14661466
ori_link.inner_range(),
1467-
&item.attrs.doc_strings,
1467+
&item.attrs.doc_strings(),
14681468
) {
14691469
Some((sp, _)) => sp,
14701470
None => item.attr_span(self.cx.tcx),
@@ -1911,7 +1911,7 @@ fn report_diagnostic(
19111911
MarkdownLinkRange::Destination(md_range) => {
19121912
let mut md_range = md_range.clone();
19131913
let sp =
1914-
source_span_for_markdown_range(tcx, dox, &md_range, &item.attrs.doc_strings)
1914+
source_span_for_markdown_range(tcx, dox, &md_range, &item.attrs.doc_strings())
19151915
.map(|(mut sp, _)| {
19161916
while dox.as_bytes().get(md_range.start) == Some(&b' ')
19171917
|| dox.as_bytes().get(md_range.start) == Some(&b'`')
@@ -1930,7 +1930,7 @@ fn report_diagnostic(
19301930
(sp, MarkdownLinkRange::Destination(md_range))
19311931
}
19321932
MarkdownLinkRange::WholeLink(md_range) => (
1933-
source_span_for_markdown_range(tcx, dox, md_range, &item.attrs.doc_strings)
1933+
source_span_for_markdown_range(tcx, dox, md_range, &item.attrs.doc_strings())
19341934
.map(|(sp, _)| sp),
19351935
link_range.clone(),
19361936
),

‎src/librustdoc/passes/lint/bare_urls.rs‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,9 @@ pub(super) fn visit_item(cx: &DocContext<'_>, item: &Item, hir_id: HirId, dox: &
2121
msg: &'static str,
2222
range: Range<usize>,
2323
without_brackets: Option<&str>| {
24-
let maybe_sp = source_span_for_markdown_range(cx.tcx, dox, &range, &item.attrs.doc_strings)
25-
.map(|(sp, _)| sp);
24+
let maybe_sp =
25+
source_span_for_markdown_range(cx.tcx, dox, &range, &item.attrs.doc_strings())
26+
.map(|(sp, _)| sp);
2627
let sp = maybe_sp.unwrap_or_else(|| item.attr_span(cx.tcx));
2728
cx.tcx.node_span_lint(crate::lint::BARE_URLS, hir_id, sp, |lint| {
2829
lint.primary_message(msg)

‎src/librustdoc/passes/lint/check_code_block_syntax.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ fn check_rust_syntax(
8383
cx.tcx,
8484
dox,
8585
&code_block.range,
86-
&item.attrs.doc_strings,
86+
&item.attrs.doc_strings(),
8787
) {
8888
Some((sp, _)) => (sp, true),
8989
None => (item.attr_span(cx.tcx), false),

‎src/librustdoc/passes/lint/html_tags.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ use crate::html::markdown::main_body_opts;
1717
pub(crate) fn visit_item(cx: &DocContext<'_>, item: &Item, hir_id: HirId, dox: &str) {
1818
let tcx = cx.tcx;
1919
let report_diag = |msg: String, range: &Range<usize>, is_open_tag: bool| {
20-
let sp = match source_span_for_markdown_range(tcx, dox, range, &item.attrs.doc_strings) {
20+
let sp = match source_span_for_markdown_range(tcx, dox, range, &item.attrs.doc_strings()) {
2121
Some((sp, _)) => sp,
2222
None => item.attr_span(tcx),
2323
};
@@ -55,7 +55,7 @@ pub(crate) fn visit_item(cx: &DocContext<'_>, item: &Item, hir_id: HirId, dox: &
5555
tcx,
5656
dox,
5757
&(generics_start..generics_end),
58-
&item.attrs.doc_strings,
58+
&item.attrs.doc_strings(),
5959
) {
6060
Some((sp, _)) => sp,
6161
None => item.attr_span(tcx),

‎src/librustdoc/passes/lint/redundant_explicit_links.rs‎

Lines changed: 32 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ struct LinkData {
2525
}
2626

2727
pub(crate) fn visit_item(cx: &DocContext<'_>, item: &Item, hir_id: HirId) {
28-
let hunks = prepare_to_doc_link_resolution(&item.attrs.doc_strings);
28+
let hunks = prepare_to_doc_link_resolution(&item.attrs.doc_strings());
2929
for (item_id, doc) in hunks {
3030
if let Some(item_id) = item_id.or(item.def_id())
3131
&& !doc.is_empty()
@@ -160,22 +160,25 @@ fn check_inline_or_reference_unknown_redundancy(
160160
(find_resolution(resolutions, &dest)?, find_resolution(resolutions, resolvable_link)?);
161161

162162
if dest_res == display_res {
163-
let link_span =
164-
match source_span_for_markdown_range(cx.tcx, doc, &link_range, &item.attrs.doc_strings)
165-
{
166-
Some((sp, from_expansion)) => {
167-
if from_expansion {
168-
return None;
169-
}
170-
sp
163+
let link_span = match source_span_for_markdown_range(
164+
cx.tcx,
165+
doc,
166+
&link_range,
167+
&item.attrs.doc_strings(),
168+
) {
169+
Some((sp, from_expansion)) => {
170+
if from_expansion {
171+
return None;
171172
}
172-
None => item.attr_span(cx.tcx),
173-
};
173+
sp
174+
}
175+
None => item.attr_span(cx.tcx),
176+
};
174177
let (explicit_span, false) = source_span_for_markdown_range(
175178
cx.tcx,
176179
doc,
177180
&offset_explicit_range(doc, link_range, open, close),
178-
&item.attrs.doc_strings,
181+
&item.attrs.doc_strings(),
179182
)?
180183
else {
181184
// This `span` comes from macro expansion so skipping it.
@@ -185,7 +188,7 @@ fn check_inline_or_reference_unknown_redundancy(
185188
cx.tcx,
186189
doc,
187190
resolvable_link_range,
188-
&item.attrs.doc_strings,
191+
&item.attrs.doc_strings(),
189192
)?
190193
else {
191194
// This `span` comes from macro expansion so skipping it.
@@ -221,22 +224,25 @@ fn check_reference_redundancy(
221224
(find_resolution(resolutions, dest)?, find_resolution(resolutions, resolvable_link)?);
222225

223226
if dest_res == display_res {
224-
let link_span =
225-
match source_span_for_markdown_range(cx.tcx, doc, &link_range, &item.attrs.doc_strings)
226-
{
227-
Some((sp, from_expansion)) => {
228-
if from_expansion {
229-
return None;
230-
}
231-
sp
227+
let link_span = match source_span_for_markdown_range(
228+
cx.tcx,
229+
doc,
230+
&link_range,
231+
&item.attrs.doc_strings(),
232+
) {
233+
Some((sp, from_expansion)) => {
234+
if from_expansion {
235+
return None;
232236
}
233-
None => item.attr_span(cx.tcx),
234-
};
237+
sp
238+
}
239+
None => item.attr_span(cx.tcx),
240+
};
235241
let (explicit_span, false) = source_span_for_markdown_range(
236242
cx.tcx,
237243
doc,
238244
&offset_explicit_range(doc, link_range.clone(), b'[', b']'),
239-
&item.attrs.doc_strings,
245+
&item.attrs.doc_strings(),
240246
)?
241247
else {
242248
// This `span` comes from macro expansion so skipping it.
@@ -246,7 +252,7 @@ fn check_reference_redundancy(
246252
cx.tcx,
247253
doc,
248254
resolvable_link_range,
249-
&item.attrs.doc_strings,
255+
&item.attrs.doc_strings(),
250256
)?
251257
else {
252258
// This `span` comes from macro expansion so skipping it.
@@ -256,7 +262,7 @@ fn check_reference_redundancy(
256262
cx.tcx,
257263
doc,
258264
&offset_reference_def_range(doc, dest, link_range),
259-
&item.attrs.doc_strings,
265+
&item.attrs.doc_strings(),
260266
)?;
261267

262268
cx.tcx.node_span_lint(crate::lint::REDUNDANT_EXPLICIT_LINKS, hir_id, explicit_span, |lint| {

0 commit comments

Comments
 (0)