-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
perform invalidation directly on writes #5786
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -68,7 +68,11 @@ use turbo_tasks_hash::hash_xxh3_hash64; | |
use util::{extract_disk_access, join_path, normalize_path, sys_to_unix, unix_to_sys}; | ||
pub use virtual_fs::VirtualFileSystem; | ||
|
||
use self::{invalidation::WatchStart, json::UnparseableJson, mutex_map::MutexMap}; | ||
use self::{ | ||
invalidation::{WatchStart, Write}, | ||
json::UnparseableJson, | ||
mutex_map::MutexMap, | ||
}; | ||
use crate::{ | ||
attach::AttachedFileSystem, | ||
invalidation::WatchChange, | ||
|
@@ -200,6 +204,20 @@ impl DiskFileSystem { | |
Ok(()) | ||
} | ||
|
||
/// registers the path as an invalidator for the current task, | ||
/// has to be called within a turbo-tasks function. It removes and returns | ||
/// the current list of invalidators. | ||
fn register_sole_invalidator(&self, path: &Path) -> Result<HashSet<Invalidator>> { | ||
let invalidator = turbo_tasks::get_invalidator(); | ||
let mut invalidator_map = self.invalidator_map.lock().unwrap(); | ||
let old_invalidators = invalidator_map.insert(path_to_key(path), [invalidator].into()); | ||
#[cfg(not(any(target_os = "macos", target_os = "windows")))] | ||
if let Some(dir) = path.parent() { | ||
self.watcher.ensure_watching(dir, self.root_path())?; | ||
} | ||
Ok(old_invalidators.unwrap_or_default()) | ||
} | ||
|
||
/// registers the path as an invalidator for the current task, | ||
/// has to be called within a turbo-tasks function | ||
fn register_dir_invalidator(&self, path: &Path) -> Result<()> { | ||
|
@@ -487,14 +505,33 @@ impl DiskFileSystem { | |
path.join(&*unix_to_sys(&fs_path.path)) | ||
}) | ||
} | ||
|
||
fn invalidate_from_write(&self, full_path: &Path, invalidators: HashSet<Invalidator>) { | ||
if !invalidators.is_empty() { | ||
if let Some(path) = format_absolute_fs_path(&full_path, &self.name, self.root_path()) { | ||
if invalidators.len() == 1 { | ||
let invalidator = invalidators.into_iter().next().unwrap(); | ||
invalidator.invalidate_with_reason(Write { path }); | ||
jridgewell marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} else { | ||
invalidators.into_iter().for_each(|invalidator| { | ||
invalidator.invalidate_with_reason(Write { path: path.clone() }); | ||
}); | ||
} | ||
} else { | ||
invalidators.into_iter().for_each(|invalidator| { | ||
invalidator.invalidate(); | ||
}); | ||
} | ||
} | ||
} | ||
} | ||
|
||
struct PathLockGuard<'a>( | ||
RwLockReadGuard<'a, ()>, | ||
mutex_map::MutexMapGuard<'a, PathBuf>, | ||
); | ||
|
||
fn format_absolute_fs_path(path: &Path, name: &str, root_path: &PathBuf) -> Option<String> { | ||
fn format_absolute_fs_path(path: &Path, name: &str, root_path: &Path) -> Option<String> { | ||
let path = if let Ok(rel_path) = path.strip_prefix(root_path) { | ||
let path = if MAIN_SEPARATOR != '/' { | ||
let rel_path = rel_path.to_string_lossy().replace(MAIN_SEPARATOR, "/"); | ||
|
@@ -710,22 +747,29 @@ impl FileSystem for DiskFileSystem { | |
let full_path = self.to_sys_path(fs_path).await?; | ||
let content = content.await?; | ||
|
||
// Track the file, so that we will rewrite it if it ever changes. | ||
fs_path.track().await?; | ||
|
||
let _lock = self.lock_path(&full_path).await; | ||
|
||
// Track the file, so that we will rewrite it if it ever changes. | ||
let old_invalidators = self.register_sole_invalidator(&full_path)?; | ||
|
||
// We perform an untracked comparison here, so that this write is not dependent | ||
// on a read's Vc<FileContent> (and the memory it holds). Our untracked read can | ||
// be freed immediately. Given this is an output file, it's unlikely any Turbo | ||
// code will need to read the file from disk into a Vc<FileContent>, so we're | ||
// not wasting cycles. | ||
let compare = content.streaming_compare(full_path.clone()).await?; | ||
if compare == FileComparison::Equal { | ||
if !old_invalidators.is_empty() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: comment why we're preserving the old invalidators. Especially because it seems like we're trying to emit the same contents from multiple tasks, shouldn't that be an error, too? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. old_invalidators can also contain invalidators from We don't want to invalidate read calls when writing identical content to a file. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
But that means there are two owners of the file. Shouldn't that be an error, why aren't they reusing the same write cell? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe, but |
||
let key = path_to_key(&full_path); | ||
for i in old_invalidators { | ||
self.invalidator_map.insert(key.clone(), i); | ||
} | ||
} | ||
return Ok(Completion::unchanged()); | ||
} | ||
|
||
let create_directory = compare == FileComparison::Create; | ||
|
||
match &*content { | ||
FileContent::Content(file) => { | ||
if create_directory { | ||
|
@@ -769,6 +813,8 @@ impl FileSystem for DiskFileSystem { | |
} | ||
} | ||
|
||
self.invalidate_from_write(&full_path, old_invalidators); | ||
|
||
Ok(Completion::new()) | ||
} | ||
|
||
|
@@ -779,6 +825,8 @@ impl FileSystem for DiskFileSystem { | |
target: Vc<LinkContent>, | ||
) -> Result<Vc<Completion>> { | ||
let full_path = self.to_sys_path(fs_path).await?; | ||
// TODO(sokra) preform a untracked read here, register an invalidator and get | ||
// all existing invalidators | ||
let old_content = fs_path | ||
.read_link() | ||
.await | ||
|
@@ -834,7 +882,7 @@ impl FileSystem for DiskFileSystem { | |
return Err(anyhow!("invalid symlink target: {}", full_path.display())); | ||
} | ||
LinkContent::NotFound => { | ||
retry_future(|| fs::remove_file(full_path.clone())) | ||
retry_future(|| fs::remove_file(&full_path)) | ||
.await | ||
.or_else(|err| { | ||
if err.kind() == ErrorKind::NotFound { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be merged with
register_invalidator
. The difference seems to be whether it returns the old value, which could just be discarded by the callers of the oldregister_invalidator
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
register_invalidator
doesn't remove the old_invalidators. It only adds an additional invalidator.register_sole_invalidator
removes the old_invalidators. Basically replacing the list with only that one invalidator.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how we get two different behaviors out of these two pieces of code:
They're both calling
map.insert(key)
, so how does one append and one replaces?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are different
insert
methods.InvalidatorMap::insert
adds a single invalidator to the list.HashMap::insert
replaces the current list with a different list.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I didn't realize that
lock
wasn't itself a mutex, it was a method returning the mutex guardedHashMap
.