From c323a2d5fe69375ee12d428de6b4dc7812be34c8 Mon Sep 17 00:00:00 2001 From: Peter Ammon Date: Sat, 8 Nov 2025 18:44:21 -0800 Subject: [PATCH] Continued refactoring of history Use fstat() instead of lseek() to determine history file size. Pass around the file_id instead of recomputing it. This saves a few syscalls; no behavior change is expected. --- src/env_universal_common.rs | 13 ++++++++----- src/fs.rs | 13 ++++++------- src/history/file.rs | 14 +++++++------- src/history/history.rs | 8 ++++---- 4 files changed, 25 insertions(+), 23 deletions(-) diff --git a/src/env_universal_common.rs b/src/env_universal_common.rs index 685894e3d..51cd88616 100644 --- a/src/env_universal_common.rs +++ b/src/env_universal_common.rs @@ -175,7 +175,7 @@ pub fn sync(&mut self) -> (bool, Option) { let rewrite = |old_file: &File, tmp_file: &mut File| -> std::io::Result>> { - match self.load_from_file(old_file) { + match self.load_from_file(old_file, file_id_for_file(old_file)) { Some(potential_update) => { if potential_update.do_save { let contents = Self::serialize_with_vars(&potential_update.data.new_vars); @@ -364,8 +364,8 @@ fn load_from_path_narrow(&mut self) -> Option { } FLOG!(uvar_file, "universal log reading from file"); - match lock_and_load(&self.vars_path, |f| { - Ok(self.load_from_file(f).map(|update| update.data)) + match lock_and_load(&self.vars_path, |f, file_id| { + Ok(self.load_from_file(f, file_id).map(|update| update.data)) }) { Ok(( file_id, @@ -402,9 +402,12 @@ fn load_from_path_narrow(&mut self) -> Option { // IMPORTANT: Callers of this code assume that a return value of None means that the file id has // not changed. Do not return None in other situations without modifying the callers // accordingly. Otherwise, problems with self.ok_to_save are expected to occur. - fn load_from_file(&self, file: &File) -> Option> { + fn load_from_file( + &self, + file: &File, + current_file_id: FileId, + ) -> Option> { // Get the dev / inode. - let current_file_id = file_id_for_file(file); if current_file_id == self.last_read_file_id { FLOG!(uvar_file, "universal log sync elided based on fstat()"); None diff --git a/src/fs.rs b/src/fs.rs index 7721533b8..2c79199cf 100644 --- a/src/fs.rs +++ b/src/fs.rs @@ -213,14 +213,13 @@ pub fn fsync(file: &File) -> std::io::Result<()> { /// If the file does not exist this function will return an error. pub fn lock_and_load(path: &wstr, load: F) -> std::io::Result<(FileId, UserData)> where - F: Fn(&File) -> std::io::Result, + F: Fn(&File, FileId) -> std::io::Result, { match LockedFile::new(LockingMode::Shared, path) { Ok(locked_file) => { - return Ok(( - file_id_for_file(locked_file.get()), - load(locked_file.get())?, - )); + let file_id = file_id_for_file(locked_file.get()); + let user_data = load(locked_file.get(), file_id.clone())?; + return Ok((file_id, user_data)); } Err(e) => { FLOGF!( @@ -246,11 +245,11 @@ pub fn lock_and_load(path: &wstr, load: F) -> std::io::Result<(File // Fallback implementation for situations where locking is unavailable. let max_attempts = 1000; for _ in 0..max_attempts { - let initial_file_id = file_id_for_path(path); // If we cannot open the file, there is nothing we can do, // so just return immediately. let file = wopen_cloexec(path, OFlag::O_RDONLY, Mode::empty())?; - let loaded_data = match load(&file) { + let initial_file_id = file_id_for_file(&file); + let loaded_data = match load(&file, initial_file_id.clone()) { Ok(update_data) => update_data, Err(_) => { // Retry if load function failed. Because we do not hold a lock, this might be diff --git a/src/history/file.rs b/src/history/file.rs index b10e113d8..35de393dd 100644 --- a/src/history/file.rs +++ b/src/history/file.rs @@ -2,7 +2,7 @@ use std::{ fs::File, - io::{Read, Seek, SeekFrom, Write}, + io::{Read, Write}, ops::{Deref, DerefMut}, os::fd::AsRawFd, time::{SystemTime, UNIX_EPOCH}, @@ -18,13 +18,14 @@ common::wcs2bytes, flog::FLOG, path::{DirRemoteness, path_get_data_remoteness}, + wutil::FileId, }; /// History file types. #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub enum HistoryFileType { - Fish2_0, - Fish1_x, + Fish2_0, // old format with just timestamp and item + Fish1_x, // YAML-style format } /// A type wrapping up the logic around mmap and munmap. @@ -118,10 +119,10 @@ pub struct RawHistoryFile { } impl RawHistoryFile { - /// Construct a history file contents from a [`File`] reference. - pub fn create(mut history_file: &File) -> std::io::Result { + /// Construct a history file contents from a [`File`] reference and its file id. + pub fn create(history_file: &File, file_id: FileId) -> std::io::Result { // Check that the file is seekable, and its size. - let len: usize = match history_file.seek(SeekFrom::End(0))?.try_into() { + let len: usize = match file_id.size.try_into() { Ok(len) => len, Err(err) => { return Err(std::io::Error::new( @@ -138,7 +139,6 @@ pub fn create(mut history_file: &File) -> std::io::Result { let map_anon = |mut file: &File, len: usize| -> std::io::Result { let mut region = MmapRegion::map_anon(len)?; // If we mapped anonymous memory, we have to read from the file. - file.seek(SeekFrom::Start(0))?; file.read_exact(&mut region)?; Ok(region) }; diff --git a/src/history/history.rs b/src/history/history.rs index 3852e66bd..c90aae40c 100644 --- a/src/history/history.rs +++ b/src/history/history.rs @@ -31,7 +31,6 @@ fs::File, io::{BufRead, Read, Write}, mem::MaybeUninit, - num::NonZeroUsize, ops::ControlFlow, sync::{Arc, Mutex, MutexGuard}, time::{Duration, SystemTime, UNIX_EPOCH}, @@ -108,7 +107,7 @@ pub enum SearchDirection { /// When we rewrite the history, the number of items we keep. // FIXME: https://github.com/rust-lang/rust/issues/67441 -const HISTORY_SAVE_MAX: NonZeroUsize = NonZeroUsize::new(1024 * 256).unwrap(); +const HISTORY_SAVE_MAX: usize = 1024 * 256; /// Default buffer size for flushing to the history file. const HISTORY_OUTPUT_BUFFER_SIZE: usize = 64 * 1024; @@ -519,11 +518,12 @@ fn rewrite_to_temporary_file( // We are reading FROM existing_file and writing TO dst // Make an LRU cache to save only the last N elements. - let mut lru = LruCache::new(HISTORY_SAVE_MAX); + let mut lru = LruCache::new(HISTORY_SAVE_MAX.try_into().unwrap()); // Read in existing items (which may have changed out from underneath us, so don't trust our // old file contents). - if let Ok(local_file) = RawHistoryFile::create(existing_file) { + let file_id = file_id_for_file(existing_file); + if let Ok(local_file) = RawHistoryFile::create(existing_file, file_id) { for offset in local_file.offsets(None) { // Try decoding an old item. let Some(old_item) = local_file.decode_item(offset) else {