From 25c6660a59c621de1ec406e4f9b2ccfbd21e1b90 Mon Sep 17 00:00:00 2001 From: cloudhead Date: Mon, 13 May 2024 14:30:41 +0200 Subject: [PATCH] node: Use inventory cache for checking missing Use the cache when checking whether we're missing an inventory. This is a lot faster. --- radicle-node/src/bounded.rs | 2 +- radicle-node/src/service.rs | 21 ++++++++++----------- radicle/src/storage.rs | 14 ++++++++++++++ radicle/src/storage/git.rs | 29 ++++++++++++++++++++++++++++- radicle/src/test/storage.rs | 19 +++++++++++++++++++ 5 files changed, 72 insertions(+), 13 deletions(-) diff --git a/radicle-node/src/bounded.rs b/radicle-node/src/bounded.rs index c1bb3379..6b9b3c25 100644 --- a/radicle-node/src/bounded.rs +++ b/radicle-node/src/bounded.rs @@ -159,7 +159,7 @@ impl BoundedVec { self.v } - /// Calls [`Vec::Drain`]. + /// Calls [`std::vec::Drain`]. pub fn drain>(&mut self, range: R) -> std::vec::Drain { self.v.drain(range) } diff --git a/radicle-node/src/service.rs b/radicle-node/src/service.rs index 396c3404..085bd553 100644 --- a/radicle-node/src/service.rs +++ b/radicle-node/src/service.rs @@ -1431,6 +1431,9 @@ where } } + let inventory = self.storage.inventory_ref(); + let mut missing = Vec::new(); + for id in message.inventory.as_slice() { // TODO: Move this out (good luck with the borrow checker). if let Some(sess) = self.sessions.get_mut(announcer) { @@ -1448,22 +1451,18 @@ where ) { // Only if we do not have the repository locally do we fetch here. // If we do have it, only fetch after receiving a ref announcement. - match self.storage.contains(id) { - Ok(true) => { - // Do nothing. - } - Ok(false) => { - debug!(target: "service", "Missing seeded inventory {id}; initiating fetch.."); - self.fetch(*id, *announcer, FETCH_TIMEOUT, None); - } - Err(e) => { - error!(target: "service", "Error checking local inventory for {id}: {e}"); - } + if !inventory.contains(id) { + missing.push(*id); } } } } + drop(inventory); + for rid in missing { + debug!(target: "service", "Missing seeded inventory {rid}; initiating fetch.."); + self.fetch(rid, *announcer, FETCH_TIMEOUT, None); + } return Ok(relay); } AnnouncementMessage::Refs(message) => { diff --git a/radicle/src/storage.rs b/radicle/src/storage.rs index 0d65bb7e..3f9673c3 100644 --- a/radicle/src/storage.rs +++ b/radicle/src/storage.rs @@ -402,6 +402,9 @@ impl Deref for Remote { /// Read-only operations on a storage instance. pub trait ReadStorage { type Repository: ReadRepository; + type InventoryRef<'a>: Deref + where + Self: 'a; /// Get user info for this storage. fn info(&self) -> &UserInfo; @@ -414,6 +417,9 @@ pub trait ReadStorage { /// Get the inventory of repositories hosted under this storage. /// This function should typically only return public repositories. fn inventory(&self) -> Result; + /// Return a reference to the inventory. Note that this function may hold a + /// lock to the inventory. + fn inventory_ref<'a, 's: 'a>(&'s self) -> Self::InventoryRef<'a>; /// Return all repositories (public and private). fn repositories(&self) -> Result>, Error>; /// Insert this repository into the inventory. @@ -658,6 +664,7 @@ where S: ReadStorage + 'static, { type Repository = S::Repository; + type InventoryRef<'a> = S::InventoryRef<'a> where T: 'a; fn info(&self) -> &UserInfo { self.deref().info() @@ -683,6 +690,13 @@ where self.deref().inventory() } + fn inventory_ref<'a, 's: 'a>(&'s self) -> Self::InventoryRef<'a> + where + T: 'a, + { + self.deref().inventory_ref() + } + fn get(&self, rid: RepoId) -> Result>, RepositoryError> { self.deref().get(rid) } diff --git a/radicle/src/storage/git.rs b/radicle/src/storage/git.rs index 9a196b84..4ec7e8e1 100644 --- a/radicle/src/storage/git.rs +++ b/radicle/src/storage/git.rs @@ -5,7 +5,7 @@ pub mod transport; use std::collections::{BTreeMap, BTreeSet, HashMap}; use std::ops::{Deref, DerefMut}; use std::path::{Path, PathBuf}; -use std::sync::{Arc, Mutex}; +use std::sync::{Arc, Mutex, MutexGuard}; use std::{fs, io}; use crypto::{Signer, Verified}; @@ -72,6 +72,23 @@ impl<'a> TryFrom> for Ref { } } +/// A reference to a locked inventory. +pub struct InventoryLock<'a> { + guard: MutexGuard<'a, Option>>, +} + +impl<'a> Deref for InventoryLock<'a> { + type Target = BTreeSet; + + #[track_caller] + fn deref(&self) -> &Self::Target { + match *self.guard { + Some(ref cache) => cache, + None => panic!("InventoryLock::deref: inventory cache was not initialized"), + } + } +} + #[derive(Debug, Clone)] pub struct Storage { path: PathBuf, @@ -82,6 +99,7 @@ pub struct Storage { impl ReadStorage for Storage { type Repository = Repository; + type InventoryRef<'a> = InventoryLock<'a>; fn info(&self) -> &UserInfo { &self.info @@ -103,6 +121,15 @@ impl ReadStorage for Storage { Ok(false) } + fn inventory_ref<'a, 's: 'a>(&'s self) -> InventoryLock<'a> { + let guard = self + .inventory + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + + InventoryLock { guard } + } + fn inventory(&self) -> Result { let mut cache = self .inventory diff --git a/radicle/src/test/storage.rs b/radicle/src/test/storage.rs index d3f84883..c514da41 100644 --- a/radicle/src/test/storage.rs +++ b/radicle/src/test/storage.rs @@ -56,8 +56,21 @@ impl MockStorage { } } +pub struct InventoryLock { + inner: Inventory, +} + +impl std::ops::Deref for InventoryLock { + type Target = BTreeSet; + + fn deref(&self) -> &Self::Target { + &self.inner + } +} + impl ReadStorage for MockStorage { type Repository = MockRepository; + type InventoryRef<'a> = InventoryLock; fn info(&self) -> &git::UserInfo { &self.info @@ -79,6 +92,12 @@ impl ReadStorage for MockStorage { Ok(self.repos.keys().cloned().collect::>()) } + fn inventory_ref<'a, 's: 'a>(&'s self) -> Self::InventoryRef<'a> { + InventoryLock { + inner: self.inventory().unwrap(), + } + } + fn insert(&self, _rid: RepoId) {} fn repository(&self, rid: RepoId) -> Result {