rustdoc: Get rid of shared mutable state in Context

[camelid]

Context currently has a .shared field, which is shared mutable state.
It would be nice if we could redesign the code such that we don't have shared
mutable state.

One idea I have for accomplishing this is passing &mut SharedContext in
wherever it's needed, instead of getting it out of Context.

[jyn514]

@camelid this is actually not too hard to change; the tricky bit is that SharedContext is stupid expensive to clone (it's almost 2kb, and it's cloned for each item we render).

(this includes the changes from #88222)

diff --git a/src/librustdoc/html/render/context.rs b/src/librustdoc/html/render/context.rs
index 733bedfdde9..3d29c6021c1 100644
--- a/src/librustdoc/html/render/context.rs
+++ b/src/librustdoc/html/render/context.rs
@@ -61,7 +61,7 @@
     /// Issue for improving the situation: [#82381][]
     ///
     /// [#82381]: https://github.com/rust-lang/rust/issues/82381
-    crate shared: Rc<SharedContext<'tcx>>,
+    crate shared: Box<SharedContext<'tcx>>,
     /// This flag indicates whether `[src]` links should be generated or not. If
     /// the source files are present in the html rendering, then this will be
     /// `true`.
@@ -73,6 +73,7 @@
 rustc_data_structures::static_assert_size!(Context<'_>, 104);
 
 /// Shared mutable state used in [`Context`] and elsewhere.
+// #[derive(Clone)] // TODO: bad
 crate struct SharedContext<'tcx> {
     crate tcx: TyCtxt<'tcx>,
     /// The path to the crate root source minus the file name.
@@ -503,7 +504,7 @@ fn init(
             dst,
             render_redirect_pages: false,
             id_map: RefCell::new(id_map),
-            shared: Rc::new(scx),
+            shared: Box::new(scx),
             include_sources,
         };
 
@@ -512,12 +513,12 @@ fn init(
         }
 
         // Build our search index
-        let index = build_index(&krate, &mut Rc::get_mut(&mut cx.shared).unwrap().cache, tcx);
+        let index = build_index(&krate, &mut cx.shared.cache, tcx);
 
         // Write shared runs within a flock; disable thread dispatching of IO temporarily.
-        Rc::get_mut(&mut cx.shared).unwrap().fs.set_sync_only(true);
+        cx.shared.fs.set_sync_only(true);
         write_shared(&cx, &krate, index, &md_opts)?;
-        Rc::get_mut(&mut cx.shared).unwrap().fs.set_sync_only(false);
+        cx.shared.fs.set_sync_only(false);
         Ok((cx, krate))
     }
 
@@ -527,7 +528,9 @@ fn make_child_renderer(&self) -> Self {
             dst: self.dst.clone(),
             render_redirect_pages: self.render_redirect_pages,
             id_map: RefCell::new(IdMap::new()),
-            shared: Rc::clone(&self.shared),
+            // TODO: this clone is stupid expensive
+            shared: todo!(),
+            // shared: self.shared.clone(),
             include_sources: self.include_sources,
         }
     }
@@ -608,7 +611,7 @@ fn after_krate(&mut self) -> Result<(), Error> {
         }
 
         // Flush pending errors.
-        Rc::get_mut(&mut self.shared).unwrap().fs.close();
+        self.shared.fs.close();
         let nb_errors =
             self.shared.errors.iter().map(|err| self.tcx().sess.struct_err(&err).emit()).count();
         if nb_errors > 0 {

I'm going to look into removing make_child_render.

[jyn514]

I'm going to look into removing make_child_render.

Ok, I found why this exists: otherwise the work order has to be sequenced, not parallel. In particular, mod_item_in and mod_item_out are very stateful with pushes and pops; if Context isn't cloned, then everything has to be iterated in preorder depth-first order, not whatever order we use today (postorder depth-first?).

[jyn514]

I think we might be able to avoid dst by using the original --output passed by the user and combining it with the parent module of the current item, which we can get from the tcx.

I don't understand what current is doing - is it just the list of parent modules and the current item? That seems like we could also get it from the tcx.

id_map is probably the hardest - I think we can reset it to an empty map in mod_item_in, rather than needing a separate make_child_renderer. Some code to do that that may or may not work:

diff --git a/src/librustdoc/html/render/context.rs b/src/librustdoc/html/render/context.rs
index 84bc1454f73..0df61a8b256 100644
--- a/src/librustdoc/html/render/context.rs
+++ b/src/librustdoc/html/render/context.rs
@@ -1,4 +1,4 @@
-use std::cell::RefCell;
+use std::cell::{RefCell, RefMut};
 use std::collections::BTreeMap;
 use std::error::Error as StdError;
 use std::io;
@@ -55,7 +55,10 @@
     /// 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<IdMap>,
+    ///
+    /// INVARIANT: there is always at least one map in this list (because `mod_item_out` is never
+    /// called without first calling `mod_item_in`).
+    pub(super) id_maps: Vec<RefCell<IdMap>>,
     /// Shared mutable state.
     ///
     /// Issue for improving the situation: [#82381][]
@@ -70,7 +73,7 @@
 
 // `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<'_>, 104);
+rustc_data_structures::static_assert_size!(Context<'_>, 88);
 
 /// Shared mutable state used in [`Context`] and elsewhere.
 crate struct SharedContext<'tcx> {
@@ -161,9 +164,12 @@ pub(super) fn sess(&self) -> &'tcx Session {
         &self.shared.tcx.sess
     }
 
+    pub(super) fn id_map(&self) -> RefMut<'_, IdMap> {
+        self.id_maps.last().unwrap().borrow_mut()
+    }
+
     pub(super) fn derive_id(&self, id: String) -> String {
-        let mut map = self.id_map.borrow_mut();
-        map.derive(id)
+        self.id_map().derive(id)
     }
 
     /// String representation of how to get back to the root path of the 'doc/'
@@ -502,7 +508,7 @@ fn init(
             current: Vec::new(),
             dst,
             render_redirect_pages: false,
-            id_map: RefCell::new(id_map),
+            id_maps: vec![RefCell::new(id_map)],
             shared: Rc::new(scx),
             include_sources,
         };
@@ -608,6 +614,7 @@ fn after_krate(&mut self) -> Result<(), Error> {
     }
 
     fn mod_item_in(&mut self, item: &clean::Item) -> Result<(), Error> {
+        self.id_maps.push(RefCell::new(IdMap::new()));
         // Stripped modules survive the rustdoc passes (i.e., `strip-private`)
         // if they contain impls for public types. These modules can also
         // contain items such as publicly re-exported structures.
@@ -653,6 +660,7 @@ fn mod_item_out(&mut self) -> Result<(), Error> {
         // Go back to where we were at
         self.dst.pop();
         self.current.pop();
+        self.id_maps.pop();
         Ok(())
     }
 
diff --git a/src/librustdoc/html/render/mod.rs b/src/librustdoc/html/render/mod.rs
index 7704abc9a72..31319df8086 100644
--- a/src/librustdoc/html/render/mod.rs
+++ b/src/librustdoc/html/render/mod.rs
@@ -485,14 +485,13 @@ fn document(w: &mut Buffer, cx: &Context<'_>, item: &clean::Item, parent: Option
 
 /// Render md_text as markdown.
 fn render_markdown(w: &mut Buffer, cx: &Context<'_>, md_text: &str, links: Vec<RenderedLink>) {
-    let mut ids = cx.id_map.borrow_mut();
     write!(
         w,
         "<div class=\"docblock\">{}</div>",
         Markdown(
             md_text,
             &links,
-            &mut ids,
+            &mut cx.id_map(),
             cx.shared.codes,
             cx.shared.edition(),
             &cx.shared.playground
@@ -622,7 +621,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.id_map();
             let html = MarkdownHtml(
                 &note,
                 &mut ids,
@@ -661,13 +660,12 @@ fn short_item_info(
         message.push_str(&format!(" ({})", feature));
 
         if let Some(unstable_reason) = reason {
-            let mut ids = cx.id_map.borrow_mut();
             message = format!(
                 "<details><summary>{}</summary>{}</details>",
                 message,
                 MarkdownHtml(
                     &unstable_reason.as_str(),
-                    &mut ids,
+                    &mut cx.id_map(),
                     error_codes,
                     cx.shared.edition(),
                     &cx.shared.playground,
@@ -1537,14 +1535,13 @@ fn render_default_items(
         }
 
         if let Some(ref dox) = cx.shared.maybe_collapsed_doc_value(&i.impl_item) {
-            let mut ids = cx.id_map.borrow_mut();
             write!(
                 w,
                 "<div class=\"docblock\">{}</div>",
                 Markdown(
                     &*dox,
                     &i.impl_item.links(cx),
-                    &mut ids,
+                    &mut cx.id_map(),
                     cx.shared.codes,
                     cx.shared.edition(),
                     &cx.shared.playground

[camelid]

I don't understand what current is doing - is it just the list of parent modules and the current item? That seems like we could also get it from the tcx.

I think so, but it's possible there's some differences between it and the TyCtxt def path (re-exports? doc(inline)? etc.).

[camelid]

I don't understand what current is doing - is it just the list of parent modules and the current item? That seems like we could also get it from the tcx.

I think getting it from Cache.paths might be a simpler route to go; I think it might handle re-exports for you.

[Wardenfar]

@rustbot claim