Skip to content

Stop cloning Context so much #133345

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Dec 2, 2024
Merged

Stop cloning Context so much #133345

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8047340
Stop cloning `Context` so much
GuillaumeGomez Nov 21, 2024
46afbc0
Split ID maps in two parts: the constant one and the updated one
GuillaumeGomez Nov 23, 2024
2e242f8
Store default ID map in a static
GuillaumeGomez Nov 25, 2024
5fa1653
Move `SharedContext` out of `Rc`
GuillaumeGomez Nov 25, 2024
5f9e716
Add documentation for new `FormatRenderer` trait items
GuillaumeGomez Nov 27, 2024
e60a7a4
Add `unreachable!` in new `FormatRenderer` methods for `JsonRenderer`…
GuillaumeGomez Nov 28, 2024
b492170
Rename `FormatRenderer::InfoType` into `ModuleData` and rename `Forma…
GuillaumeGomez Nov 28, 2024
69ed026
Fix link to `FormatRenderer` method
GuillaumeGomez Dec 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 63 additions & 33 deletions src/librustdoc/formats/renderer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use rustc_data_structures::profiling::SelfProfilerRef;
use rustc_middle::ty::TyCtxt;
use tracing::debug;

use crate::clean;
use crate::config::RenderOptions;
Expand All @@ -17,6 +17,18 @@ pub(crate) trait FormatRenderer<'tcx>: Sized {
///
/// This is true for html, and false for json. See #80664
const RUN_ON_MODULE: bool;
/// This associated type is the type where the current module information is stored.
///
/// For each module, we go through their items by calling for each item:
///
/// 1. save_module_data
/// 2. item
/// 3. set_back_info
///
/// However,the `item` method might update information in `self` (for example if the child is
/// a module). To prevent it to impact the other children of the current module, we need to
/// reset the information between each call to `item` by using `set_back_info`.
type ModuleData;

/// Sets up any state required for the renderer. When this is called the cache has already been
/// populated.
Expand All @@ -27,8 +39,20 @@ pub(crate) trait FormatRenderer<'tcx>: Sized {
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;
/// This method is called right before call [`Self::item`]. This method returns a type
/// containing information that needs to be reset after the [`Self::item`] method has been
/// called with the [`Self::set_back_info`] method.
///
/// In short it goes like this:
///
/// ```ignore (not valid code)
/// let reset_data = type.save_module_data();
/// type.item(item)?;
/// type.set_back_info(reset_data);
/// ```
fn save_module_data(&mut self) -> Self::ModuleData;
/// Used to reset current module's information.
fn set_back_info(&mut self, info: Self::ModuleData);

/// Renders a single non-module item. This means no recursive sub-item rendering is required.
fn item(&mut self, item: clean::Item) -> Result<(), Error>;
Expand All @@ -47,6 +71,40 @@ pub(crate) trait FormatRenderer<'tcx>: Sized {
fn cache(&self) -> &Cache;
}

fn run_format_inner<'tcx, T: FormatRenderer<'tcx>>(
cx: &mut T,
item: clean::Item,
prof: &SelfProfilerRef,
) -> Result<(), Error> {
if item.is_mod() && T::RUN_ON_MODULE {
// modules are special because they add a namespace. We also need to
// recurse into the items of the module as well.
let _timer =
prof.generic_activity_with_arg("render_mod_item", item.name.unwrap().to_string());

cx.mod_item_in(&item)?;
let (clean::StrippedItem(box clean::ModuleItem(module)) | clean::ModuleItem(module)) =
item.inner.kind
else {
unreachable!()
};
for it in module.items {
let info = cx.save_module_data();
run_format_inner(cx, it, prof)?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old version used a heap-allocated stack, and this version instead uses the call stack.

I think that's fine, as long as you call stacker?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering we recurse on modules (other items, they stop at one level), I think we're on the safe side. Do you see a case where it could be an issue?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if it's going to be a problem. I just noticed that the old didn't do this.

If you don't think it's a problem, then we can go ahead with it. There might be other spots where rustdoc directly recurses on modules anyway.

cx.set_back_info(info);
}

cx.mod_item_out()?;
// FIXME: checking `item.name.is_some()` is very implicit and leads to lots of special
// cases. Use an explicit match instead.
} else if let Some(item_name) = item.name
&& !item.is_extern_crate()
{
prof.generic_activity_with_arg("render_item", item_name.as_str()).run(|| cx.item(item))?;
}
Ok(())
}

/// Main method for rendering a crate.
pub(crate) fn run_format<'tcx, T: FormatRenderer<'tcx>>(
krate: clean::Crate,
Expand All @@ -66,36 +124,8 @@ pub(crate) fn run_format<'tcx, T: FormatRenderer<'tcx>>(
}

// Render the crate documentation
let mut work = vec![(format_renderer.make_child_renderer(), krate.module)];

while let Some((mut cx, item)) = work.pop() {
if item.is_mod() && T::RUN_ON_MODULE {
// modules are special because they add a namespace. We also need to
// recurse into the items of the module as well.
let _timer =
prof.generic_activity_with_arg("render_mod_item", item.name.unwrap().to_string());

cx.mod_item_in(&item)?;
let (clean::StrippedItem(box clean::ModuleItem(module)) | clean::ModuleItem(module)) =
item.inner.kind
else {
unreachable!()
};
for it in module.items {
debug!("Adding {:?} to worklist", it.name);
work.push((cx.make_child_renderer(), it));
}

cx.mod_item_out()?;
// FIXME: checking `item.name.is_some()` is very implicit and leads to lots of special
// cases. Use an explicit match instead.
} else if let Some(item_name) = item.name
&& !item.is_extern_crate()
{
prof.generic_activity_with_arg("render_item", item_name.as_str())
.run(|| cx.item(item))?;
}
}
run_format_inner(&mut format_renderer, krate.module, prof)?;

prof.verbose_generic_activity_with_arg("renderer_after_krate", T::descr())
.run(|| format_renderer.after_krate())
}
117 changes: 66 additions & 51 deletions src/librustdoc/html/markdown.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ use std::sync::OnceLock;
use pulldown_cmark::{
BrokenLink, CodeBlockKind, CowStr, Event, LinkType, Options, Parser, Tag, TagEnd, html,
};
use rustc_data_structures::fx::FxHashMap;
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_errors::{Diag, DiagMessage};
use rustc_hir::def_id::LocalDefId;
use rustc_middle::ty::TyCtxt;
Expand Down Expand Up @@ -1886,71 +1886,81 @@ pub struct IdMap {
existing_footnotes: usize,
}

// The map is pre-initialized and cloned each time to avoid reinitializing it repeatedly.
static DEFAULT_ID_MAP: OnceLock<FxHashMap<Cow<'static, str>, usize>> = OnceLock::new();
// The map is pre-initialized and then can be used as is to prevent cloning it for each item
// (in the sidebar rendering).
static DEFAULT_ID_MAP: OnceLock<FxHashSet<&'static str>> = OnceLock::new();

fn init_id_map() -> FxHashMap<Cow<'static, str>, usize> {
let mut map = FxHashMap::default();
fn init_id_map() -> FxHashSet<&'static str> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does performance compare if this is a function that returns bool?

fn is_builtin_id(id: &str) -> bool {
    match id {
        // This is the list of IDs used in JavaScript.
		| "help"
		| "settings"
		| "not-displayed"
		| "alternative-display"
		| "search"
		| "crate-search"
		| "crate-search-div"
		// This is the list of IDs used in HTML generated in Rust (including the ones
		// used in tera template files).
		| "themeStyle"
		| "settings-menu"
		| "help-button"
		| "sidebar-button"
		| "main-content"
		| "toggle-all-docs"
		| "all-types"
		| "default-settings"
		| "sidebar-vars"
		| "copy-path"
		| "rustdoc-toc"
		| "rustdoc-modnav"
		// This is the list of IDs used by rustdoc sections (but still generated by
		// rustdoc).
		| "fields"
		| "variants"
		| "implementors-list"
		| "synthetic-implementors-list"
		| "foreign-impls"
		| "implementations"
		| "trait-implementations"
		| "synthetic-implementations"
		| "blanket-implementations"
		| "required-associated-types"
		| "provided-associated-types"
		| "provided-associated-consts"
		| "required-associated-consts"
		| "required-methods"
		| "provided-methods"
		| "dyn-compatibility"
		| "implementors"
		| "synthetic-implementors"
		| "implementations-list"
		| "trait-implementations-list"
		| "synthetic-implementations-list"
		| "blanket-implementations-list"
		| "deref-methods"
		| "layout"
		| "aliased-type"
		=> true,
		_ => false,
    }
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

matches!. Let's find out in the next set of ideas I have as follow-up?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine. It can be tried as a follow-up PR.

let mut map = FxHashSet::default();
// This is the list of IDs used in JavaScript.
map.insert("help".into(), 1);
map.insert("settings".into(), 1);
map.insert("not-displayed".into(), 1);
map.insert("alternative-display".into(), 1);
map.insert("search".into(), 1);
map.insert("crate-search".into(), 1);
map.insert("crate-search-div".into(), 1);
map.insert("help");
map.insert("settings");
map.insert("not-displayed");
map.insert("alternative-display");
map.insert("search");
map.insert("crate-search");
map.insert("crate-search-div");
// This is the list of IDs used in HTML generated in Rust (including the ones
// used in tera template files).
map.insert("themeStyle".into(), 1);
map.insert("settings-menu".into(), 1);
map.insert("help-button".into(), 1);
map.insert("sidebar-button".into(), 1);
map.insert("main-content".into(), 1);
map.insert("toggle-all-docs".into(), 1);
map.insert("all-types".into(), 1);
map.insert("default-settings".into(), 1);
map.insert("sidebar-vars".into(), 1);
map.insert("copy-path".into(), 1);
map.insert("rustdoc-toc".into(), 1);
map.insert("rustdoc-modnav".into(), 1);
map.insert("themeStyle");
map.insert("settings-menu");
map.insert("help-button");
map.insert("sidebar-button");
map.insert("main-content");
map.insert("toggle-all-docs");
map.insert("all-types");
map.insert("default-settings");
map.insert("sidebar-vars");
map.insert("copy-path");
map.insert("rustdoc-toc");
map.insert("rustdoc-modnav");
// This is the list of IDs used by rustdoc sections (but still generated by
// rustdoc).
map.insert("fields".into(), 1);
map.insert("variants".into(), 1);
map.insert("implementors-list".into(), 1);
map.insert("synthetic-implementors-list".into(), 1);
map.insert("foreign-impls".into(), 1);
map.insert("implementations".into(), 1);
map.insert("trait-implementations".into(), 1);
map.insert("synthetic-implementations".into(), 1);
map.insert("blanket-implementations".into(), 1);
map.insert("required-associated-types".into(), 1);
map.insert("provided-associated-types".into(), 1);
map.insert("provided-associated-consts".into(), 1);
map.insert("required-associated-consts".into(), 1);
map.insert("required-methods".into(), 1);
map.insert("provided-methods".into(), 1);
map.insert("dyn-compatibility".into(), 1);
map.insert("implementors".into(), 1);
map.insert("synthetic-implementors".into(), 1);
map.insert("implementations-list".into(), 1);
map.insert("trait-implementations-list".into(), 1);
map.insert("synthetic-implementations-list".into(), 1);
map.insert("blanket-implementations-list".into(), 1);
map.insert("deref-methods".into(), 1);
map.insert("layout".into(), 1);
map.insert("aliased-type".into(), 1);
map.insert("fields");
map.insert("variants");
map.insert("implementors-list");
map.insert("synthetic-implementors-list");
map.insert("foreign-impls");
map.insert("implementations");
map.insert("trait-implementations");
map.insert("synthetic-implementations");
map.insert("blanket-implementations");
map.insert("required-associated-types");
map.insert("provided-associated-types");
map.insert("provided-associated-consts");
map.insert("required-associated-consts");
map.insert("required-methods");
map.insert("provided-methods");
map.insert("dyn-compatibility");
map.insert("implementors");
map.insert("synthetic-implementors");
map.insert("implementations-list");
map.insert("trait-implementations-list");
map.insert("synthetic-implementations-list");
map.insert("blanket-implementations-list");
map.insert("deref-methods");
map.insert("layout");
map.insert("aliased-type");
map
}

impl IdMap {
pub fn new() -> Self {
IdMap { map: DEFAULT_ID_MAP.get_or_init(init_id_map).clone(), existing_footnotes: 0 }
IdMap { map: FxHashMap::default(), existing_footnotes: 0 }
}

pub(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(),
None => {
let candidate = candidate.to_string();
if DEFAULT_ID_MAP.get_or_init(init_id_map).contains(candidate.as_str()) {
let id = format!("{}-{}", candidate, 1);
self.map.insert(candidate.into(), 2);
id
} else {
candidate
}
}
Some(a) => {
let id = format!("{}-{}", candidate.as_ref(), *a);
*a += 1;
Expand All @@ -1970,4 +1980,9 @@ impl IdMap {
closure(self, &mut existing_footnotes);
self.existing_footnotes = existing_footnotes;
}

pub(crate) fn clear(&mut self) {
self.map.clear();
self.existing_footnotes = 0;
}
}
Loading
Loading