From 9763eb87a3a389b90c6540f56194d2f7f78d62be Mon Sep 17 00:00:00 2001 From: Camelid Date: Sun, 21 Feb 2021 14:21:43 -0800 Subject: [PATCH 1/8] rustdoc: Move most shared fields to `SharedContext` ...and remove `Rc`s for the moved fields. The only shared one that I didn't move was `cache`; see the doc-comment I added to `cache` for details. --- src/librustdoc/html/render/context.rs | 56 +++++++++++++-------------- src/librustdoc/html/render/mod.rs | 29 ++++++++++---- 2 files changed, 48 insertions(+), 37 deletions(-) diff --git a/src/librustdoc/html/render/context.rs b/src/librustdoc/html/render/context.rs index 976168a9ac4..990a05679f1 100644 --- a/src/librustdoc/html/render/context.rs +++ b/src/librustdoc/html/render/context.rs @@ -3,11 +3,11 @@ use std::collections::BTreeMap; use std::io; use std::path::PathBuf; use std::rc::Rc; -use std::sync::mpsc::{channel, Receiver}; +use std::sync::mpsc::channel; use std::sync::Arc; use rustc_data_structures::fx::FxHashMap; -use rustc_hir::def_id::{DefId, LOCAL_CRATE}; +use rustc_hir::def_id::LOCAL_CRATE; use rustc_middle::ty::TyCtxt; use rustc_session::Session; use rustc_span::edition::Edition; @@ -31,7 +31,7 @@ use crate::formats::item_type::ItemType; use crate::formats::FormatRenderer; use crate::html::escape::Escape; use crate::html::format::Buffer; -use crate::html::markdown::{self, plain_text_summary, ErrorCodes, IdMap}; +use crate::html::markdown::{self, plain_text_summary, ErrorCodes}; use crate::html::{layout, sources}; /// Major driving force in all rustdoc rendering. This contains information @@ -53,20 +53,16 @@ crate struct Context<'tcx> { /// real location of an item. This is used to allow external links to /// publicly reused items to redirect to the right location. crate render_redirect_pages: bool, - /// `None` by default, depends on the `generate-redirect-map` option flag. If this field is set - /// to `Some(...)`, it'll store redirections and then generate a JSON file at the top level of - /// the crate. - crate redirections: Option>>>, - /// The map used to ensure all generated 'id=' attributes are unique. - pub(super) id_map: Rc>, - /// Tracks section IDs for `Deref` targets so they match in both the main - /// body and the sidebar. - pub(super) deref_id_map: Rc>>, crate shared: Arc>, - all: Rc>, - /// Storage for the errors produced while generating documentation so they - /// can be printed together at the end. - crate errors: Rc>, + /// The [`Cache`] used during rendering. + /// + /// Ideally the cache would be in [`SharedContext`], but it's mutated + /// between when the `SharedContext` is created and when `Context` + /// is created, so more refactoring would be needed. + /// + /// It's immutable once in `Context`, so it's not as bad that it's not in + /// `SharedContext`. + // FIXME: move `cache` to `SharedContext` crate cache: Rc, } @@ -91,7 +87,7 @@ impl<'tcx> Context<'tcx> { } pub(super) fn derive_id(&self, id: String) -> String { - let mut map = self.id_map.borrow_mut(); + let mut map = self.shared.id_map.borrow_mut(); map.derive(id) } @@ -149,8 +145,8 @@ impl<'tcx> Context<'tcx> { }; { - self.id_map.borrow_mut().reset(); - self.id_map.borrow_mut().populate(&INITIAL_IDS); + self.shared.id_map.borrow_mut().reset(); + self.shared.id_map.borrow_mut().populate(&INITIAL_IDS); } if !self.render_redirect_pages { @@ -169,7 +165,7 @@ impl<'tcx> Context<'tcx> { path.push('/'); } path.push_str(&item_path(ty, names.last().unwrap())); - match self.redirections { + match self.shared.redirections { Some(ref redirections) => { let mut current_path = String::new(); for name in &self.current { @@ -383,6 +379,11 @@ impl<'tcx> FormatRenderer<'tcx> for Context<'tcx> { edition, codes: ErrorCodes::from(unstable_features.is_nightly_build()), playground, + id_map: RefCell::new(id_map), + deref_id_map: RefCell::new(FxHashMap::default()), + all: RefCell::new(AllTypes::new()), + errors: receiver, + redirections: if generate_redirect_map { Some(Default::default()) } else { None }, }; // Add the default themes to the `Vec` of stylepaths @@ -409,13 +410,8 @@ impl<'tcx> FormatRenderer<'tcx> for Context<'tcx> { current: Vec::new(), dst, render_redirect_pages: false, - id_map: Rc::new(RefCell::new(id_map)), - deref_id_map: Rc::new(RefCell::new(FxHashMap::default())), shared: Arc::new(scx), - all: Rc::new(RefCell::new(AllTypes::new())), - errors: Rc::new(receiver), cache: Rc::new(cache), - redirections: if generate_redirect_map { Some(Default::default()) } else { None }, }; CURRENT_DEPTH.with(|s| s.set(0)); @@ -464,7 +460,7 @@ impl<'tcx> FormatRenderer<'tcx> for Context<'tcx> { } else { String::new() }; - let all = self.all.replace(AllTypes::new()); + let all = self.shared.all.replace(AllTypes::new()); let v = layout::render( &self.shared.layout, &page, @@ -494,7 +490,7 @@ impl<'tcx> FormatRenderer<'tcx> for Context<'tcx> { &style_files, ); self.shared.fs.write(&settings_file, v.as_bytes())?; - if let Some(redirections) = self.redirections.take() { + if let Some(ref redirections) = self.shared.redirections { if !redirections.borrow().is_empty() { let redirect_map_path = self.dst.join(&*krate.name.as_str()).join("redirect-map.json"); @@ -506,7 +502,7 @@ impl<'tcx> FormatRenderer<'tcx> for Context<'tcx> { // Flush pending errors. Arc::get_mut(&mut self.shared).unwrap().fs.close(); - let nb_errors = self.errors.iter().map(|err| diag.struct_err(&err).emit()).count(); + let nb_errors = self.shared.errors.iter().map(|err| diag.struct_err(&err).emit()).count(); if nb_errors > 0 { Err(Error::new(io::Error::new(io::ErrorKind::Other, "I/O error"), "")) } else { @@ -585,13 +581,13 @@ impl<'tcx> FormatRenderer<'tcx> for Context<'tcx> { self.shared.fs.write(&joint_dst, buf.as_bytes())?; if !self.render_redirect_pages { - self.all.borrow_mut().append(full_path(self, &item), &item_type); + self.shared.all.borrow_mut().append(full_path(self, &item), &item_type); } // If the item is a macro, redirect from the old macro URL (with !) // to the new one (without). if item_type == ItemType::Macro { let redir_name = format!("{}.{}!.html", item_type, name); - if let Some(ref redirections) = self.redirections { + if let Some(ref redirections) = self.shared.redirections { let crate_name = &self.shared.layout.krate; redirections.borrow_mut().insert( format!("{}/{}", crate_name, redir_name), diff --git a/src/librustdoc/html/render/mod.rs b/src/librustdoc/html/render/mod.rs index 50cae50c2c3..3bb13d29992 100644 --- a/src/librustdoc/html/render/mod.rs +++ b/src/librustdoc/html/render/mod.rs @@ -41,6 +41,7 @@ use std::fmt; use std::path::{Path, PathBuf}; use std::str; use std::string::ToString; +use std::sync::mpsc::Receiver; use itertools::Itertools; use rustc_ast_pretty::pprust; @@ -69,7 +70,7 @@ use crate::html::format::{ PrintWithSpace, WhereClause, }; use crate::html::layout; -use crate::html::markdown::{self, ErrorCodes, Markdown, MarkdownHtml, MarkdownSummaryLine}; +use crate::html::markdown::{self, ErrorCodes, IdMap, Markdown, MarkdownHtml, MarkdownSummaryLine}; /// A pair of name and its optional document. crate type NameDoc = (String, Option); @@ -119,6 +120,19 @@ crate struct SharedContext<'tcx> { crate edition: Edition, crate codes: ErrorCodes, playground: Option, + /// The map used to ensure all generated 'id=' attributes are unique. + id_map: RefCell, + /// Tracks section IDs for `Deref` targets so they match in both the main + /// body and the sidebar. + deref_id_map: RefCell>, + all: RefCell, + /// Storage for the errors produced while generating documentation so they + /// can be printed together at the end. + crate errors: Receiver, + /// `None` by default, depends on the `generate-redirect-map` option flag. If this field is set + /// to `Some(...)`, it'll store redirections and then generate a JSON file at the top level of + /// the crate. + crate redirections: Option>>, } impl SharedContext<'_> { @@ -635,7 +649,7 @@ fn render_markdown( prefix: &str, is_hidden: bool, ) { - let mut ids = cx.id_map.borrow_mut(); + let mut ids = cx.shared.id_map.borrow_mut(); write!( w, "
{}{}
", @@ -795,7 +809,7 @@ fn short_item_info( if let Some(note) = note { let note = note.as_str(); - let mut ids = cx.id_map.borrow_mut(); + let mut ids = cx.shared.id_map.borrow_mut(); let html = MarkdownHtml( ¬e, &mut ids, @@ -834,7 +848,7 @@ fn short_item_info( message.push_str(&format!(" ({})", feature)); if let Some(unstable_reason) = reason { - let mut ids = cx.id_map.borrow_mut(); + let mut ids = cx.shared.id_map.borrow_mut(); message = format!( "
{}{}
", message, @@ -1174,7 +1188,8 @@ fn render_assoc_items( type_.print(cx.cache()) ))); debug!("Adding {} to deref id map", type_.print(cx.cache())); - cx.deref_id_map + cx.shared + .deref_id_map .borrow_mut() .insert(type_.def_id_full(cx.cache()).unwrap(), id.clone()); write!( @@ -1481,7 +1496,7 @@ fn render_impl( } if let Some(ref dox) = cx.shared.maybe_collapsed_doc_value(&i.impl_item) { - let mut ids = cx.id_map.borrow_mut(); + let mut ids = cx.shared.id_map.borrow_mut(); write!( w, "
{}
", @@ -2030,7 +2045,7 @@ fn sidebar_deref_methods(cx: &Context<'_>, out: &mut Buffer, impl_: &Impl, v: &V .flat_map(|i| get_methods(i.inner_impl(), true, &mut used_links, deref_mut, c)) .collect::>(); if !ret.is_empty() { - let deref_id_map = cx.deref_id_map.borrow(); + let deref_id_map = cx.shared.deref_id_map.borrow(); let id = deref_id_map .get(&real_target.def_id_full(cx.cache()).unwrap()) .expect("Deref section without derived id"); From c4bb66c2842cdd433ad6dbe1168726ff1ca148d3 Mon Sep 17 00:00:00 2001 From: Camelid Date: Sun, 21 Feb 2021 14:25:21 -0800 Subject: [PATCH 2/8] rustdoc: Replace `Arc` around `SharedContext` with `Rc` It doesn't look like it's shared across threads, so it doesn't need to be thread-safe. Of course, since we're using Rust, we'll get an error if we try to share it across threads, so this should be safe :) --- src/librustdoc/html/render/context.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/librustdoc/html/render/context.rs b/src/librustdoc/html/render/context.rs index 990a05679f1..064c9759b58 100644 --- a/src/librustdoc/html/render/context.rs +++ b/src/librustdoc/html/render/context.rs @@ -4,7 +4,6 @@ use std::io; use std::path::PathBuf; use std::rc::Rc; use std::sync::mpsc::channel; -use std::sync::Arc; use rustc_data_structures::fx::FxHashMap; use rustc_hir::def_id::LOCAL_CRATE; @@ -53,7 +52,7 @@ crate struct Context<'tcx> { /// real location of an item. This is used to allow external links to /// publicly reused items to redirect to the right location. crate render_redirect_pages: bool, - crate shared: Arc>, + crate shared: Rc>, /// The [`Cache`] used during rendering. /// /// Ideally the cache would be in [`SharedContext`], but it's mutated @@ -410,16 +409,16 @@ impl<'tcx> FormatRenderer<'tcx> for Context<'tcx> { current: Vec::new(), dst, render_redirect_pages: false, - shared: Arc::new(scx), + shared: Rc::new(scx), cache: Rc::new(cache), }; CURRENT_DEPTH.with(|s| s.set(0)); // Write shared runs within a flock; disable thread dispatching of IO temporarily. - Arc::get_mut(&mut cx.shared).unwrap().fs.set_sync_only(true); + Rc::get_mut(&mut cx.shared).unwrap().fs.set_sync_only(true); write_shared(&cx, &krate, index, &md_opts)?; - Arc::get_mut(&mut cx.shared).unwrap().fs.set_sync_only(false); + Rc::get_mut(&mut cx.shared).unwrap().fs.set_sync_only(false); Ok((cx, krate)) } @@ -501,7 +500,7 @@ impl<'tcx> FormatRenderer<'tcx> for Context<'tcx> { } // Flush pending errors. - Arc::get_mut(&mut self.shared).unwrap().fs.close(); + Rc::get_mut(&mut self.shared).unwrap().fs.close(); let nb_errors = self.shared.errors.iter().map(|err| diag.struct_err(&err).emit()).count(); if nb_errors > 0 { Err(Error::new(io::Error::new(io::ErrorKind::Other, "I/O error"), "")) From b3d2a371bb8c0bccbc07d3a29c79218495549509 Mon Sep 17 00:00:00 2001 From: Camelid Date: Sun, 21 Feb 2021 14:35:15 -0800 Subject: [PATCH 3/8] rustdoc: Make a bunch of fields private Also create issue for removing shared mutable state. --- src/librustdoc/html/render/context.rs | 15 ++++++++++----- src/librustdoc/html/render/mod.rs | 15 ++++++++------- 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/src/librustdoc/html/render/context.rs b/src/librustdoc/html/render/context.rs index 064c9759b58..e8d323b9af1 100644 --- a/src/librustdoc/html/render/context.rs +++ b/src/librustdoc/html/render/context.rs @@ -44,15 +44,20 @@ use crate::html::{layout, sources}; crate struct Context<'tcx> { /// Current hierarchy of components leading down to what's currently being /// rendered - crate current: Vec, + pub(super) current: Vec, /// The current destination folder of where HTML artifacts should be placed. /// This changes as the context descends into the module hierarchy. - crate dst: PathBuf, + pub(super) dst: PathBuf, /// A flag, which when `true`, will render pages which redirect to the /// real location of an item. This is used to allow external links to /// publicly reused items to redirect to the right location. - crate render_redirect_pages: bool, - crate shared: Rc>, + pub(super) render_redirect_pages: bool, + /// Shared mutable state. + /// + /// Issue for improving the situation: [#82381][] + /// + /// [#82381]: https://github.com/rust-lang/rust/issues/82381 + pub(super) shared: Rc>, /// The [`Cache`] used during rendering. /// /// Ideally the cache would be in [`SharedContext`], but it's mutated @@ -62,7 +67,7 @@ crate struct Context<'tcx> { /// It's immutable once in `Context`, so it's not as bad that it's not in /// `SharedContext`. // FIXME: move `cache` to `SharedContext` - crate cache: Rc, + pub(super) cache: Rc, } impl<'tcx> Context<'tcx> { diff --git a/src/librustdoc/html/render/mod.rs b/src/librustdoc/html/render/mod.rs index 3bb13d29992..c074b789e74 100644 --- a/src/librustdoc/html/render/mod.rs +++ b/src/librustdoc/html/render/mod.rs @@ -81,6 +81,7 @@ crate fn ensure_trailing_slash(v: &str) -> impl fmt::Display + '_ { }) } +/// Shared mutable state used in [`Context`] and elsewhere. crate struct SharedContext<'tcx> { crate tcx: TyCtxt<'tcx>, /// The path to the crate root source minus the file name. @@ -96,16 +97,16 @@ crate struct SharedContext<'tcx> { /// The local file sources we've emitted and their respective url-paths. crate local_sources: FxHashMap, /// Whether the collapsed pass ran - crate collapsed: bool, + collapsed: bool, /// The base-URL of the issue tracker for when an item has been tagged with /// an issue number. - crate issue_tracker_base_url: Option, + issue_tracker_base_url: Option, /// The directories that have already been created in this doc run. Used to reduce the number /// of spurious `create_dir_all` calls. - crate created_dirs: RefCell>, + created_dirs: RefCell>, /// This flag indicates whether listings of modules (in the side bar and documentation itself) /// should be ordered alphabetically or in order of appearance (in the source code). - crate sort_modules_alphabetically: bool, + sort_modules_alphabetically: bool, /// Additional CSS files to be added to the generated docs. crate style_files: Vec, /// Suffix to be added on resource files (if suffix is "-v2" then "light.css" becomes @@ -118,7 +119,7 @@ crate struct SharedContext<'tcx> { crate fs: DocFS, /// The default edition used to parse doctests. crate edition: Edition, - crate codes: ErrorCodes, + codes: ErrorCodes, playground: Option, /// The map used to ensure all generated 'id=' attributes are unique. id_map: RefCell, @@ -128,11 +129,11 @@ crate struct SharedContext<'tcx> { all: RefCell, /// Storage for the errors produced while generating documentation so they /// can be printed together at the end. - crate errors: Receiver, + errors: Receiver, /// `None` by default, depends on the `generate-redirect-map` option flag. If this field is set /// to `Some(...)`, it'll store redirections and then generate a JSON file at the top level of /// the crate. - crate redirections: Option>>, + redirections: Option>>, } impl SharedContext<'_> { From 3bc879e76bb428d42831179829606dede0c84f94 Mon Sep 17 00:00:00 2001 From: Camelid Date: Sun, 21 Feb 2021 17:19:43 -0800 Subject: [PATCH 4/8] rustdoc: Add static size assertion for `Context` It's cloned a lot, so we don't want it to grow in size unexpectedly. Only run the assert on x86-64 since the size is architecture-dependent. --- src/librustdoc/html/render/context.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/librustdoc/html/render/context.rs b/src/librustdoc/html/render/context.rs index e8d323b9af1..50d160ccd82 100644 --- a/src/librustdoc/html/render/context.rs +++ b/src/librustdoc/html/render/context.rs @@ -70,6 +70,10 @@ crate struct Context<'tcx> { pub(super) cache: Rc, } +// `Context` is cloned a lot, so we don't want the size to grow unexpectedly. +#[cfg(target_arch = "x86_64")] +rustc_data_structures::static_assert_size!(Context<'_>, 72); + impl<'tcx> Context<'tcx> { pub(super) fn path(&self, filename: &str) -> PathBuf { // We use splitn vs Path::extension here because we might get a filename From 4c51a66d674c32b3d8eeebcd1ae276611e9f4c12 Mon Sep 17 00:00:00 2001 From: Camelid Date: Mon, 22 Feb 2021 15:43:43 -0800 Subject: [PATCH 5/8] Don't share `id_map` and `deref_id_map` All the tests passed, so it doesn't seem they need to be shared. Plus they should be item/page-specific. I'm not sure why they were shared before. I think the reason `id_map` worked as a shared value before is that it is cleared before rendering each item (in `render_item`). And then I'm guessing `deref_id_map` worked because it's a hashmap keyed by `DefId`, so there was no overlap (though I'm guessing we could have had issues in the future). Note that `id_map` currently still has to be cleared because otherwise child items would inherit the `id_map` of their parent. I'm hoping to figure out a way to stop cloning `Context`, but until then we have to reset `id_map`. --- src/librustdoc/html/render/context.rs | 21 +++++++++++++-------- src/librustdoc/html/render/mod.rs | 20 +++++++------------- 2 files changed, 20 insertions(+), 21 deletions(-) diff --git a/src/librustdoc/html/render/context.rs b/src/librustdoc/html/render/context.rs index 50d160ccd82..c2dfb64b2ab 100644 --- a/src/librustdoc/html/render/context.rs +++ b/src/librustdoc/html/render/context.rs @@ -6,7 +6,7 @@ use std::rc::Rc; use std::sync::mpsc::channel; use rustc_data_structures::fx::FxHashMap; -use rustc_hir::def_id::LOCAL_CRATE; +use rustc_hir::def_id::{DefId, LOCAL_CRATE}; use rustc_middle::ty::TyCtxt; use rustc_session::Session; use rustc_span::edition::Edition; @@ -30,7 +30,7 @@ use crate::formats::item_type::ItemType; use crate::formats::FormatRenderer; use crate::html::escape::Escape; use crate::html::format::Buffer; -use crate::html::markdown::{self, plain_text_summary, ErrorCodes}; +use crate::html::markdown::{self, plain_text_summary, ErrorCodes, IdMap}; use crate::html::{layout, sources}; /// Major driving force in all rustdoc rendering. This contains information @@ -52,6 +52,11 @@ crate struct Context<'tcx> { /// real location of an item. This is used to allow external links to /// publicly reused items to redirect to the right location. pub(super) render_redirect_pages: bool, + /// The map used to ensure all generated 'id=' attributes are unique. + pub(super) id_map: RefCell, + /// Tracks section IDs for `Deref` targets so they match in both the main + /// body and the sidebar. + pub(super) deref_id_map: RefCell>, /// Shared mutable state. /// /// Issue for improving the situation: [#82381][] @@ -72,7 +77,7 @@ crate struct Context<'tcx> { // `Context` is cloned a lot, so we don't want the size to grow unexpectedly. #[cfg(target_arch = "x86_64")] -rustc_data_structures::static_assert_size!(Context<'_>, 72); +rustc_data_structures::static_assert_size!(Context<'_>, 152); impl<'tcx> Context<'tcx> { pub(super) fn path(&self, filename: &str) -> PathBuf { @@ -95,7 +100,7 @@ impl<'tcx> Context<'tcx> { } pub(super) fn derive_id(&self, id: String) -> String { - let mut map = self.shared.id_map.borrow_mut(); + let mut map = self.id_map.borrow_mut(); map.derive(id) } @@ -153,8 +158,8 @@ impl<'tcx> Context<'tcx> { }; { - self.shared.id_map.borrow_mut().reset(); - self.shared.id_map.borrow_mut().populate(&INITIAL_IDS); + self.id_map.borrow_mut().reset(); + self.id_map.borrow_mut().populate(&INITIAL_IDS); } if !self.render_redirect_pages { @@ -387,8 +392,6 @@ impl<'tcx> FormatRenderer<'tcx> for Context<'tcx> { edition, codes: ErrorCodes::from(unstable_features.is_nightly_build()), playground, - id_map: RefCell::new(id_map), - deref_id_map: RefCell::new(FxHashMap::default()), all: RefCell::new(AllTypes::new()), errors: receiver, redirections: if generate_redirect_map { Some(Default::default()) } else { None }, @@ -418,6 +421,8 @@ impl<'tcx> FormatRenderer<'tcx> for Context<'tcx> { current: Vec::new(), dst, render_redirect_pages: false, + id_map: RefCell::new(id_map), + deref_id_map: RefCell::new(FxHashMap::default()), shared: Rc::new(scx), cache: Rc::new(cache), }; diff --git a/src/librustdoc/html/render/mod.rs b/src/librustdoc/html/render/mod.rs index c074b789e74..339c3ec15f6 100644 --- a/src/librustdoc/html/render/mod.rs +++ b/src/librustdoc/html/render/mod.rs @@ -70,7 +70,7 @@ use crate::html::format::{ PrintWithSpace, WhereClause, }; use crate::html::layout; -use crate::html::markdown::{self, ErrorCodes, IdMap, Markdown, MarkdownHtml, MarkdownSummaryLine}; +use crate::html::markdown::{self, ErrorCodes, Markdown, MarkdownHtml, MarkdownSummaryLine}; /// A pair of name and its optional document. crate type NameDoc = (String, Option); @@ -121,11 +121,6 @@ crate struct SharedContext<'tcx> { crate edition: Edition, codes: ErrorCodes, playground: Option, - /// The map used to ensure all generated 'id=' attributes are unique. - id_map: RefCell, - /// Tracks section IDs for `Deref` targets so they match in both the main - /// body and the sidebar. - deref_id_map: RefCell>, all: RefCell, /// Storage for the errors produced while generating documentation so they /// can be printed together at the end. @@ -650,7 +645,7 @@ fn render_markdown( prefix: &str, is_hidden: bool, ) { - let mut ids = cx.shared.id_map.borrow_mut(); + let mut ids = cx.id_map.borrow_mut(); write!( w, "
{}{}
", @@ -810,7 +805,7 @@ fn short_item_info( if let Some(note) = note { let note = note.as_str(); - let mut ids = cx.shared.id_map.borrow_mut(); + let mut ids = cx.id_map.borrow_mut(); let html = MarkdownHtml( ¬e, &mut ids, @@ -849,7 +844,7 @@ fn short_item_info( message.push_str(&format!(" ({})", feature)); if let Some(unstable_reason) = reason { - let mut ids = cx.shared.id_map.borrow_mut(); + let mut ids = cx.id_map.borrow_mut(); message = format!( "
{}{}
", message, @@ -1189,8 +1184,7 @@ fn render_assoc_items( type_.print(cx.cache()) ))); debug!("Adding {} to deref id map", type_.print(cx.cache())); - cx.shared - .deref_id_map + cx.deref_id_map .borrow_mut() .insert(type_.def_id_full(cx.cache()).unwrap(), id.clone()); write!( @@ -1497,7 +1491,7 @@ fn render_impl( } if let Some(ref dox) = cx.shared.maybe_collapsed_doc_value(&i.impl_item) { - let mut ids = cx.shared.id_map.borrow_mut(); + let mut ids = cx.id_map.borrow_mut(); write!( w, "
{}
", @@ -2046,7 +2040,7 @@ fn sidebar_deref_methods(cx: &Context<'_>, out: &mut Buffer, impl_: &Impl, v: &V .flat_map(|i| get_methods(i.inner_impl(), true, &mut used_links, deref_mut, c)) .collect::>(); if !ret.is_empty() { - let deref_id_map = cx.shared.deref_id_map.borrow(); + let deref_id_map = cx.deref_id_map.borrow(); let id = deref_id_map .get(&real_target.def_id_full(cx.cache()).unwrap()) .expect("Deref section without derived id"); From ff39c46959b0c6926c0199f59f65af585e131e7d Mon Sep 17 00:00:00 2001 From: Camelid Date: Mon, 1 Mar 2021 16:30:58 -0800 Subject: [PATCH 6/8] Box some fields to reduce `Context` size Reduced from 152 bytes to 88 bytes. --- src/librustdoc/html/render/context.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/librustdoc/html/render/context.rs b/src/librustdoc/html/render/context.rs index c2dfb64b2ab..23c2a925105 100644 --- a/src/librustdoc/html/render/context.rs +++ b/src/librustdoc/html/render/context.rs @@ -53,10 +53,10 @@ crate struct Context<'tcx> { /// publicly reused items to redirect to the right location. pub(super) render_redirect_pages: bool, /// The map used to ensure all generated 'id=' attributes are unique. - pub(super) id_map: RefCell, + pub(super) id_map: Box>, /// Tracks section IDs for `Deref` targets so they match in both the main /// body and the sidebar. - pub(super) deref_id_map: RefCell>, + pub(super) deref_id_map: Box>>, /// Shared mutable state. /// /// Issue for improving the situation: [#82381][] @@ -77,7 +77,7 @@ crate struct Context<'tcx> { // `Context` is cloned a lot, so we don't want the size to grow unexpectedly. #[cfg(target_arch = "x86_64")] -rustc_data_structures::static_assert_size!(Context<'_>, 152); +rustc_data_structures::static_assert_size!(Context<'_>, 88); impl<'tcx> Context<'tcx> { pub(super) fn path(&self, filename: &str) -> PathBuf { @@ -421,8 +421,8 @@ impl<'tcx> FormatRenderer<'tcx> for Context<'tcx> { current: Vec::new(), dst, render_redirect_pages: false, - id_map: RefCell::new(id_map), - deref_id_map: RefCell::new(FxHashMap::default()), + id_map: Box::new(RefCell::new(id_map)), + deref_id_map: Box::new(RefCell::new(FxHashMap::default())), shared: Rc::new(scx), cache: Rc::new(cache), }; From c09d9d34f02c72b93da25ba27758db7d7cddb1f4 Mon Sep 17 00:00:00 2001 From: Camelid Date: Mon, 1 Mar 2021 19:12:03 -0800 Subject: [PATCH 7/8] Don't unnecessarily clone some fields in `Context` There was no need to clone `id_map` because it was reset before each item was rendered. `deref_id_map` was not reset, but it was keyed by `DefId` and thus was unlikely to have collisions (at least for now). Now we just clone the fields that need to be cloned, and instead create fresh versions of the others. --- src/librustdoc/formats/renderer.rs | 9 ++++++--- src/librustdoc/html/markdown.rs | 4 ---- src/librustdoc/html/markdown/tests.rs | 12 +++--------- src/librustdoc/html/render/context.rs | 21 +++++++++++++++------ src/librustdoc/json/mod.rs | 4 ++++ 5 files changed, 28 insertions(+), 22 deletions(-) diff --git a/src/librustdoc/formats/renderer.rs b/src/librustdoc/formats/renderer.rs index b779363e5c7..9b0d193a0bc 100644 --- a/src/librustdoc/formats/renderer.rs +++ b/src/librustdoc/formats/renderer.rs @@ -9,7 +9,7 @@ use crate::formats::cache::Cache; /// Allows for different backends to rustdoc to be used with the `run_format()` function. Each /// backend renderer has hooks for initialization, documenting an item, entering and exiting a /// module, and cleanup/finalizing output. -crate trait FormatRenderer<'tcx>: Clone { +crate trait FormatRenderer<'tcx>: Sized { /// Gives a description of the renderer. Used for performance profiling. fn descr() -> &'static str; @@ -23,6 +23,9 @@ crate trait FormatRenderer<'tcx>: Clone { tcx: TyCtxt<'tcx>, ) -> Result<(Self, clean::Crate), Error>; + /// Make a new renderer to render a child of the item currently being rendered. + fn make_child_renderer(&self) -> Self; + /// Renders a single non-module item. This means no recursive sub-item rendering is required. fn item(&mut self, item: clean::Item) -> Result<(), Error>; @@ -67,7 +70,7 @@ crate fn run_format<'tcx, T: FormatRenderer<'tcx>>( item.name = Some(krate.name); // Render the crate documentation - let mut work = vec![(format_renderer.clone(), item)]; + let mut work = vec![(format_renderer.make_child_renderer(), item)]; let unknown = rustc_span::Symbol::intern(""); while let Some((mut cx, item)) = work.pop() { @@ -87,7 +90,7 @@ crate fn run_format<'tcx, T: FormatRenderer<'tcx>>( }; for it in module.items { debug!("Adding {:?} to worklist", it.name); - work.push((cx.clone(), it)); + work.push((cx.make_child_renderer(), it)); } cx.mod_item_out(&name)?; diff --git a/src/librustdoc/html/markdown.rs b/src/librustdoc/html/markdown.rs index f8ca259fb9a..ccc51c243ad 100644 --- a/src/librustdoc/html/markdown.rs +++ b/src/librustdoc/html/markdown.rs @@ -1373,10 +1373,6 @@ impl IdMap { } } - crate fn reset(&mut self) { - self.map = init_id_map(); - } - crate fn derive + ToString>(&mut self, candidate: S) -> String { let id = match self.map.get_mut(candidate.as_ref()) { None => candidate.to_string(), diff --git a/src/librustdoc/html/markdown/tests.rs b/src/librustdoc/html/markdown/tests.rs index 59ca841715c..e016d2c3ab0 100644 --- a/src/librustdoc/html/markdown/tests.rs +++ b/src/librustdoc/html/markdown/tests.rs @@ -38,15 +38,9 @@ fn test_unique_id() { "assoc_type.Item-1", ]; - let map = RefCell::new(IdMap::new()); - let test = || { - let mut map = map.borrow_mut(); - let actual: Vec = input.iter().map(|s| map.derive(s.to_string())).collect(); - assert_eq!(&actual[..], expected); - }; - test(); - map.borrow_mut().reset(); - test(); + let mut map = IdMap::new(); + let actual: Vec = input.iter().map(|s| map.derive(s.to_string())).collect(); + assert_eq!(&actual[..], expected); } #[test] diff --git a/src/librustdoc/html/render/context.rs b/src/librustdoc/html/render/context.rs index 23c2a925105..41d4aef7c7a 100644 --- a/src/librustdoc/html/render/context.rs +++ b/src/librustdoc/html/render/context.rs @@ -40,7 +40,6 @@ use crate::html::{layout, sources}; /// It is intended that this context is a lightweight object which can be fairly /// easily cloned because it is cloned per work-job (about once per item in the /// rustdoc tree). -#[derive(Clone)] crate struct Context<'tcx> { /// Current hierarchy of components leading down to what's currently being /// rendered @@ -157,11 +156,6 @@ impl<'tcx> Context<'tcx> { static_extra_scripts: &[], }; - { - self.id_map.borrow_mut().reset(); - self.id_map.borrow_mut().populate(&INITIAL_IDS); - } - if !self.render_redirect_pages { layout::render( &self.shared.layout, @@ -436,6 +430,21 @@ impl<'tcx> FormatRenderer<'tcx> for Context<'tcx> { Ok((cx, krate)) } + fn make_child_renderer(&self) -> Self { + let mut id_map = IdMap::new(); + id_map.populate(&INITIAL_IDS); + + Self { + current: self.current.clone(), + dst: self.dst.clone(), + render_redirect_pages: self.render_redirect_pages, + id_map: Box::new(RefCell::new(id_map)), + deref_id_map: Box::new(RefCell::new(FxHashMap::default())), + shared: Rc::clone(&self.shared), + cache: Rc::clone(&self.cache), + } + } + fn after_krate( &mut self, krate: &clean::Crate, diff --git a/src/librustdoc/json/mod.rs b/src/librustdoc/json/mod.rs index e6edf33d23c..406c57826a0 100644 --- a/src/librustdoc/json/mod.rs +++ b/src/librustdoc/json/mod.rs @@ -149,6 +149,10 @@ impl<'tcx> FormatRenderer<'tcx> for JsonRenderer<'tcx> { )) } + fn make_child_renderer(&self) -> Self { + self.clone() + } + /// Inserts an item into the index. This should be used rather than directly calling insert on /// the hashmap because certain items (traits and types) need to have their mappings for trait /// implementations filled out before they're inserted. From 5b7409797555b8fcfb50dc92fcda9bd1298d70c4 Mon Sep 17 00:00:00 2001 From: Camelid Date: Mon, 1 Mar 2021 20:06:49 -0800 Subject: [PATCH 8/8] Undo addition of boxes I don't think the boxing helped performance, in fact I think it potentially made it worse. The data was still being copied, but now it was through a pointer. Thinking about it more, I think boxing might only help when you're passing a big object around by value all the time, rather than the slowdown being that you're cloning it. --- src/librustdoc/html/markdown/tests.rs | 1 - src/librustdoc/html/render/context.rs | 14 +++++++------- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/librustdoc/html/markdown/tests.rs b/src/librustdoc/html/markdown/tests.rs index e016d2c3ab0..e2ce9ad23f4 100644 --- a/src/librustdoc/html/markdown/tests.rs +++ b/src/librustdoc/html/markdown/tests.rs @@ -1,7 +1,6 @@ use super::{plain_text_summary, short_markdown_summary}; use super::{ErrorCodes, IdMap, Ignore, LangString, Markdown, MarkdownHtml}; use rustc_span::edition::{Edition, DEFAULT_EDITION}; -use std::cell::RefCell; #[test] fn test_unique_id() { diff --git a/src/librustdoc/html/render/context.rs b/src/librustdoc/html/render/context.rs index 41d4aef7c7a..864fbccbcc4 100644 --- a/src/librustdoc/html/render/context.rs +++ b/src/librustdoc/html/render/context.rs @@ -52,10 +52,10 @@ crate struct Context<'tcx> { /// publicly reused items to redirect to the right location. pub(super) render_redirect_pages: bool, /// The map used to ensure all generated 'id=' attributes are unique. - pub(super) id_map: Box>, + pub(super) id_map: RefCell, /// Tracks section IDs for `Deref` targets so they match in both the main /// body and the sidebar. - pub(super) deref_id_map: Box>>, + pub(super) deref_id_map: RefCell>, /// Shared mutable state. /// /// Issue for improving the situation: [#82381][] @@ -76,7 +76,7 @@ crate struct Context<'tcx> { // `Context` is cloned a lot, so we don't want the size to grow unexpectedly. #[cfg(target_arch = "x86_64")] -rustc_data_structures::static_assert_size!(Context<'_>, 88); +rustc_data_structures::static_assert_size!(Context<'_>, 152); impl<'tcx> Context<'tcx> { pub(super) fn path(&self, filename: &str) -> PathBuf { @@ -415,8 +415,8 @@ impl<'tcx> FormatRenderer<'tcx> for Context<'tcx> { current: Vec::new(), dst, render_redirect_pages: false, - id_map: Box::new(RefCell::new(id_map)), - deref_id_map: Box::new(RefCell::new(FxHashMap::default())), + id_map: RefCell::new(id_map), + deref_id_map: RefCell::new(FxHashMap::default()), shared: Rc::new(scx), cache: Rc::new(cache), }; @@ -438,8 +438,8 @@ impl<'tcx> FormatRenderer<'tcx> for Context<'tcx> { current: self.current.clone(), dst: self.dst.clone(), render_redirect_pages: self.render_redirect_pages, - id_map: Box::new(RefCell::new(id_map)), - deref_id_map: Box::new(RefCell::new(FxHashMap::default())), + id_map: RefCell::new(id_map), + deref_id_map: RefCell::new(FxHashMap::default()), shared: Rc::clone(&self.shared), cache: Rc::clone(&self.cache), }