Skip to content

Commit

Permalink
more perf
Browse files Browse the repository at this point in the history
remove unneeded into_owned
  • Loading branch information
KGrewal1 committed Nov 28, 2024
1 parent 901c5ab commit aacbd87
Showing 1 changed file with 8 additions and 10 deletions.
18 changes: 8 additions & 10 deletions nativelink-store/src/filesystem_store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -633,14 +633,14 @@ impl<Fe: FileEntry> FilesystemStore<Fe> {
self.evicting_map
.get::<StoreKey<'static>>(&digest.into())
.await
.ok_or_else(|| make_err!(Code::NotFound, "{} not found in filesystem store", digest))
.ok_or_else(|| make_err!(Code::NotFound, "{digest} not found in filesystem store"))
}

async fn update_file<'a>(
self: Pin<&'a Self>,
mut entry: Fe,
mut resumeable_temp_file: fs::ResumeableFileSlot,
final_digest: StoreKey<'static>,
final_key: StoreKey<'static>,
mut reader: DropCloserReadHalf,
) -> Result<(), Error> {
let mut data_size = 0;
Expand Down Expand Up @@ -685,10 +685,10 @@ impl<Fe: FileEntry> FilesystemStore<Fe> {
drop(resumeable_temp_file);

*entry.data_size_mut() = data_size;
self.emplace_file(final_digest, Arc::new(entry)).await
self.emplace_file(final_key, Arc::new(entry)).await
}

async fn emplace_file(&self, key: StoreKey<'_>, entry: Arc<Fe>) -> Result<(), Error> {
async fn emplace_file(&self, key: StoreKey<'static>, entry: Arc<Fe>) -> Result<(), Error> {
// This sequence of events is quite ticky to understand due to the amount of triggers that
// happen, async'ness of it and the locking. So here is a breakdown of what happens:
// 1. Here will hold a write lock on any file operations of this FileEntry.
Expand All @@ -709,9 +709,6 @@ impl<Fe: FileEntry> FilesystemStore<Fe> {
let evicting_map = self.evicting_map.clone();
let rename_fn = self.rename_fn;

// we need to extend the lifetime into 'static, for background spawn
let key = key.borrow().into_owned();

// We need to guarantee that this will get to the end even if the parent future is dropped.
// See: https://github.com/TraceMachina/nativelink/issues/495
background_spawn!("filesystem_store_emplace_file", async move {
Expand Down Expand Up @@ -750,13 +747,14 @@ impl<Fe: FileEntry> FilesystemStore<Fe> {
drop(encoded_file_path);
// It is possible that the item in our map is no longer the item we inserted,
// So, we need to conditionally remove it only if the pointers are the same.

evicting_map
.remove_if(&key, |map_entry| Arc::<Fe>::ptr_eq(map_entry, &entry))
.await;
return Err(err);
}
encoded_file_path.path_type = PathType::Content;
encoded_file_path.key = key.borrow().into_owned();
encoded_file_path.key = key;
Ok(())
})
.await
Expand Down Expand Up @@ -817,7 +815,7 @@ impl<Fe: FileEntry> StoreDriver for FilesystemStore<Fe> {
)
.await?;

self.update_file(entry, temp_file, key.borrow().into_owned(), reader)
self.update_file(entry, temp_file, key.into_owned(), reader)
.await
.err_tip(|| format!("While processing with temp file {temp_full_path:?}"))
}
Expand Down Expand Up @@ -860,7 +858,7 @@ impl<Fe: FileEntry> StoreDriver for FilesystemStore<Fe> {
// We are done with the file, if we hold a reference to the file here, it could
// result in a deadlock if `emplace_file()` also needs file descriptors.
drop(file);
self.emplace_file(key, Arc::new(entry))
self.emplace_file(key.into_owned(), Arc::new(entry))
.await
.err_tip(|| "Could not move file into store in upload_file_to_store, maybe dest is on different volume?")?;
return Ok(None);
Expand Down

0 comments on commit aacbd87

Please sign in to comment.