From 84114cf02c7d742ff4bbfb1952734839d7e013eb Mon Sep 17 00:00:00 2001 From: Florian Klink Date: Fri, 10 May 2024 08:31:11 +0300 Subject: refactor(tvix/castore/blobservice/memory): use parking_lot RwLock This one doesn't require us to deal with poisoning, is upgradeable and the right thing to use when locking access to data, not IO resources. Change-Id: I78634953a73404500d28f51f1d93a87e215c8149 Reviewed-on: https://cl.tvl.fyi/c/depot/+/11612 Autosubmit: flokli Tested-by: BuildkiteCI Reviewed-by: Connor Brewster --- tvix/Cargo.lock | 9 +++++---- tvix/Cargo.nix | 16 ++++++++++------ tvix/castore/src/blobservice/memory.rs | 28 +++++++++------------------- tvix/crate-hashes.json | 4 ++-- tvix/store/Cargo.toml | 1 + 5 files changed, 27 insertions(+), 31 deletions(-) (limited to 'tvix') diff --git a/tvix/Cargo.lock b/tvix/Cargo.lock index d0d310fdb6bb..92229302dddf 100644 --- a/tvix/Cargo.lock +++ b/tvix/Cargo.lock @@ -2305,7 +2305,7 @@ dependencies = [ "hyper 0.14.28", "itertools 0.12.0", "md-5", - "parking_lot 0.12.1", + "parking_lot 0.12.2", "percent-encoding", "quick-xml", "rand", @@ -2460,9 +2460,9 @@ dependencies = [ [[package]] name = "parking_lot" -version = "0.12.1" +version = "0.12.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3742b2c103b9f06bc9fff0a37ff4912935851bee6d36f3c02bcc755bcfec228f" +checksum = "7e4af0ca4f6caed20e900d564c242b8e5d4903fdacf31d3daf527b66fe6f42fb" dependencies = [ "lock_api", "parking_lot_core 0.9.9", @@ -4298,7 +4298,7 @@ dependencies = [ "lazy_static", "libc", "object_store", - "parking_lot 0.12.1", + "parking_lot 0.12.2", "petgraph", "pin-project-lite", "prost 0.12.3", @@ -4472,6 +4472,7 @@ dependencies = [ "opentelemetry", "opentelemetry-otlp", "opentelemetry_sdk", + "parking_lot 0.12.2", "pin-project-lite", "prost 0.12.3", "prost-build", diff --git a/tvix/Cargo.nix b/tvix/Cargo.nix index 74794b834cee..32f0c8c8e97d 100644 --- a/tvix/Cargo.nix +++ b/tvix/Cargo.nix @@ -7112,7 +7112,7 @@ rec { } { name = "parking_lot"; - packageId = "parking_lot 0.12.1"; + packageId = "parking_lot 0.12.2"; } { name = "percent-encoding"; @@ -7683,11 +7683,11 @@ rec { }; resolvedDefaultFeatures = [ "default" ]; }; - "parking_lot 0.12.1" = rec { + "parking_lot 0.12.2" = rec { crateName = "parking_lot"; - version = "0.12.1"; - edition = "2018"; - sha256 = "13r2xk7mnxfc5g0g6dkdxqdqad99j7s7z8zhzz4npw5r0g0v4hip"; + version = "0.12.2"; + edition = "2021"; + sha256 = "1ys2dzz6cysjmwyivwxczl1ljpcf5cj4qmhdj07d5bkc9z5g0jky"; authors = [ "Amanieu d'Antras " ]; @@ -13697,7 +13697,7 @@ rec { } { name = "parking_lot"; - packageId = "parking_lot 0.12.1"; + packageId = "parking_lot 0.12.2"; } { name = "petgraph"; @@ -14430,6 +14430,10 @@ rec { optional = true; features = [ "rt-tokio" ]; } + { + name = "parking_lot"; + packageId = "parking_lot 0.12.2"; + } { name = "pin-project-lite"; packageId = "pin-project-lite"; diff --git a/tvix/castore/src/blobservice/memory.rs b/tvix/castore/src/blobservice/memory.rs index 25eec334de60..873d06b461de 100644 --- a/tvix/castore/src/blobservice/memory.rs +++ b/tvix/castore/src/blobservice/memory.rs @@ -1,9 +1,7 @@ +use parking_lot::RwLock; use std::io::{self, Cursor, Write}; use std::task::Poll; -use std::{ - collections::HashMap, - sync::{Arc, RwLock}, -}; +use std::{collections::HashMap, sync::Arc}; use tonic::async_trait; use tracing::instrument; @@ -19,13 +17,13 @@ pub struct MemoryBlobService { impl BlobService for MemoryBlobService { #[instrument(skip_all, ret, err, fields(blob.digest=%digest))] async fn has(&self, digest: &B3Digest) -> io::Result { - let db = self.db.read().unwrap(); + let db = self.db.read(); Ok(db.contains_key(digest)) } #[instrument(skip_all, err, fields(blob.digest=%digest))] async fn open_read(&self, digest: &B3Digest) -> io::Result>> { - let db = self.db.read().unwrap(); + let db = self.db.read(); match db.get(digest).map(|x| Cursor::new(x.clone())) { Some(result) => Ok(Some(Box::new(result))), @@ -109,24 +107,16 @@ impl BlobWriter for MemoryBlobWriter { } else { let (buf, hasher) = self.writers.take().unwrap(); - // We know self.hasher is doing blake3 hashing, so this won't fail. let digest: B3Digest = hasher.finalize().as_bytes().into(); // Only insert if the blob doesn't already exist. - let db = self.db.read().map_err(|e| { - io::Error::new(io::ErrorKind::BrokenPipe, format!("RwLock poisoned: {}", e)) - })?; + let mut db = self.db.upgradable_read(); if !db.contains_key(&digest) { - // drop the read lock, so we can open for writing. - drop(db); - // open the database for writing. - let mut db = self.db.write().map_err(|e| { - io::Error::new(io::ErrorKind::BrokenPipe, format!("RwLock poisoned: {}", e)) - })?; - - // and put buf in there. This will move buf out. - db.insert(digest.clone(), buf); + db.with_upgraded(|db| { + // and put buf in there. This will move buf out. + db.insert(digest.clone(), buf); + }); } self.digest = Some(digest.clone()); diff --git a/tvix/crate-hashes.json b/tvix/crate-hashes.json index ca45e4317698..2c1e740cb9b1 100644 --- a/tvix/crate-hashes.json +++ b/tvix/crate-hashes.json @@ -1,4 +1,4 @@ { - "bigtable_rs 0.2.9 (git+https://github.com/flokli/bigtable_rs?rev=0af404741dfc40eb9fa99cf4d4140a09c5c20df7#0af404741dfc40eb9fa99cf4d4140a09c5c20df7)": "1njjam1lx2xlnm7a41lga8601vmjgqz0fvc77x24gd04pc7avxll", - "wu-manber 0.1.0 (git+https://github.com/tvlfyi/wu-manber.git#0d5b22bea136659f7de60b102a7030e0daaa503d)": "1zhk83lbq99xzyjwphv2qrb8f8qgfqwa5bbbvyzm0z0bljsjv0pd" + "git+https://github.com/flokli/bigtable_rs?rev=0af404741dfc40eb9fa99cf4d4140a09c5c20df7#0.2.9": "1njjam1lx2xlnm7a41lga8601vmjgqz0fvc77x24gd04pc7avxll", + "git+https://github.com/tvlfyi/wu-manber.git#wu-manber@0.1.0": "1zhk83lbq99xzyjwphv2qrb8f8qgfqwa5bbbvyzm0z0bljsjv0pd" } \ No newline at end of file diff --git a/tvix/store/Cargo.toml b/tvix/store/Cargo.toml index bf04e9a1fbdf..b362a87ffb1d 100644 --- a/tvix/store/Cargo.toml +++ b/tvix/store/Cargo.toml @@ -42,6 +42,7 @@ url = "2.4.0" walkdir = "2.4.0" reqwest = { version = "0.11.22", features = ["rustls-tls-native-roots", "stream"], default-features = false } lru = "0.12.3" +parking_lot = "0.12.2" [dependencies.tonic-reflection] optional = true -- cgit 1.4.1