Skip to content

Commit

Permalink
Improve perf of all buffer consuming APIs (#502)
Browse files Browse the repository at this point in the history
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
  • Loading branch information
SUPERCILEX authored Jun 12, 2023
1 parent 3dfd62c commit e601dcf
Show file tree
Hide file tree
Showing 9 changed files with 91 additions and 44 deletions.
6 changes: 5 additions & 1 deletion src/backend/libc/fs/syscalls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,11 @@ pub(crate) fn readlink(path: &CStr, buf: &mut [u8]) -> io::Result<usize> {

#[cfg(not(target_os = "redox"))]
#[inline]
pub(crate) fn readlinkat(dirfd: BorrowedFd<'_>, path: &CStr, buf: &mut [u8]) -> io::Result<usize> {
pub(crate) fn readlinkat(
dirfd: BorrowedFd<'_>,
path: &CStr,
buf: &mut [MaybeUninit<u8>],
) -> io::Result<usize> {
unsafe {
ret_usize(c::readlinkat(
borrowed_fd(dirfd),
Expand Down
2 changes: 1 addition & 1 deletion src/backend/libc/process/syscalls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ pub(crate) fn chroot(path: &CStr) -> io::Result<()> {

#[cfg(feature = "fs")]
#[cfg(not(target_os = "wasi"))]
pub(crate) fn getcwd(buf: &mut [u8]) -> io::Result<()> {
pub(crate) fn getcwd(buf: &mut [MaybeUninit<u8>]) -> io::Result<()> {
unsafe { ret_discarded_char_ptr(c::getcwd(buf.as_mut_ptr().cast(), buf.len())) }
}

Expand Down
2 changes: 1 addition & 1 deletion src/backend/libc/termios/syscalls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ pub(crate) fn isatty(fd: BorrowedFd<'_>) -> bool {

#[cfg(feature = "procfs")]
#[cfg(not(any(target_os = "fuchsia", target_os = "wasi")))]
pub(crate) fn ttyname(dirfd: BorrowedFd<'_>, buf: &mut [u8]) -> io::Result<usize> {
pub(crate) fn ttyname(dirfd: BorrowedFd<'_>, buf: &mut [MaybeUninit<u8>]) -> io::Result<usize> {
unsafe {
// `ttyname_r` returns its error status rather than using `errno`.
match c::ttyname_r(borrowed_fd(dirfd), buf.as_mut_ptr().cast(), buf.len()) {
Expand Down
6 changes: 5 additions & 1 deletion src/backend/linux_raw/fs/syscalls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -861,7 +861,11 @@ pub(crate) fn readlink(path: &CStr, buf: &mut [u8]) -> io::Result<usize> {
}

#[inline]
pub(crate) fn readlinkat(dirfd: BorrowedFd<'_>, path: &CStr, buf: &mut [u8]) -> io::Result<usize> {
pub(crate) fn readlinkat(
dirfd: BorrowedFd<'_>,
path: &CStr,
buf: &mut [MaybeUninit<u8>],
) -> io::Result<usize> {
let (buf_addr_mut, buf_len) = slice_mut(buf);
unsafe {
ret_usize(syscall!(
Expand Down
2 changes: 1 addition & 1 deletion src/backend/linux_raw/process/syscalls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ pub(crate) fn chroot(filename: &CStr) -> io::Result<()> {

#[cfg(feature = "fs")]
#[inline]
pub(crate) fn getcwd(buf: &mut [u8]) -> io::Result<usize> {
pub(crate) fn getcwd(buf: &mut [MaybeUninit<u8>]) -> io::Result<usize> {
let (buf_addr_mut, buf_len) = slice_mut(buf);
unsafe { ret_usize(syscall!(__NR_getcwd, buf_addr_mut, buf_len)) }
}
Expand Down
19 changes: 12 additions & 7 deletions src/backend/linux_raw/termios/syscalls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,8 @@ pub(crate) fn isatty(fd: BorrowedFd<'_>) -> bool {
}

#[cfg(feature = "procfs")]
pub(crate) fn ttyname(fd: BorrowedFd<'_>, buf: &mut [u8]) -> io::Result<usize> {
#[allow(unsafe_code)]
pub(crate) fn ttyname(fd: BorrowedFd<'_>, buf: &mut [MaybeUninit<u8>]) -> io::Result<usize> {
let fd_stat = crate::backend::fs::syscalls::fstat(fd)?;

// Quick check: if `fd` isn't a character device, it's not a tty.
Expand All @@ -300,14 +301,18 @@ pub(crate) fn ttyname(fd: BorrowedFd<'_>, buf: &mut [u8]) -> io::Result<usize> {
if r == buf.len() {
return Err(io::Errno::RANGE);
}
buf[r] = b'\0';
// SAFETY: readlinkat returns the number of bytes placed in the buffer
buf[r].write(b'\0');

// Check that the path we read refers to the same file as `fd`.
let path = CStr::from_bytes_with_nul(&buf[..=r]).unwrap();

let path_stat = crate::backend::fs::syscalls::stat(path)?;
if path_stat.st_dev != fd_stat.st_dev || path_stat.st_ino != fd_stat.st_ino {
return Err(io::Errno::NODEV);
{
// SAFETY: We just wrote the NUL byte above
let path = unsafe { CStr::from_ptr(buf.as_ptr().cast()) };

let path_stat = crate::backend::fs::syscalls::stat(path)?;
if path_stat.st_dev != fd_stat.st_dev || path_stat.st_ino != fd_stat.st_ino {
return Err(io::Errno::NODEV);
}
}

Ok(r)
Expand Down
38 changes: 26 additions & 12 deletions src/fs/at.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ pub fn openat<P: path::Arg, Fd: AsFd>(

/// `readlinkat(fd, path)`—Reads the contents of a symlink.
///
/// If `reuse` is non-empty, reuse its buffer to store the result if possible.
/// If `reuse` already has available capacity, reuse it if possible.
///
/// # References
/// - [POSIX]
Expand All @@ -81,24 +81,38 @@ pub fn readlinkat<P: path::Arg, Fd: AsFd, B: Into<Vec<u8>>>(
path.into_with_c_str(|path| _readlinkat(dirfd.as_fd(), path, reuse.into()))
}

#[allow(unsafe_code)]
fn _readlinkat(dirfd: BorrowedFd<'_>, path: &CStr, mut buffer: Vec<u8>) -> io::Result<CString> {
// This code would benefit from having a better way to read into
// uninitialized memory, but that requires `unsafe`.
buffer.clear();
buffer.reserve(SMALL_PATH_BUFFER_SIZE);
buffer.resize(buffer.capacity(), 0_u8);

loop {
let nread = backend::fs::syscalls::readlinkat(dirfd.as_fd(), path, &mut buffer)?;
let nread =
backend::fs::syscalls::readlinkat(dirfd.as_fd(), path, buffer.spare_capacity_mut())?;

let nread = nread as usize;
assert!(nread <= buffer.len());
if nread < buffer.len() {
buffer.resize(nread, 0_u8);
return Ok(CString::new(buffer).unwrap());
debug_assert!(nread <= buffer.capacity());
if nread < buffer.capacity() {
// SAFETY from the man page:
// "On success, these calls return the number of bytes placed in buf."
unsafe {
buffer.set_len(nread);
}

// SAFETY:
// - "readlink places the contents of the symbolic link pathname in the buffer buf"
// - POSIX definition 3.271 Pathname (https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_271
// "A string that is used to identify a file."
// POSIX definition 3.375: String (https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_375)
// "A contiguous sequence of bytes terminated by and including the first null byte."
// - "readlink does not append a terminating null byte to buf."
//
// Thus, there will be no NUL bytes in the string.
unsafe {
return Ok(CString::from_vec_unchecked(buffer));
}
}
buffer.reserve(1); // use `Vec` reallocation strategy to grow capacity exponentially
buffer.resize(buffer.capacity(), 0_u8);

buffer.reserve(buffer.capacity() + 1); // use `Vec` reallocation strategy to grow capacity exponentially
}
}

Expand Down
31 changes: 20 additions & 11 deletions src/process/chdir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use crate::backend::fd::AsFd;
use crate::{backend, io};
#[cfg(feature = "fs")]
use {
crate::ffi::CString,
crate::ffi::{CStr, CString},
crate::path::{self, SMALL_PATH_BUFFER_SIZE},
alloc::vec::Vec,
};
Expand Down Expand Up @@ -40,7 +40,7 @@ pub fn fchdir<Fd: AsFd>(fd: Fd) -> io::Result<()> {

/// `getCWD`—Return the current working directory.
///
/// If `reuse` is non-empty, reuse its buffer to store the result if possible.
/// If `reuse` already has available capacity, reuse it if possible.
///
/// # References
/// - [POSIX]
Expand All @@ -57,23 +57,32 @@ pub fn getcwd<B: Into<Vec<u8>>>(reuse: B) -> io::Result<CString> {
}

#[cfg(feature = "fs")]
#[allow(unsafe_code)]
fn _getcwd(mut buffer: Vec<u8>) -> io::Result<CString> {
// This code would benefit from having a better way to read into
// uninitialized memory, but that requires `unsafe`.
buffer.clear();
buffer.reserve(SMALL_PATH_BUFFER_SIZE);
buffer.resize(buffer.capacity(), 0_u8);

loop {
match backend::process::syscalls::getcwd(&mut buffer) {
match backend::process::syscalls::getcwd(buffer.spare_capacity_mut()) {
Err(io::Errno::RANGE) => {
buffer.reserve(1); // use `Vec` reallocation strategy to grow capacity exponentially
buffer.resize(buffer.capacity(), 0_u8);
buffer.reserve(buffer.capacity() + 1); // use `Vec` reallocation strategy to grow capacity exponentially
}
Ok(_) => {
let len = buffer.iter().position(|x| *x == b'\0').unwrap();
buffer.resize(len, 0_u8);
return Ok(CString::new(buffer).unwrap());
// SAFETY:
// - "These functions return a null-terminated string"
// - POSIX definition 3.375: String (https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_375)
// "A contiguous sequence of bytes terminated by and including the first null byte."
//
// Thus, there will be a single NUL byte at the end of the string.
unsafe {
buffer.set_len(
CStr::from_ptr(buffer.as_ptr().cast())
.to_bytes_with_nul()
.len(),
);

return Ok(CString::from_vec_with_nul_unchecked(buffer));
}
}
Err(errno) => return Err(errno),
}
Expand Down
29 changes: 20 additions & 9 deletions src/termios/tty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ pub fn isatty<Fd: AsFd>(fd: Fd) -> bool {

/// `ttyname_r(fd)`
///
/// If `reuse` is non-empty, reuse its buffer to store the result if possible.
/// If `reuse` already has available capacity, reuse it if possible.
///
/// # References
/// - [POSIX]
Expand All @@ -43,22 +43,33 @@ pub fn ttyname<Fd: AsFd, B: Into<Vec<u8>>>(dirfd: Fd, reuse: B) -> io::Result<CS

#[cfg(not(any(target_os = "fuchsia", target_os = "wasi")))]
#[cfg(feature = "procfs")]
#[allow(unsafe_code)]
fn _ttyname(dirfd: BorrowedFd<'_>, mut buffer: Vec<u8>) -> io::Result<CString> {
// This code would benefit from having a better way to read into
// uninitialized memory, but that requires `unsafe`.
buffer.clear();
buffer.reserve(SMALL_PATH_BUFFER_SIZE);
buffer.resize(buffer.capacity(), 0_u8);

loop {
match backend::termios::syscalls::ttyname(dirfd, &mut buffer) {
match backend::termios::syscalls::ttyname(dirfd, buffer.spare_capacity_mut()) {
Err(io::Errno::RANGE) => {
buffer.reserve(1); // use `Vec` reallocation strategy to grow capacity exponentially
buffer.resize(buffer.capacity(), 0_u8);
buffer.reserve(buffer.capacity() + 1); // use `Vec` reallocation strategy to grow capacity exponentially
}
Ok(len) => {
buffer.resize(len, 0_u8);
return Ok(CString::new(buffer).unwrap());
// SAFETY: assume the backend returns the length of the string
unsafe {
buffer.set_len(len);
}

// SAFETY:
// - "ttyname_r stores this pathname in the buffer buf"
// - POSIX definition 3.271 Pathname (https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_271
// "A string that is used to identify a file."
// POSIX definition 3.375: String (https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_375)
// "A contiguous sequence of bytes terminated by and including the first null byte."
//
// Thus, there will be a single NUL byte at the end of the string.
unsafe {
return Ok(CString::from_vec_with_nul_unchecked(buffer));
}
}
Err(errno) => return Err(errno),
}
Expand Down

0 comments on commit e601dcf

Please sign in to comment.