From bea81a37397df49bde21b69541fb87664d499538 Mon Sep 17 00:00:00 2001 From: Camelid Date: Sun, 21 Feb 2021 14:21:43 -0800 Subject: [PATCH 1/7] 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/mod.rs | 60 ++++++++++++++++++------------- 1 file changed, 35 insertions(+), 25 deletions(-) diff --git a/src/librustdoc/html/render/mod.rs b/src/librustdoc/html/render/mod.rs index 7ca355ed11cc7..1d285340f047c 100644 --- a/src/librustdoc/html/render/mod.rs +++ b/src/librustdoc/html/render/mod.rs @@ -111,16 +111,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, - /// The map used to ensure all generated 'id=' attributes are unique. - id_map: Rc>, - /// Tracks section IDs for `Deref` targets so they match in both the main - /// body and the sidebar. - 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, } @@ -163,6 +163,15 @@ 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, } impl<'tcx> Context<'tcx> { @@ -478,6 +487,10 @@ 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, }; // Add the default themes to the `Vec` of stylepaths @@ -504,11 +517,7 @@ 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), }; @@ -558,7 +567,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, @@ -591,7 +600,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 { @@ -670,7 +679,7 @@ 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). @@ -1517,7 +1526,7 @@ fn settings(root_path: &str, suffix: &str, themes: &[StylePath]) -> Result { 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) } @@ -1572,8 +1581,8 @@ impl Context<'_> { }; { - 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 { @@ -1841,7 +1850,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, "
{}{}
", @@ -2319,7 +2328,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, @@ -2358,7 +2367,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, @@ -3513,7 +3522,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!( @@ -3819,7 +3829,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, "
{}
", @@ -4448,7 +4458,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 894fdf91fa6044e6b889372cd00eb06eebda60d6 Mon Sep 17 00:00:00 2001 From: Camelid Date: Sun, 21 Feb 2021 14:25:21 -0800 Subject: [PATCH 2/7] 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/mod.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/librustdoc/html/render/mod.rs b/src/librustdoc/html/render/mod.rs index 1d285340f047c..11186614c976d 100644 --- a/src/librustdoc/html/render/mod.rs +++ b/src/librustdoc/html/render/mod.rs @@ -44,7 +44,6 @@ use std::rc::Rc; use std::str; use std::string::ToString; use std::sync::mpsc::{channel, Receiver}; -use std::sync::Arc; use itertools::Itertools; use rustc_ast_pretty::pprust; @@ -111,7 +110,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 @@ -517,16 +516,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)) } @@ -599,7 +598,7 @@ impl<'tcx> FormatRenderer<'tcx> for Context<'tcx> { self.shared.fs.write(&settings_file, v.as_bytes())?; // 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 12a886f8a4dd214f5d34983a47e4a08ae4a657af Mon Sep 17 00:00:00 2001 From: Camelid Date: Sun, 21 Feb 2021 14:35:15 -0800 Subject: [PATCH 3/7] rustdoc: Make a bunch of fields private --- src/librustdoc/html/render/mod.rs | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/src/librustdoc/html/render/mod.rs b/src/librustdoc/html/render/mod.rs index 11186614c976d..42fbe6c4af760 100644 --- a/src/librustdoc/html/render/mod.rs +++ b/src/librustdoc/html/render/mod.rs @@ -102,15 +102,16 @@ crate fn ensure_trailing_slash(v: &str) -> impl fmt::Display + '_ { crate struct Context<'tcx> { /// Current hierarchy of components leading down to what's currently being /// rendered - crate current: Vec, + 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, + 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>, + render_redirect_pages: bool, + /// Shared mutable state. We should probably redesign this. + shared: Rc>, /// The [`Cache`] used during rendering. /// /// Ideally the cache would be in [`SharedContext`], but it's mutated @@ -120,9 +121,10 @@ 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, + cache: Rc, } +/// Shared mutable state in [`Context`]. We should probably redesign this. crate struct SharedContext<'tcx> { crate tcx: TyCtxt<'tcx>, /// The path to the crate root source minus the file name. @@ -138,16 +140,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 @@ -160,7 +162,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, @@ -170,7 +172,7 @@ 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, } impl<'tcx> Context<'tcx> { From a0e2fe08efb1331331ce3f448b389378bc511817 Mon Sep 17 00:00:00 2001 From: Camelid Date: Sun, 21 Feb 2021 17:19:43 -0800 Subject: [PATCH 4/7] rustdoc: Add static size assertion for `Context` It's cloned a lot, so we don't want it to grow in size unexpectedly. --- src/librustdoc/html/render/mod.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/librustdoc/html/render/mod.rs b/src/librustdoc/html/render/mod.rs index 42fbe6c4af760..c34289494e111 100644 --- a/src/librustdoc/html/render/mod.rs +++ b/src/librustdoc/html/render/mod.rs @@ -124,6 +124,9 @@ crate struct Context<'tcx> { cache: Rc, } +// `Context` is cloned a lot, so we don't want the size to grow unexpectedly. +rustc_data_structures::static_assert_size!(Context<'_>, 72); + /// Shared mutable state in [`Context`]. We should probably redesign this. crate struct SharedContext<'tcx> { crate tcx: TyCtxt<'tcx>, From 9580b56350f8ce83ddc845a9a60f4ab24f83404c Mon Sep 17 00:00:00 2001 From: Camelid Date: Sun, 21 Feb 2021 17:25:00 -0800 Subject: [PATCH 5/7] Add issue for removing shared mutable state --- src/librustdoc/html/render/mod.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/librustdoc/html/render/mod.rs b/src/librustdoc/html/render/mod.rs index c34289494e111..99aa3d6c123e6 100644 --- a/src/librustdoc/html/render/mod.rs +++ b/src/librustdoc/html/render/mod.rs @@ -110,7 +110,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. render_redirect_pages: bool, - /// Shared mutable state. We should probably redesign this. + /// Shared mutable state. + /// + /// Issue for improving the situation: [#82381][] + /// + /// [#82381]: https://github.com/rust-lang/rust/issues/82381 shared: Rc>, /// The [`Cache`] used during rendering. /// @@ -127,7 +131,7 @@ crate struct Context<'tcx> { // `Context` is cloned a lot, so we don't want the size to grow unexpectedly. rustc_data_structures::static_assert_size!(Context<'_>, 72); -/// Shared mutable state in [`Context`]. We should probably redesign this. +/// 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. From 1701d66065aa0f91f1fd4ce08f84b71df00df408 Mon Sep 17 00:00:00 2001 From: Camelid Date: Sun, 21 Feb 2021 18:57:18 -0800 Subject: [PATCH 6/7] Only run static assert on x86-64 The size is architecture-dependent. --- src/librustdoc/html/render/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/librustdoc/html/render/mod.rs b/src/librustdoc/html/render/mod.rs index 99aa3d6c123e6..d29ca8b76ba34 100644 --- a/src/librustdoc/html/render/mod.rs +++ b/src/librustdoc/html/render/mod.rs @@ -129,6 +129,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); /// Shared mutable state used in [`Context`] and elsewhere. From 3823a17a3b87ab55a99df97a95f05bb4d496d8f4 Mon Sep 17 00:00:00 2001 From: Camelid Date: Mon, 22 Feb 2021 15:43:43 -0800 Subject: [PATCH 7/7] 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/mod.rs | 36 +++++++++++++++---------------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/src/librustdoc/html/render/mod.rs b/src/librustdoc/html/render/mod.rs index d29ca8b76ba34..79cc0733b8b3c 100644 --- a/src/librustdoc/html/render/mod.rs +++ b/src/librustdoc/html/render/mod.rs @@ -110,6 +110,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. render_redirect_pages: bool, + /// 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>, /// Shared mutable state. /// /// Issue for improving the situation: [#82381][] @@ -130,7 +135,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); /// Shared mutable state used in [`Context`] and elsewhere. crate struct SharedContext<'tcx> { @@ -172,11 +177,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. @@ -496,8 +496,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, }; @@ -526,6 +524,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), }; @@ -1535,7 +1535,7 @@ fn settings(root_path: &str, suffix: &str, themes: &[StylePath]) -> Result { 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) } @@ -1590,8 +1590,9 @@ impl Context<'_> { }; { - self.shared.id_map.borrow_mut().reset(); - self.shared.id_map.borrow_mut().populate(&INITIAL_IDS); + // FIXME: remove this once `Context` is no longer cloned + self.id_map.borrow_mut().reset(); + self.id_map.borrow_mut().populate(&INITIAL_IDS); } if !self.render_redirect_pages { @@ -1859,7 +1860,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, "
{}{}
", @@ -2337,7 +2338,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, @@ -2376,7 +2377,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, @@ -3531,8 +3532,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!( @@ -3838,7 +3838,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, "
{}
", @@ -4467,7 +4467,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");