Skip to content

rustdoc: reduce number of copies when using parallel IO #88219

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 1 commit into from
Sep 16, 2021
Merged

rustdoc: reduce number of copies when using parallel IO #88219

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 6 additions & 6 deletions src/librustdoc/docfs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

use std::fs;
use std::io;
use std::path::Path;
use std::path::{Path, PathBuf};
use std::string::ToString;
use std::sync::mpsc::Sender;

Expand Down Expand Up @@ -55,17 +55,17 @@ impl DocFS {
fs::create_dir_all(path)
}

crate fn write<P, C, E>(&self, path: P, contents: C) -> Result<(), E>
crate fn write<E>(
&self,
path: PathBuf,
contents: impl 'static + Send + AsRef<[u8]>,
) -> Result<(), E>
where
P: AsRef<Path>,
C: AsRef<[u8]>,
E: PathError,
{
if !self.sync_only && cfg!(windows) {
// A possible future enhancement after more detailed profiling would
// be to create the file sync so errors are reported eagerly.
let path = path.as_ref().to_path_buf();
let contents = contents.as_ref().to_vec();
let sender = self.errors.clone().expect("can't write after closing");
rayon::spawn(move || {
Copy link
Member

Choose a reason for hiding this comment

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

Rayon isn't designed for use with blocking IO (https://docs.rs/rayon/1.5.1/rayon/fn.join.html#warning-about-blocking-io). I think it would be a good idea to change this to use a threadpool.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahhh, that explains why it was so much slower locally. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Wait, I'm confused - that warning is specifically on join, and spawn says it already uses a threadpool:

Fires off a task into the Rayon threadpool in the “static” or “global” scope.

fs::write(&path, contents).unwrap_or_else(|e| {
Expand Down
14 changes: 7 additions & 7 deletions src/librustdoc/html/render/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,7 +583,7 @@ impl<'tcx> FormatRenderer<'tcx> for Context<'tcx> {
|buf: &mut Buffer| all.print(buf),
&self.shared.style_files,
);
self.shared.fs.write(final_file, v.as_bytes())?;
self.shared.fs.write(final_file, v)?;

// Generating settings page.
page.title = "Rustdoc settings";
Expand All @@ -605,14 +605,14 @@ impl<'tcx> FormatRenderer<'tcx> for Context<'tcx> {
)?,
&style_files,
);
self.shared.fs.write(&settings_file, v.as_bytes())?;
self.shared.fs.write(settings_file, v)?;
if let Some(ref redirections) = self.shared.redirections {
if !redirections.borrow().is_empty() {
let redirect_map_path =
self.dst.join(&*crate_name.as_str()).join("redirect-map.json");
let paths = serde_json::to_string(&*redirections.borrow()).unwrap();
self.shared.ensure_dir(&self.dst.join(&*crate_name.as_str()))?;
self.shared.fs.write(&redirect_map_path, paths.as_bytes())?;
self.shared.fs.write(redirect_map_path, paths)?;
}
}

Expand Down Expand Up @@ -650,7 +650,7 @@ impl<'tcx> FormatRenderer<'tcx> for Context<'tcx> {
if !buf.is_empty() {
self.shared.ensure_dir(&self.dst)?;
let joint_dst = self.dst.join("index.html");
scx.fs.write(&joint_dst, buf.as_bytes())?;
scx.fs.write(joint_dst, buf)?;
}

// Render sidebar-items.js used throughout this module.
Expand All @@ -662,7 +662,7 @@ impl<'tcx> FormatRenderer<'tcx> for Context<'tcx> {
let items = self.build_sidebar_items(module);
let js_dst = self.dst.join("sidebar-items.js");
let v = format!("initSidebarItems({});", serde_json::to_string(&items).unwrap());
scx.fs.write(&js_dst, &v)?;
scx.fs.write(js_dst, v)?;
}
Ok(())
}
Expand Down Expand Up @@ -696,7 +696,7 @@ impl<'tcx> FormatRenderer<'tcx> for Context<'tcx> {
let file_name = &item_path(item_type, &name.as_str());
self.shared.ensure_dir(&self.dst)?;
let joint_dst = self.dst.join(file_name);
self.shared.fs.write(&joint_dst, buf.as_bytes())?;
self.shared.fs.write(joint_dst, buf)?;

if !self.render_redirect_pages {
self.shared.all.borrow_mut().append(full_path(self, &item), &item_type);
Expand All @@ -714,7 +714,7 @@ impl<'tcx> FormatRenderer<'tcx> for Context<'tcx> {
} else {
let v = layout::redirect(file_name);
let redir_dst = self.dst.join(redir_name);
self.shared.fs.write(&redir_dst, v.as_bytes())?;
self.shared.fs.write(redir_dst, v)?;
}
}
}
Expand Down
70 changes: 39 additions & 31 deletions src/librustdoc/html/render/write_shared.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,10 +105,10 @@ impl Context<'_> {
self.dst.join(&filename)
}

fn write_shared<C: AsRef<[u8]>>(
fn write_shared(
&self,
resource: SharedResource<'_>,
contents: C,
contents: impl 'static + Send + AsRef<[u8]>,
emit: &[EmitType],
) -> Result<(), Error> {
if resource.should_emit(emit) {
Expand All @@ -121,25 +121,23 @@ impl Context<'_> {
fn write_minify(
&self,
resource: SharedResource<'_>,
contents: &str,
contents: impl 'static + Send + AsRef<str> + AsRef<[u8]>,
minify: bool,
emit: &[EmitType],
) -> Result<(), Error> {
let tmp;
let contents = if minify {
tmp = if resource.extension() == Some(&OsStr::new("css")) {
if minify {
let contents = contents.as_ref();
let contents = if resource.extension() == Some(&OsStr::new("css")) {
minifier::css::minify(contents).map_err(|e| {
Error::new(format!("failed to minify CSS file: {}", e), resource.path(self))
})?
} else {
minifier::js::minify(contents)
};
tmp.as_bytes()
self.write_shared(resource, contents, emit)
} else {
contents.as_bytes()
};

self.write_shared(resource, contents, emit)
self.write_shared(resource, contents, emit)
}
}
}

Expand All @@ -155,15 +153,21 @@ pub(super) fn write_shared(
let lock_file = cx.dst.join(".lock");
let _lock = try_err!(flock::Lock::new(&lock_file, true, true, true), &lock_file);

// The weird `: &_` is to work around a borrowck bug: https://github.com/rust-lang/rust/issues/41078#issuecomment-293646723
let write_minify = |p, c: &_| {
// Minified resources are usually toolchain resources. If they're not, they should use `cx.write_minify` directly.
fn write_minify(
basename: &'static str,
contents: impl 'static + Send + AsRef<str> + AsRef<[u8]>,
cx: &Context<'_>,
options: &RenderOptions,
) -> Result<(), Error> {
cx.write_minify(
SharedResource::ToolchainSpecific { basename: p },
c,
SharedResource::ToolchainSpecific { basename },
contents,
options.enable_minification,
&options.emit,
)
};
}

// Toolchain resources should never be dynamic.
let write_toolchain = |p: &'static _, c: &'static _| {
cx.write_shared(SharedResource::ToolchainSpecific { basename: p }, c, &options.emit)
Expand Down Expand Up @@ -210,12 +214,12 @@ pub(super) fn write_shared(
"details.undocumented > summary::before, details.rustdoc-toggle > summary::before",
"toggle-plus.svg",
);
write_minify("rustdoc.css", &rustdoc_css)?;
write_minify("rustdoc.css", rustdoc_css, cx, options)?;

// Add all the static files. These may already exist, but we just
// overwrite them anyway to make sure that they're fresh and up-to-date.
write_minify("settings.css", static_files::SETTINGS_CSS)?;
write_minify("noscript.css", static_files::NOSCRIPT_CSS)?;
write_minify("settings.css", static_files::SETTINGS_CSS, cx, options)?;
write_minify("noscript.css", static_files::NOSCRIPT_CSS, cx, options)?;

// To avoid "light.css" to be overwritten, we'll first run over the received themes and only
// then we'll run over the "official" styles.
Expand All @@ -228,9 +232,9 @@ pub(super) fn write_shared(

// Handle the official themes
match theme {
"light" => write_minify("light.css", static_files::themes::LIGHT)?,
"dark" => write_minify("dark.css", static_files::themes::DARK)?,
"ayu" => write_minify("ayu.css", static_files::themes::AYU)?,
"light" => write_minify("light.css", static_files::themes::LIGHT, cx, options)?,
"dark" => write_minify("dark.css", static_files::themes::DARK, cx, options)?,
"ayu" => write_minify("ayu.css", static_files::themes::AYU, cx, options)?,
_ => {
// Handle added third-party themes
let filename = format!("{}.{}", theme, extension);
Expand Down Expand Up @@ -264,26 +268,30 @@ pub(super) fn write_shared(
// Maybe we can change the representation to move this out of main.js?
write_minify(
"main.js",
&static_files::MAIN_JS.replace(
static_files::MAIN_JS.replace(
"/* INSERT THEMES HERE */",
&format!(" = {}", serde_json::to_string(&themes).unwrap()),
),
cx,
options,
)?;
write_minify("search.js", static_files::SEARCH_JS)?;
write_minify("settings.js", static_files::SETTINGS_JS)?;
write_minify("search.js", static_files::SEARCH_JS, cx, options)?;
write_minify("settings.js", static_files::SETTINGS_JS, cx, options)?;

if cx.include_sources {
write_minify("source-script.js", static_files::sidebar::SOURCE_SCRIPT)?;
write_minify("source-script.js", static_files::sidebar::SOURCE_SCRIPT, cx, options)?;
}

{
write_minify(
"storage.js",
&format!(
format!(
"var resourcesSuffix = \"{}\";{}",
cx.shared.resource_suffix,
static_files::STORAGE_JS
),
cx,
options,
)?;
}

Expand All @@ -292,12 +300,12 @@ pub(super) fn write_shared(
// This varies based on the invocation, so it can't go through the write_minify wrapper.
cx.write_minify(
SharedResource::InvocationSpecific { basename: "theme.css" },
&buffer,
buffer,
options.enable_minification,
&options.emit,
)?;
}
write_minify("normalize.css", static_files::NORMALIZE_CSS)?;
write_minify("normalize.css", static_files::NORMALIZE_CSS, cx, options)?;
for (name, contents) in &*FILES_UNVERSIONED {
cx.write_shared(SharedResource::Unversioned { name }, contents, &options.emit)?;
}
Expand Down Expand Up @@ -512,7 +520,7 @@ pub(super) fn write_shared(
content,
&cx.shared.style_files,
);
cx.shared.fs.write(&dst, v.as_bytes())?;
cx.shared.fs.write(dst, v)?;
}
}

Expand Down Expand Up @@ -602,7 +610,7 @@ pub(super) fn write_shared(
}",
);
v.push_str("})()");
cx.shared.fs.write(&mydst, &v)?;
cx.shared.fs.write(mydst, v)?;
}
Ok(())
}
2 changes: 1 addition & 1 deletion src/librustdoc/html/sources.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ impl SourceCollector<'_, 'tcx> {
},
&self.cx.shared.style_files,
);
self.cx.shared.fs.write(&cur, v.as_bytes())?;
self.cx.shared.fs.write(cur, v)?;
self.emitted_local_sources.insert(p);
Ok(())
}
Expand Down