Skip loading of local cache data when possible #17495

Eric-Arellano · 2022-11-08T21:12:14Z

Eric-Arellano · 2022-11-09T19:07:44Z

src/rust/engine/fs/store/src/lib.rs

+  async fn download_digest_to_local<
+    FRemote: Fn(Bytes) -> Result<(), String> + Send + Sync + 'static,


This function was simply extracted from load_bytes_with

Eric-Arellano · 2022-11-09T19:08:08Z

src/rust/engine/fs/store/src/lib.rs

+          )
+        })?;
+
+        f_remote(bytes.clone())?;


FYI this clone seems like wasted performance in some cases, but will leave it for a followup

Bytes.clone() is very cheap, because it is reference counted. But yea, I think you're right that it is no longer necessary here.

Eric-Arellano · 2022-11-09T19:08:33Z

src/rust/engine/fs/store/src/lib.rs

-  /// Ensure that a directory is locally loadable, which will download it from the Remote store as
-  /// a sideeffect (if one is configured).
-  ///
-  pub async fn ensure_local_has_recursive_directory(


This was inlined into the new function ensure_local_has_files

Eric-Arellano · 2022-11-09T19:08:45Z

src/rust/engine/fs/store/src/lib.rs

+    let missing_file_digests = self
+      .local
+      .get_missing_digests(EntryType::File, file_digests)
+      .await?;
+    if missing_file_digests.is_empty() {
+      return Ok(());
+    }


This logic is new

Eric-Arellano · 2022-11-09T19:09:06Z

src/rust/engine/fs/store/src/lib.rs

-        .map(|file_digest| self.ensure_local_has_file(file_digest))
-        .collect::<Vec<_>>(),
+        .map(|file_digest| async move {
+          if let Err(e) = remote
+            .download_digest_to_local(self.local.clone(), file_digest, EntryType::File, |_| Ok(()))
+            .await


This is the key change. We now only download, don't load.

stuhood

Looks great, thanks!

stuhood · 2022-11-09T19:42:46Z

src/rust/engine/fs/store/src/lib.rs

@@ -229,6 +229,59 @@ impl RemoteStore {
      .await
      .map(|&()| ())
  }
+
+  /// Download the digest to the local byte store from this remote store. The function `f_remote`
+  /// can be used to validate/transform the bytes.


Suggested change

/// can be used to validate/transform the bytes.

/// can be used to validate the bytes.

Bytes is immutable, so the function can only validate.

stuhood · 2022-11-09T19:43:35Z

src/rust/engine/fs/store/src/lib.rs

+        // TODO: Now that we always copy from the remote store to the local store before executing
+        // the caller's logic against the local store, `remote::ByteStore::load_bytes_with` no
+        // longer needs to accept a function.


Can you add a pointer to #17065 here? I have some more ideas that I will likely put on that ticket today.

stuhood · 2022-11-09T19:49:15Z

src/rust/engine/fs/store/src/lib.rs

+          )
+        })?;
+
+        f_remote(bytes.clone())?;


Bytes.clone() is very cheap, because it is reference counted. But yea, I think you're right that it is no longer necessary here.

stuhood · 2022-11-09T19:50:18Z

src/rust/engine/fs/store/src/lib.rs

-  pub async fn ensure_local_has_recursive_directory(
+  /// Ensure that the files are locally loadable. This will download them from the remote store as
+  /// a side effect, if one is configured.
+  pub async fn ensure_local_has_files(


Maybe rename to: ensure_local_has_digests, ensure_local_has, ensure_downloaded, etc.

Somewhat implicit in here is that we also want to guarantee that the Directory protos have been persisted locally... so this method should probably also call record_digest_trie on each loaded tree to ensure that they exist locally as well. It's likely a bug that we weren't doing that before.

Is the idea that at the end of callling download_digest_to_local on every missing file digest, we then also use a future::try_join_all that calls ensure_directory_digest_persisted on every directory digest inputted?

we then also use a future::try_join_all that calls ensure_directory_digest_persisted on every directory digest inputted?

Yea, either at the beginning or the end is fine. It's also possible to use try_join! to do it concurrently.

But you should be able to use record_digest_trie rather than ensure_directory_digest_persisted, because you will have already called load_digest_trie at the beginning of the method.

@stuhood I want to make sure I got it right why it's safe to use record_digest_trie before we hit the remote store. That function is only about storing the directory entries, which don't need to come from the remote? We only download file entries from the remote?

That function is only about storing the directory entries, which don't need to come from the remote? We only download file entries from the remote?

Right: record_digest_trie only stores the Directory entries. Unlike files, Directory entries may be in one of three places: 1. memory, 2. local, 3. remote... record_digest_trie ensures that they are actually on disk locally, rather than potentially only in memory or remote.

Eric-Arellano · 2022-11-10T16:38:53Z

src/rust/engine/fs/store/src/lib.rs

+          }
+          directory::Entry::Symlink(_) | directory::Entry::Directory(_) => (),
+        });
+        self.record_digest_trie(trie, true).await?;


@stuhood this sets initial_lease to true. I wasn't sure what that was about

... me neither. I'm pretty sure that we should remove that parameter, but haven't spent the time to confirm.

A comment here to explain why we do this (or in the method signature) would be great.

stuhood · 2022-11-10T16:43:48Z

src/rust/engine/fs/store/src/lib.rs

+    // let _ = future::try_join_all(
+    //   directory_digests


Oh, stale code from iterating

stuhood · 2022-11-10T16:44:41Z

src/rust/engine/fs/store/src/lib.rs

+          }
+          directory::Entry::Symlink(_) | directory::Entry::Directory(_) => (),
+        });
+        self.record_digest_trie(trie, true).await?;


A comment here to explain why we do this (or in the method signature) would be great.

Eric-Arellano · 2022-11-10T17:15:33Z

@stuhood should this be cherry-picked to 2.15?

stuhood · 2022-11-10T18:28:39Z

@stuhood should this be cherry-picked to 2.15?

I believe so, yea. A 95% performance difference for a cache lookup amounts to a bug.

…uild#17495) Closes pantsbuild#17448. # Conflicts: # src/rust/engine/fs/store/src/lib.rs

#17524) Closes #17448.

Eric-Arellano added the category:performance label Nov 8, 2022

Eric-Arellano added 5 commits November 9, 2022 13:03

Combine into a single batched function, but not parallelized

a03782a

Only operate on missing digests

643c3c6

Parallelize directory digest handling

2bba68c

Only download, don't load the bytes

e5f6a4d

Parallelize

31b0543

Eric-Arellano force-pushed the fix-cache-perf branch from 9b52ed3 to 31b0543 Compare November 9, 2022 19:03

Eric-Arellano changed the title ~~WIP: Skip loading of local cache data when possible~~ Skip loading of local cache data when possible Nov 9, 2022

Eric-Arellano marked this pull request as ready for review November 9, 2022 19:03

Fix test

0e76b70

Eric-Arellano commented Nov 9, 2022

View reviewed changes

stuhood reviewed Nov 9, 2022

View reviewed changes

Eric-Arellano added 3 commits November 9, 2022 17:15

Initial feedback

839251f

Fix test

965ff98

Use record_digest_trie

6188dcd

Eric-Arellano commented Nov 10, 2022

View reviewed changes

stuhood approved these changes Nov 10, 2022

View reviewed changes

Fix bug! Dir digest may have multiple file entries

56c415f

Eric-Arellano enabled auto-merge (squash) November 10, 2022 18:08

Eric-Arellano merged commit 19de895 into pantsbuild:main Nov 10, 2022

Eric-Arellano deleted the fix-cache-perf branch November 10, 2022 18:24

stuhood added the needs-cherrypick label Nov 10, 2022

stuhood added this to the 2.15.x milestone Nov 10, 2022

Eric-Arellano added a commit to Eric-Arellano/pants that referenced this pull request Nov 10, 2022

Skip loading of local cache data when possible (Cherry-pick of pantsb…

5c4e307

…uild#17495) Closes pantsbuild#17448. # Conflicts: # src/rust/engine/fs/store/src/lib.rs

Eric-Arellano removed the needs-cherrypick label Nov 10, 2022

Eric-Arellano added a commit that referenced this pull request Nov 10, 2022

Skip loading of local cache data when possible (Cherry-pick of #17495) (

0c620f1

#17524) Closes #17448.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip loading of local cache data when possible #17495

Skip loading of local cache data when possible #17495

Eric-Arellano commented Nov 8, 2022

Eric-Arellano Nov 9, 2022

Eric-Arellano Nov 9, 2022

stuhood Nov 9, 2022

Eric-Arellano Nov 9, 2022

Eric-Arellano Nov 9, 2022

Eric-Arellano Nov 9, 2022

stuhood left a comment

stuhood Nov 9, 2022

stuhood Nov 9, 2022

stuhood Nov 9, 2022

stuhood Nov 9, 2022

Eric-Arellano Nov 9, 2022

stuhood Nov 9, 2022

Eric-Arellano Nov 10, 2022

stuhood Nov 10, 2022

Eric-Arellano Nov 10, 2022

stuhood Nov 10, 2022

stuhood Nov 10, 2022

stuhood Nov 10, 2022

Eric-Arellano Nov 10, 2022

stuhood Nov 10, 2022

Eric-Arellano commented Nov 10, 2022

stuhood commented Nov 10, 2022

		async fn download_digest_to_local<
		FRemote: Fn(Bytes) -> Result<(), String> + Send + Sync + 'static,

	/// can be used to validate/transform the bytes.
	/// can be used to validate the bytes.

Skip loading of local cache data when possible #17495

Skip loading of local cache data when possible #17495

Conversation

Eric-Arellano commented Nov 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eric-Arellano commented Nov 10, 2022

stuhood commented Nov 10, 2022