Auto merge of rust-lang#82356 - camelid:render-cleanup, r=GuillaumeGomez

bors · bors · commit 4b9f5cc4c10a · 2021-03-09T04:33:43.000Z
rustdoc: Cleanup `html::render::Context`

- Move most shared fields to `SharedContext` (except for `cache`, which
  isn't mutated anyway)
- Replace a use of `Arc` with `Rc`
- Make a bunch of fields private
- Add static size assertion for `Context`
- Don't share `id_map` and `deref_id_map`
diff --git 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("<unknown item>");
     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
@@ -1373,10 +1373,6 @@ impl IdMap {
         }
     }
 
-    crate fn reset(&mut self) {
-        self.map = init_id_map();
-    }
-
     crate fn derive<S: AsRef<str> + 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
@@ -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() {
@@ -38,15 +37,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<String> = 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<String> = 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
@@ -3,8 +3,7 @@ use std::collections::BTreeMap;
 use std::io;
 use std::path::PathBuf;
 use std::rc::Rc;
-use std::sync::mpsc::{channel, Receiver};
-use std::sync::Arc;
+use std::sync::mpsc::channel;
 
 use rustc_data_structures::fx::FxHashMap;
 use rustc_hir::def_id::{DefId, LOCAL_CRATE};
@@ -41,35 +40,44 @@ 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
-    crate current: Vec<String>,
+    pub(super) current: Vec<String>,
     /// 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,
-    /// `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<Rc<RefCell<FxHashMap<String, String>>>>,
+    pub(super) render_redirect_pages: bool,
     /// The map used to ensure all generated 'id=' attributes are unique.
-    pub(super) id_map: Rc<RefCell<IdMap>>,
+    pub(super) id_map: RefCell<IdMap>,
     /// Tracks section IDs for `Deref` targets so they match in both the main
     /// body and the sidebar.
-    pub(super) deref_id_map: Rc<RefCell<FxHashMap<DefId, String>>>,
-    crate shared: Arc<SharedContext<'tcx>>,
-    all: Rc<RefCell<AllTypes>>,
-    /// Storage for the errors produced while generating documentation so they
-    /// can be printed together at the end.
-    crate errors: Rc<Receiver<String>>,
-    crate cache: Rc<Cache>,
+    pub(super) deref_id_map: RefCell<FxHashMap<DefId, String>>,
+    /// Shared mutable state.
+    ///
+    /// Issue for improving the situation: [#82381][]
+    ///
+    /// [#82381]: https://github.com/rust-lang/rust/issues/82381
+    pub(super) shared: Rc<SharedContext<'tcx>>,
+    /// 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`
+    pub(super) cache: Rc<Cache>,
 }
 
+// `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);
+
 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
@@ -148,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,
@@ -169,7 +172,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 +386,9 @@ impl<'tcx> FormatRenderer<'tcx> for Context<'tcx> {
             edition,
             codes: ErrorCodes::from(unstable_features.is_nightly_build()),
             playground,
+            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,24 +415,36 @@ 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),
+            id_map: RefCell::new(id_map),
+            deref_id_map: RefCell::new(FxHashMap::default()),
+            shared: Rc::new(scx),
             cache: Rc::new(cache),
-            redirections: if generate_redirect_map { Some(Default::default()) } else { None },
         };
 
         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))
     }
 
+    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: RefCell::new(id_map),
+            deref_id_map: RefCell::new(FxHashMap::default()),
+            shared: Rc::clone(&self.shared),
+            cache: Rc::clone(&self.cache),
+        }
+    }
+
     fn after_krate(
         &mut self,
         krate: &clean::Crate,
@@ -464,7 +482,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 +512,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");
@@ -505,8 +523,8 @@ 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();
+        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"), ""))
         } else {
@@ -585,13 +603,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
@@ -42,6 +42,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;
@@ -81,6 +82,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 +98,16 @@ crate struct SharedContext<'tcx> {
     /// The local file sources we've emitted and their respective url-paths.
     crate local_sources: FxHashMap<PathBuf, String>,
     /// 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<String>,
+    issue_tracker_base_url: Option<String>,
     /// 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<FxHashSet<PathBuf>>,
+    created_dirs: RefCell<FxHashSet<PathBuf>>,
     /// 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<StylePath>,
     /// Suffix to be added on resource files (if suffix is "-v2" then "light.css" becomes
@@ -118,8 +120,16 @@ crate struct SharedContext<'tcx> {
     crate fs: DocFS,
     /// The default edition used to parse doctests.
     crate edition: Edition,
-    crate codes: ErrorCodes,
+    codes: ErrorCodes,
     playground: Option<markdown::Playground>,
+    all: RefCell<AllTypes>,
+    /// Storage for the errors produced while generating documentation so they
+    /// can be printed together at the end.
+    errors: Receiver<String>,
+    /// `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.
+    redirections: Option<RefCell<FxHashMap<String, String>>>,
 }
 
 impl SharedContext<'_> {
diff --git a/src/librustdoc/json/mod.rs b/src/librustdoc/json/mod.rs
@@ -148,6 +148,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.

Original file line number	Diff line number	Diff line change
`@@ -1373,10 +1373,6 @@ impl IdMap {`
`1373`	`1373`	`}`
`1374`	`1374`	`}`
`1375`	`1375`
`1376`		`- crate fn reset(&mut self) {`
`1377`		`- self.map = init_id_map();`
`1378`		`- }`
`1379`		`-`
`1380`	`1376`	`crate fn derive<S: AsRef<str> + ToString>(&mut self, candidate: S) -> String {`
`1381`	`1377`	`let id = match self.map.get_mut(candidate.as_ref()) {`
`1382`	`1378`	`None => candidate.to_string(),`