Skip to content
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.

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

perf(cli): Optimize setting up node_modules on macOS #23980

Merged
merged 9 commits into from
May 28, 2024
Merged
Changes from 1 commit
Commits
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
Next Next commit
Optimize cloning files for macs
  • Loading branch information
nathanwhit committed May 24, 2024
commit 02c35663f398b562e30e5688351bcd7feae45aee
82 changes: 77 additions & 5 deletions cli/util/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -492,10 +492,71 @@ pub async fn remove_dir_all_if_exists(path: &Path) -> std::io::Result<()> {
}
}

/// Copies a directory to another directory.
mod clone_dir_imp {
Copy link
Member

Choose a reason for hiding this comment

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

There is a similar optimization for cp and copyFile APIs. It uses rayon for recursive clonefile and has some other optimizations for small files and fallbacks:

fn cp(from: &Path, to: &Path) -> FsResult<()> {

Maybe could be reused?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we should use rayon in non-js sync specific scenarios. I feel like that might make stuff slower by using another thread pool.

Copy link
Member Author

Choose a reason for hiding this comment

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

Gave it a shot anyway, no performance difference but I found it caused a failure when the destination is a directory that already exists - didn't dig into it but seems like something wasn't getting copied over properly (the panic is from setting up the bin entries – the origin file we're trying to symlink doesn't exist)

thread 'main' panicked at cli/npm/managed/resolvers/local/bin_entries.rs:89:48:
called `Result::unwrap()` on an `Err` value: Os { code: 2, kind: NotFound, message: "No such file or directory" }

use deno_core::error::AnyError;

use super::copy_dir_recursive;
use std::path::Path;

cfg_if::cfg_if! {
nathanwhit marked this conversation as resolved.
Show resolved Hide resolved
if #[cfg(target_vendor = "apple")] {
use super::copy_dir_recursive_with;
use std::os::unix::ffi::OsStrExt;
fn clonefile(from: &Path, to: &Path) -> std::io::Result<()> {
let from = std::ffi::CString::new(from.as_os_str().as_bytes())?;
let to = std::ffi::CString::new(to.as_os_str().as_bytes())?;
let ret = unsafe { libc::clonefile(from.as_ptr(), to.as_ptr(), 0) };
if ret != 0 {
return Err(std::io::Error::last_os_error());
}
Ok(())
}

pub(super) fn clone_dir_recursive(
from: &Path,
to: &Path,
) -> Result<(), AnyError> {
if let Some(parent) = to.parent() {
std::fs::create_dir_all(parent)?;
}
if let Err(err) = clonefile(from, to) {
if err.kind() != std::io::ErrorKind::AlreadyExists {
log::warn!("Failed to clone dir {:?} to {:?} via clonefile: {}", from, to, err);
}
if let Err(err) = copy_dir_recursive_with(from, to, |src, dst| clonefile(src, dst).map_err(Into::into)) {
log::warn!("Failed to clone dir {:?} to {:?}: {}", from, to, err);
copy_dir_recursive(from, to)?;
}
}

Ok(())
}
} else {
use super::hard_link_dir_recursive;
pub(super) fn clone_dir_recursive(from: &Path, to: &Path) -> Result<(), AnyError> {
if let Err(e) = hard_link_dir_recursive(from, to) {
log::debug!("Failed to hard link dir {:?} to {:?}: {}", from, to, e);
copy_dir_recursive(from, to)?;
}
}
}
}
}

/// Clones a directory to another directory. The exact method
/// is not guaranteed - it may be a hardlink, copy, or other platform-specific
/// operation.
///
/// Note: Does not handle symlinks.
pub fn copy_dir_recursive(from: &Path, to: &Path) -> Result<(), AnyError> {
pub fn clone_dir_recursive(from: &Path, to: &Path) -> Result<(), AnyError> {
clone_dir_imp::clone_dir_recursive(from, to)
}

fn copy_dir_recursive_with(
from: &Path,
to: &Path,
file_copier: impl Fn(&Path, &Path) -> Result<(), AnyError>,
) -> Result<(), AnyError> {
std::fs::create_dir_all(to)
.with_context(|| format!("Creating {}", to.display()))?;
let read_dir = std::fs::read_dir(from)
Expand All @@ -512,15 +573,26 @@ pub fn copy_dir_recursive(from: &Path, to: &Path) -> Result<(), AnyError> {
format!("Dir {} to {}", new_from.display(), new_to.display())
})?;
} else if file_type.is_file() {
std::fs::copy(&new_from, &new_to).with_context(|| {
format!("Copying {} to {}", new_from.display(), new_to.display())
})?;
file_copier(&new_from, &new_to)?;
}
}

Ok(())
}

/// Copies a directory to another directory.
///
/// Note: Does not handle symlinks.
pub fn copy_dir_recursive(from: &Path, to: &Path) -> Result<(), AnyError> {
copy_dir_recursive_with(from, to, |from, to| {
std::fs::copy(from, to).with_context(|| {
format!("Copying {} to {}", from.display(), to.display())
})?;
Ok(())
})
}

#[allow(dead_code)]
/// Hardlinks the files in one directory to another directory.
///
/// Note: Does not handle symlinks.
Expand Down