Skip to content
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

The remoting client currently buffers all fetched blobs in memory before storing to LMDB. #17065

Closed
jsirois opened this issue Sep 30, 2022 · 4 comments · Fixed by #18054
Closed

Comments

@jsirois
Copy link
Contributor

jsirois commented Sep 30, 2022

That's here:

let read_result_closure = async {
let mut buf = BytesMut::with_capacity(digest.size_bytes);
while let Some(response) = stream.next().await {
// Record the observed time to receive the first response for this read.
if let Some(start) = start_opt.take() {
if let Some(workunit_store_handle) = workunit_store::get_workunit_store_handle() {
let timing: Result<u64, _> =
Instant::now().duration_since(start).as_micros().try_into();
if let Ok(obs) = timing {
workunit_store_handle
.store
.record_observation(ObservationMetric::RemoteStoreTimeToFirstByteMicros, obs);
}
}
}
buf.extend_from_slice(&(response?).data);
}
Ok(buf.freeze())
};

For things like ~GB pytorch wheel zips; that could be problematic.

@jsirois jsirois added the bug label Sep 30, 2022
@stuhood
Copy link
Member

stuhood commented Sep 30, 2022

One option would be to store into a temporary file, and then copy into LMDB with:

///
/// Store data in two passes, without buffering it entirely into memory. Prefer
/// `Self::store_bytes` for small values which fit comfortably in memory.
///
pub async fn store<F, R>(
&self,
entry_type: EntryType,
initial_lease: bool,
data_is_immutable: bool,
data_provider: F,
) -> Result<Digest, String>
where
R: Read + Debug,
F: Fn() -> Result<R, io::Error> + Send + 'static,
{

But that would use more passes over the data than we strictly need. We do need to re-compute and validate the Digest of the data (because we don't immediately trust the data that we get over the wire), but the Digest could be computed while writing the data to the temporary file, rather than via reading it in two passes as fn store will do.

@stuhood
Copy link
Member

stuhood commented Nov 9, 2022

One other much more fundamental idea would be to actually begin to "size split" our local::Store, and to store files larger than a threshold as files on disk. LMDB is great for small files and directory entries, but it's advantages diminish for files which are too large to buffer in RAM.

That would have advantages for this codepath, but it would also have advantages for #17282, because if we chose our heuristics well, we could symlink directly from a large-file store, rather than first copying a large file out of the store and into a real file. cc @thejcannon

@stuhood stuhood added the remote label Nov 14, 2022
stuhood pushed a commit that referenced this issue Jan 18, 2023
This is an no-functionality-change refactoring of `store::remote::ByteStore::load_bytes_with` that's arguably cleaner and also step towards #11149. In particular:

1. that method doesn't need to take a closure any more, and thus is refactored to just be the "simplest": `load_bytes(...) -> Result<Option<Bytes>, String>`
2. that method previously didn't retry, and thus users had to do the retries themselves: this moves the retries to be fully within the `load_bytes` method itself, which is both easier to use, and keeps implementation details like gRPC (previously exposed as the `ByteStoreError::Grpc`/`tonic::Status` error variant) entirely contained to `store::remote::ByteStore`
3. to emphasise that last point, the `ByteStoreError` enum can thus become private, because it's an implementation detail of `store::remote::ByteStore`, no longer exposed in the public API

Step 1 resolves (and removes) a TODO comment. That TODO references #17065, but this patch _doesn't_ fix that issue.
@stuhood
Copy link
Member

stuhood commented Jan 20, 2023

One other much more fundamental idea would be to actually begin to "size split" our local::Store, and to store files larger than a threshold as files on disk. LMDB is great for small files and directory entries, but it's advantages diminish for files which are too large to buffer in RAM.

That would have advantages for this codepath, but it would also have advantages for #17282, because if we chose our heuristics well, we could symlink directly from a large-file store, rather than first copying a large file out of the store and into a real file. cc @thejcannon

Opened #18048 for this idea.

@huonw
Copy link
Contributor

huonw commented Feb 12, 2023

But that would use more passes over the data than we strictly need. We do need to re-compute and validate the Digest of the data (because we don't immediately trust the data that we get over the wire), but the Digest could be computed while writing the data to the temporary file, rather than via reading it in two passes as fn store will do.

I've opened #18231 for this, since #18054 solves the basic "avoid buffering into memory" issue, without optimising the hashing.

huonw added a commit that referenced this issue Feb 12, 2023
This fixes #17065 by having remote cache loads be able to be streamed to
disk. In essence, the remote store now has a `load_file` method in addition to
`load_bytes`, and thus the caller can decide to download to a file instead.

This doesn't make progress towards #18048 (this PR doesn't touch the local
store at all), but I think it will help with integrating the remote store with
that code: in theory the `File` could be provided in a way that can be part of
the "large file pool" directly (and indeed, the decision about whether to
download to a file or into memory ties into that).

This also does a theoretically unnecessary extra pass over the data (as
discussed in #18231) to verify the digest, but I think it'd make sense to do
that as a future optimisation, since it'll require refactoring more deeply
(down into `sharded_lmdb` and `hashing`, I think) and is best to build on
#18153 once that lands.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants