Skip to content

Commit

Permalink
Auto merge of #130857 - lukas-code:no-clones-allowed, r=notriddle
Browse files Browse the repository at this point in the history
rustdoc perf: clone `clean::Item` less

In #130798, I caused a small perf regression for rustdoc (see #130807 (comment)), so here is a small improvement to make up for it 😺.

This change is actually unrelated to the minor perf regression in `Item::stability` and instead fixes a more relevant perf problem that I found while investigating: For certain crates with many impls on type aliases, we unnecessarily cloned large `clean::Item`s multiple times -- now we just borrow them.
  • Loading branch information
bors committed Sep 27, 2024
2 parents b9dc4a3 + 345077a commit a3f76a2
Show file tree
Hide file tree
Showing 14 changed files with 42 additions and 44 deletions.
5 changes: 2 additions & 3 deletions src/librustdoc/clean/inline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -837,8 +837,7 @@ pub(crate) fn record_extern_trait(cx: &mut DocContext<'_>, did: DefId) {
}

{
if cx.external_traits.borrow().contains_key(&did) || cx.active_extern_traits.contains(&did)
{
if cx.external_traits.contains_key(&did) || cx.active_extern_traits.contains(&did) {
return;
}
}
Expand All @@ -850,6 +849,6 @@ pub(crate) fn record_extern_trait(cx: &mut DocContext<'_>, did: DefId) {
debug!("record_extern_trait: {did:?}");
let trait_ = build_external_trait(cx, did);

cx.external_traits.borrow_mut().insert(did, trait_);
cx.external_traits.insert(did, trait_);
cx.active_extern_traits.remove(&did);
}
4 changes: 1 addition & 3 deletions src/librustdoc/clean/types.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
use std::borrow::Cow;
use std::cell::RefCell;
use std::hash::Hash;
use std::path::PathBuf;
use std::rc::Rc;
use std::sync::{Arc, OnceLock as OnceCell};
use std::{fmt, iter};

Expand Down Expand Up @@ -115,7 +113,7 @@ impl From<DefId> for ItemId {
pub(crate) struct Crate {
pub(crate) module: Item,
/// Only here so that they can be filtered through the rustdoc passes.
pub(crate) external_traits: Rc<RefCell<FxHashMap<DefId, Trait>>>,
pub(crate) external_traits: Box<FxHashMap<DefId, Trait>>,
}

impl Crate {
Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/clean/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ pub(crate) fn krate(cx: &mut DocContext<'_>) -> Crate {
}));
}

Crate { module, external_traits: cx.external_traits.clone() }
Crate { module, external_traits: Box::new(mem::take(&mut cx.external_traits)) }
}

pub(crate) fn clean_middle_generic_args<'tcx>(
Expand Down
6 changes: 2 additions & 4 deletions src/librustdoc/core.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
use std::cell::RefCell;
use std::rc::Rc;
use std::sync::atomic::AtomicBool;
use std::sync::{Arc, LazyLock};
use std::{io, mem};
Expand Down Expand Up @@ -41,7 +39,7 @@ pub(crate) struct DocContext<'tcx> {
/// Most of this logic is copied from rustc_lint::late.
pub(crate) param_env: ParamEnv<'tcx>,
/// Later on moved through `clean::Crate` into `cache`
pub(crate) external_traits: Rc<RefCell<FxHashMap<DefId, clean::Trait>>>,
pub(crate) external_traits: FxHashMap<DefId, clean::Trait>,
/// Used while populating `external_traits` to ensure we don't process the same trait twice at
/// the same time.
pub(crate) active_extern_traits: DefIdSet,
Expand Down Expand Up @@ -359,7 +357,7 @@ pub(crate) fn run_global_ctxt(
// Note that in case of `#![no_core]`, the trait is not available.
if let Some(sized_trait_did) = ctxt.tcx.lang_items().sized_trait() {
let sized_trait = build_external_trait(&mut ctxt, sized_trait_did);
ctxt.external_traits.borrow_mut().insert(sized_trait_did, sized_trait);
ctxt.external_traits.insert(sized_trait_did, sized_trait);
}

debug!("crate: {:?}", tcx.hir().krate());
Expand Down
11 changes: 7 additions & 4 deletions src/librustdoc/fold.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::mem;

use crate::clean::*;

pub(crate) fn strip_item(mut item: Item) -> Item {
Expand Down Expand Up @@ -116,10 +118,11 @@ pub(crate) trait DocFolder: Sized {
fn fold_crate(&mut self, mut c: Crate) -> Crate {
c.module = self.fold_item(c.module).unwrap();

let external_traits = { std::mem::take(&mut *c.external_traits.borrow_mut()) };
for (k, mut v) in external_traits {
v.items = v.items.into_iter().filter_map(|i| self.fold_item(i)).collect();
c.external_traits.borrow_mut().insert(k, v);
for trait_ in c.external_traits.values_mut() {
trait_.items = mem::take(&mut trait_.items)
.into_iter()
.filter_map(|i| self.fold_item(i))
.collect();
}

c
Expand Down
3 changes: 2 additions & 1 deletion src/librustdoc/formats/cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,8 @@ impl Cache {

// Crawl the crate to build various caches used for the output
debug!(?cx.cache.crate_version);
cx.cache.traits = krate.external_traits.take();
assert!(cx.external_traits.is_empty());
cx.cache.traits = mem::take(&mut krate.external_traits);

// Cache where all our extern crates are located
// FIXME: this part is specific to HTML so it'd be nice to remove it from the common code
Expand Down
18 changes: 9 additions & 9 deletions src/librustdoc/html/render/write_shared.rs
Original file line number Diff line number Diff line change
Expand Up @@ -824,9 +824,9 @@ impl Serialize for Implementor {
/// this visitor works to reverse that: `aliased_types` is a map
/// from target to the aliases that reference it, and each one
/// will generate one file.
struct TypeImplCollector<'cx, 'cache> {
struct TypeImplCollector<'cx, 'cache, 'item> {
/// Map from DefId-of-aliased-type to its data.
aliased_types: IndexMap<DefId, AliasedType<'cache>>,
aliased_types: IndexMap<DefId, AliasedType<'cache, 'item>>,
visited_aliases: FxHashSet<DefId>,
cache: &'cache Cache,
cx: &'cache mut Context<'cx>,
Expand All @@ -847,26 +847,26 @@ struct TypeImplCollector<'cx, 'cache> {
/// ]
/// )
/// ```
struct AliasedType<'cache> {
struct AliasedType<'cache, 'item> {
/// This is used to generate the actual filename of this aliased type.
target_fqp: &'cache [Symbol],
target_type: ItemType,
/// This is the data stored inside the file.
/// ItemId is used to deduplicate impls.
impl_: IndexMap<ItemId, AliasedTypeImpl<'cache>>,
impl_: IndexMap<ItemId, AliasedTypeImpl<'cache, 'item>>,
}

/// The `impl_` contains data that's used to figure out if an alias will work,
/// and to generate the HTML at the end.
///
/// The `type_aliases` list is built up with each type alias that matches.
struct AliasedTypeImpl<'cache> {
struct AliasedTypeImpl<'cache, 'item> {
impl_: &'cache Impl,
type_aliases: Vec<(&'cache [Symbol], Item)>,
type_aliases: Vec<(&'cache [Symbol], &'item Item)>,
}

impl<'cx, 'cache> DocVisitor for TypeImplCollector<'cx, 'cache> {
fn visit_item(&mut self, it: &Item) {
impl<'cx, 'cache, 'item> DocVisitor<'item> for TypeImplCollector<'cx, 'cache, 'item> {
fn visit_item(&mut self, it: &'item Item) {
self.visit_item_recur(it);
let cache = self.cache;
let ItemKind::TypeAliasItem(ref t) = it.kind else { return };
Expand Down Expand Up @@ -927,7 +927,7 @@ impl<'cx, 'cache> DocVisitor for TypeImplCollector<'cx, 'cache> {
continue;
}
// This impl was not found in the set of rejected impls
aliased_type_impl.type_aliases.push((&self_fqp[..], it.clone()));
aliased_type_impl.type_aliases.push((&self_fqp[..], it));
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/librustdoc/html/sources.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ impl LocalSourcesCollector<'_, '_> {
}
}

impl DocVisitor for LocalSourcesCollector<'_, '_> {
impl DocVisitor<'_> for LocalSourcesCollector<'_, '_> {
fn visit_item(&mut self, item: &clean::Item) {
self.add_local_source(item);

Expand All @@ -122,7 +122,7 @@ struct SourceCollector<'a, 'tcx> {
crate_name: &'a str,
}

impl DocVisitor for SourceCollector<'_, '_> {
impl DocVisitor<'_> for SourceCollector<'_, '_> {
fn visit_item(&mut self, item: &clean::Item) {
if !self.cx.include_sources {
return;
Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/passes/calculate_doc_coverage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ impl<'a, 'b> CoverageCalculator<'a, 'b> {
}
}

impl<'a, 'b> DocVisitor for CoverageCalculator<'a, 'b> {
impl<'a, 'b> DocVisitor<'_> for CoverageCalculator<'a, 'b> {
fn visit_item(&mut self, i: &clean::Item) {
if !i.item_id.is_local() {
// non-local items are skipped because they can be out of the users control,
Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/passes/check_doc_test_visibility.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ pub(crate) fn check_doc_test_visibility(krate: Crate, cx: &mut DocContext<'_>) -
krate
}

impl<'a, 'tcx> DocVisitor for DocTestVisibilityLinter<'a, 'tcx> {
impl<'a, 'tcx> DocVisitor<'_> for DocTestVisibilityLinter<'a, 'tcx> {
fn visit_item(&mut self, item: &Item) {
look_for_tests(self.cx, &item.doc_value(), item);

Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/passes/collect_intra_doc_links.rs
Original file line number Diff line number Diff line change
Expand Up @@ -816,7 +816,7 @@ fn is_derive_trait_collision<T>(ns: &PerNS<Result<Vec<(Res, T)>, ResolutionFailu
}
}

impl<'a, 'tcx> DocVisitor for LinkCollector<'a, 'tcx> {
impl<'a, 'tcx> DocVisitor<'_> for LinkCollector<'a, 'tcx> {
fn visit_item(&mut self, item: &Item) {
self.resolve_links(item);
self.visit_item_recur(item)
Expand Down
6 changes: 4 additions & 2 deletions src/librustdoc/passes/collect_trait_impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,8 @@ pub(crate) fn collect_trait_impls(mut krate: Crate, cx: &mut DocContext<'_>) ->
panic!("collect-trait-impls can't run");
};

krate.external_traits.extend(cx.external_traits.drain());

krate
}

Expand All @@ -227,7 +229,7 @@ struct SyntheticImplCollector<'a, 'tcx> {
impls: Vec<Item>,
}

impl<'a, 'tcx> DocVisitor for SyntheticImplCollector<'a, 'tcx> {
impl<'a, 'tcx> DocVisitor<'_> for SyntheticImplCollector<'a, 'tcx> {
fn visit_item(&mut self, i: &Item) {
if i.is_struct() || i.is_enum() || i.is_union() {
// FIXME(eddyb) is this `doc(hidden)` check needed?
Expand All @@ -254,7 +256,7 @@ impl<'cache> ItemAndAliasCollector<'cache> {
}
}

impl<'cache> DocVisitor for ItemAndAliasCollector<'cache> {
impl<'cache> DocVisitor<'_> for ItemAndAliasCollector<'cache> {
fn visit_item(&mut self, i: &Item) {
self.items.insert(i.item_id);

Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/passes/lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ pub(crate) fn run_lints(krate: Crate, cx: &mut DocContext<'_>) -> Crate {
krate
}

impl<'a, 'tcx> DocVisitor for Linter<'a, 'tcx> {
impl<'a, 'tcx> DocVisitor<'_> for Linter<'a, 'tcx> {
fn visit_item(&mut self, item: &Item) {
let Some(hir_id) = DocContext::as_local_hir_id(self.cx.tcx, item.item_id) else {
// If non-local, no need to check anything.
Expand Down
19 changes: 8 additions & 11 deletions src/librustdoc/visit.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
use crate::clean::*;

pub(crate) trait DocVisitor: Sized {
fn visit_item(&mut self, item: &Item) {
pub(crate) trait DocVisitor<'a>: Sized {
fn visit_item(&mut self, item: &'a Item) {
self.visit_item_recur(item)
}

/// don't override!
fn visit_inner_recur(&mut self, kind: &ItemKind) {
fn visit_inner_recur(&mut self, kind: &'a ItemKind) {
match kind {
StrippedItem(..) => unreachable!(),
ModuleItem(i) => {
Expand Down Expand Up @@ -47,25 +47,22 @@ pub(crate) trait DocVisitor: Sized {
}

/// don't override!
fn visit_item_recur(&mut self, item: &Item) {
fn visit_item_recur(&mut self, item: &'a Item) {
match &item.kind {
StrippedItem(i) => self.visit_inner_recur(&*i),
_ => self.visit_inner_recur(&item.kind),
}
}

fn visit_mod(&mut self, m: &Module) {
fn visit_mod(&mut self, m: &'a Module) {
m.items.iter().for_each(|i| self.visit_item(i))
}

fn visit_crate(&mut self, c: &Crate) {
fn visit_crate(&mut self, c: &'a Crate) {
self.visit_item(&c.module);

// FIXME: make this a simple by-ref for loop once external_traits is cleaned up
let external_traits = { std::mem::take(&mut *c.external_traits.borrow_mut()) };
for (k, v) in external_traits {
v.items.iter().for_each(|i| self.visit_item(i));
c.external_traits.borrow_mut().insert(k, v);
for trait_ in c.external_traits.values() {
trait_.items.iter().for_each(|i| self.visit_item(i));
}
}
}

0 comments on commit a3f76a2

Please sign in to comment.