From 87930a141e6728f903dee99b91141e3d8a02f10a Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Wed, 1 Dec 2021 21:18:02 +0800 Subject: [PATCH] =?UTF-8?q?refactor!:=20remove=20pack-cache=20from=20`Find?= =?UTF-8?q?::try=5Ffind(=E2=80=A6)`=20(#266)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With the new architecture this can be an implementation detail without forcing it to be Sync. --- experiments/diffing/src/main.rs | 19 ++++----- experiments/odb-redesign/src/lib.rs | 3 +- experiments/traversal/src/main.rs | 35 ++++++---------- git-diff/tests/visit/mod.rs | 20 +++++----- git-odb/src/store/linked/find.rs | 6 +-- git-odb/tests/odb/store/linked/mod.rs | 4 +- git-pack/src/data/output/count/objects/mod.rs | 40 +++++++++---------- .../src/data/output/entry/iter_from_counts.rs | 18 +++------ git-pack/src/find_traits.rs | 16 +++----- git-pack/src/lib.rs | 5 ++- .../pack/data/output/count_and_entries.rs | 11 ++--- git-ref/tests/file/reference.rs | 2 +- git-repository/src/easy/ext/object.rs | 13 +----- git-repository/src/easy/oid.rs | 9 +---- git-repository/src/easy/reference/iter.rs | 7 +--- git-repository/src/easy/reference/mod.rs | 6 +-- git-traverse/tests/commit/mod.rs | 6 +-- git-traverse/tests/tree/mod.rs | 18 +++------ gitoxide-core/src/hours.rs | 9 ++--- gitoxide-core/src/pack/create.rs | 33 +++++++-------- 20 files changed, 103 insertions(+), 177 deletions(-) diff --git a/experiments/diffing/src/main.rs b/experiments/diffing/src/main.rs index 94d30042696..383e88243f9 100644 --- a/experiments/diffing/src/main.rs +++ b/experiments/diffing/src/main.rs @@ -38,11 +38,7 @@ fn main() -> anyhow::Result<()> { let start = Instant::now(); let all_commits = commit_id - .ancestors(|oid, buf| { - db.find_commit_iter(oid, buf, &mut odb::pack::cache::Never) - .ok() - .map(|t| t.0) - }) + .ancestors(|oid, buf| db.find_commit_iter(oid, buf).ok().map(|t| t.0)) .collect::, _>>()?; let num_diffs = all_commits.len(); let elapsed = start.elapsed(); @@ -67,7 +63,6 @@ fn main() -> anyhow::Result<()> { buf: &'b mut Vec, obj_cache: &mut memory_lru::MemoryLruCache, db: &odb::linked::Store, - pack_cache: &mut impl odb::pack::cache::DecodeEntry, ) -> Option> { let oid = oid.to_owned(); match obj_cache.get(&oid) { @@ -77,7 +72,7 @@ fn main() -> anyhow::Result<()> { Some(git_repository::objs::Data::new(*kind, buf)) } None => { - let obj = db.find(oid, buf, pack_cache).ok(); + let obj = db.find(oid, buf).ok(); if let Some((obj, _location)) = &obj { obj_cache.insert( oid, @@ -96,10 +91,11 @@ fn main() -> anyhow::Result<()> { let num_deltas = do_gitoxide_tree_diff( &all_commits, || { - let mut pack_cache = odb::pack::cache::lru::MemoryCappedHashmap::new(cache_size()); + // TODO: configure the ODB to actually use it + let _pack_cache = odb::pack::cache::lru::MemoryCappedHashmap::new(cache_size()); let db = &db; let mut obj_cache = memory_lru::MemoryLruCache::new(cache_size()); - move |oid, buf: &mut Vec| find_with_lru_mem_cache(oid, buf, &mut obj_cache, db, &mut pack_cache) + move |oid, buf: &mut Vec| find_with_lru_mem_cache(oid, buf, &mut obj_cache, db) }, Computation::MultiThreaded, )?; @@ -119,10 +115,11 @@ fn main() -> anyhow::Result<()> { let num_deltas = do_gitoxide_tree_diff( &all_commits, || { - let mut pack_cache = odb::pack::cache::lru::MemoryCappedHashmap::new(cache_size()); + // TODO: configure the ODB to actually use it + let mut _pack_cache = odb::pack::cache::lru::MemoryCappedHashmap::new(cache_size()); let db = &db; let mut obj_cache = memory_lru::MemoryLruCache::new(cache_size()); - move |oid, buf: &mut Vec| find_with_lru_mem_cache(oid, buf, &mut obj_cache, db, &mut pack_cache) + move |oid, buf: &mut Vec| find_with_lru_mem_cache(oid, buf, &mut obj_cache, db) }, Computation::SingleThreaded, )?; diff --git a/experiments/odb-redesign/src/lib.rs b/experiments/odb-redesign/src/lib.rs index b51e5c47e58..81608d21cb0 100644 --- a/experiments/odb-redesign/src/lib.rs +++ b/experiments/odb-redesign/src/lib.rs @@ -62,7 +62,7 @@ mod odb { use std::path::PathBuf; use git_hash::oid; - use git_odb::pack::{bundle::Location, cache::DecodeEntry, find::Entry, Bundle}; + use git_odb::pack::{bundle::Location, find::Entry, Bundle}; use crate::{ features, @@ -581,7 +581,6 @@ mod odb { &self, id: impl AsRef, buffer: &'a mut Vec, - pack_cache: &mut impl DecodeEntry, ) -> Result, Option)>, Self::Error> { // TODO: if the generation changes, we need to clear the pack-cache as it depends on pack-ids. // Can we simplify this so it's more obvious what generation does? They must remain stable no matter what diff --git a/experiments/traversal/src/main.rs b/experiments/traversal/src/main.rs index 290e8c892f2..7e62a7f17a6 100644 --- a/experiments/traversal/src/main.rs +++ b/experiments/traversal/src/main.rs @@ -36,11 +36,7 @@ fn main() -> anyhow::Result<()> { let start = Instant::now(); let all_commits = commit_id - .ancestors(|oid, buf| { - db.find_commit_iter(oid, buf, &mut odb::pack::cache::Never) - .ok() - .map(|t| t.0) - }) + .ancestors(|oid, buf| db.find_commit_iter(oid, buf).ok().map(|t| t.0)) .collect::, _>>()?; let elapsed = start.elapsed(); println!( @@ -143,13 +139,13 @@ fn main() -> anyhow::Result<()> { fn do_gitoxide_commit_graph_traversal( tip: ObjectId, db: &odb::linked::Store, - new_cache: impl FnOnce() -> C, + // TODO: make use of the cache + _new_cache: impl FnOnce() -> C, ) -> anyhow::Result where C: odb::pack::cache::DecodeEntry, { - let mut cache = new_cache(); - let ancestors = tip.ancestors(|oid, buf| db.find_commit_iter(oid, buf, &mut cache).ok().map(|t| t.0)); + let ancestors = tip.ancestors(|oid, buf| db.find_commit_iter(oid, buf).ok().map(|t| t.0)); let mut commits = 0; for commit_id in ancestors { let _ = commit_id?; @@ -167,7 +163,7 @@ enum Computation { fn do_gitoxide_tree_dag_traversal( commits: &[ObjectId], db: &odb::linked::Store, - new_cache: impl Fn() -> C + Sync + Send, + _new_cache: impl Fn() -> C + Sync + Send, // TODO: use this cache again mode: Computation, ) -> anyhow::Result<(usize, u64)> where @@ -202,7 +198,6 @@ where } } - let mut cache = new_cache(); let mut buf = Vec::new(); let mut buf2 = Vec::new(); let mut state = tree::breadthfirst::State::default(); @@ -211,18 +206,14 @@ where for commit in commits { let tree_id = db - .try_find(commit, &mut buf, &mut cache)? + .try_find(commit, &mut buf)? .and_then(|(o, _l)| o.try_into_commit_iter().and_then(|mut c| c.tree_id())) .expect("commit as starting point"); let mut count = Count { entries: 0, seen }; - db.find_tree_iter(tree_id, &mut buf2, &mut cache)?.0.traverse( + db.find_tree_iter(tree_id, &mut buf2)?.0.traverse( &mut state, - |oid, buf| { - db.find(oid, buf, &mut cache) - .ok() - .and_then(|(o, _l)| o.try_into_tree_iter()) - }, + |oid, buf| db.find(oid, buf).ok().and_then(|(o, _l)| o.try_into_tree_iter()), &mut count, )?; entries += count.entries as u64; @@ -264,28 +255,26 @@ where .into_par_iter() .try_for_each_init::<_, _, _, anyhow::Result<_>>( { - let new_cache = &new_cache; let seen = &seen; move || { ( Count { entries: 0, seen }, Vec::::new(), Vec::::new(), - new_cache(), tree::breadthfirst::State::default(), ) } }, - |(count, buf, buf2, cache, state), commit| { + |(count, buf, buf2, state), commit| { let tid = db - .find_commit_iter(commit, buf, cache)? + .find_commit_iter(commit, buf)? .0 .tree_id() .expect("commit as starting point"); count.entries = 0; - db.find_tree_iter(tid, buf2, cache)?.0.traverse( + db.find_tree_iter(tid, buf2)?.0.traverse( state, - |oid, buf| db.find_tree_iter(oid, buf, cache).ok().map(|t| t.0), + |oid, buf| db.find_tree_iter(oid, buf).ok().map(|t| t.0), count, )?; entries.fetch_add(count.entries as u64, std::sync::atomic::Ordering::Relaxed); diff --git a/git-diff/tests/visit/mod.rs b/git-diff/tests/visit/mod.rs index cf0e43fe058..c8dd728b119 100644 --- a/git-diff/tests/visit/mod.rs +++ b/git-diff/tests/visit/mod.rs @@ -3,7 +3,7 @@ mod changes { use git_diff::tree::{recorder, recorder::Change::*}; use git_hash::{oid, ObjectId}; use git_object::{bstr::ByteSlice, tree::EntryMode, TreeRefIter}; - use git_odb::{linked, pack, pack::Find}; + use git_odb::{linked, pack::Find}; use crate::hex_to_id; @@ -24,7 +24,7 @@ mod changes { buf: &'a mut Vec, ) -> crate::Result> { let tree_id = db - .try_find(commit, buf, &mut pack::cache::Never)? + .try_find(commit, buf)? .ok_or_else(|| format!("start commit {:?} to be present", commit))? .0 .decode()? @@ -33,7 +33,7 @@ mod changes { .tree(); Ok(db - .try_find(tree_id, buf, &mut pack::cache::Never)? + .try_find(tree_id, buf)? .expect("main tree present") .0 .try_into_tree_iter() @@ -52,7 +52,7 @@ mod changes { rhs_tree, git_diff::tree::State::default(), |oid, buf| { - db.try_find(oid, buf, &mut pack::cache::Never) + db.try_find(oid, buf) .ok() .flatten() .and_then(|obj| obj.0.try_into_tree_iter()) @@ -66,7 +66,7 @@ mod changes { let mut buf = Vec::new(); let (main_tree_id, parent_commit_id) = { let commit = db - .try_find(commit_id, &mut buf, &mut pack::cache::Never)? + .try_find(commit_id, &mut buf)? .ok_or_else(|| format!("start commit {:?} to be present", commit_id))? .0 .decode()? @@ -79,7 +79,7 @@ mod changes { }) }; let current_tree = db - .try_find(main_tree_id, &mut buf, &mut pack::cache::Never)? + .try_find(main_tree_id, &mut buf)? .expect("main tree present") .0 .try_into_tree_iter() @@ -87,11 +87,11 @@ mod changes { let mut buf2 = Vec::new(); let previous_tree: Option<_> = { parent_commit_id - .and_then(|id| db.try_find(id, &mut buf2, &mut pack::cache::Never).ok().flatten()) + .and_then(|id| db.try_find(id, &mut buf2).ok().flatten()) .and_then(|(c, _l)| c.decode().ok()) .and_then(|c| c.into_commit()) .map(|c| c.tree()) - .and_then(|tree| db.try_find(tree, &mut buf2, &mut pack::cache::Never).ok().flatten()) + .and_then(|tree| db.try_find(tree, &mut buf2).ok().flatten()) .and_then(|(tree, _)| tree.try_into_tree_iter()) }; @@ -100,7 +100,7 @@ mod changes { current_tree, &mut git_diff::tree::State::default(), |oid, buf| { - db.try_find(oid, buf, &mut pack::cache::Never) + db.try_find(oid, buf) .ok() .flatten() .and_then(|(obj, _)| obj.try_into_tree_iter()) @@ -134,7 +134,7 @@ mod changes { let head = head_of(db); commit::Ancestors::new(Some(head), commit::ancestors::State::default(), |oid, buf| { - db.try_find(oid, buf, &mut pack::cache::Never) + db.try_find(oid, buf) .ok() .flatten() .and_then(|t| t.0.try_into_commit_iter()) diff --git a/git-odb/src/store/linked/find.rs b/git-odb/src/store/linked/find.rs index 8c88d5a05a3..bed875a648c 100644 --- a/git-odb/src/store/linked/find.rs +++ b/git-odb/src/store/linked/find.rs @@ -27,7 +27,6 @@ impl crate::pack::Find for linked::Store { &self, id: impl AsRef, buffer: &'a mut Vec, - pack_cache: &mut impl pack::cache::DecodeEntry, ) -> Result, Option)>, Self::Error> { let id = id.as_ref(); for db in self.dbs.iter() { @@ -37,7 +36,7 @@ impl crate::pack::Find for linked::Store { entry_index, }) => { return db - .internal_get_packed_object_by_index(pack_id, entry_index, buffer, pack_cache) + .internal_get_packed_object_by_index(pack_id, entry_index, buffer, &mut git_pack::cache::Never) .map(|(obj, location)| Some((obj, Some(location)))) .map_err(Into::into); } @@ -117,9 +116,8 @@ impl crate::pack::Find for &linked::Store { &self, id: impl AsRef, buffer: &'a mut Vec, - pack_cache: &mut impl pack::cache::DecodeEntry, ) -> Result, Option)>, Self::Error> { - (*self).try_find(id, buffer, pack_cache) + (*self).try_find(id, buffer) } fn location_by_oid(&self, id: impl AsRef, buf: &mut Vec) -> Option { diff --git a/git-odb/tests/odb/store/linked/mod.rs b/git-odb/tests/odb/store/linked/mod.rs index b75256f46c3..94884387b36 100644 --- a/git-odb/tests/odb/store/linked/mod.rs +++ b/git-odb/tests/odb/store/linked/mod.rs @@ -48,7 +48,7 @@ mod iter { } mod locate { - use git_odb::{linked::Store, pack}; + use git_odb::linked::Store; use git_pack::Find; use crate::{hex_to_id, odb::store::linked::db}; @@ -56,7 +56,7 @@ mod locate { fn can_locate(db: &Store, hex_id: &str) { let mut buf = vec![]; assert!(db - .try_find(hex_to_id(hex_id), &mut buf, &mut pack::cache::Never) + .try_find(hex_to_id(hex_id), &mut buf) .expect("no read error") .is_some()); } diff --git a/git-pack/src/data/output/count/objects/mod.rs b/git-pack/src/data/output/count/objects/mod.rs index d2c00084afb..cc8aa9c4359 100644 --- a/git-pack/src/data/output/count/objects/mod.rs +++ b/git-pack/src/data/output/count/objects/mod.rs @@ -27,7 +27,7 @@ pub type Result = std::result::Result<(Vec, Outcome), Err /// A [`Count`][output::Count] object maintains enough state to greatly accelerate future access of packed objects. /// /// * `db` - the object store to use for accessing objects. -/// * `make_cache` - a function to create thread-local pack caches +/// * `make_object_cache` - a function to create thread-local object cache /// * `objects_ids` /// * A list of objects ids to add to the pack. Duplication checks are performed so no object is ever added to a pack twice. /// * Objects may be expanded based on the provided [`options`][Options] @@ -37,9 +37,9 @@ pub type Result = std::result::Result<(Vec, Outcome), Err /// * A flag that is set to true if the operation should stop /// * `options` /// * more configuration -pub fn objects( +pub fn objects( db: Find, - make_caches: impl Fn() -> (PackCache, ObjectCache) + Send + Clone, + make_object_cache: impl Fn() -> ObjectCache + Send + Clone, objects_ids: Iter, progress: impl Progress, should_interrupt: &AtomicBool, @@ -55,7 +55,6 @@ where Iter: Iterator> + Send, Oid: Into + Send, IterErr: std::error::Error + Send, - PackCache: crate::cache::DecodeEntry, ObjectCache: crate::cache::Object, { let lower_bound = objects_ids.size_hint().0; @@ -79,9 +78,9 @@ where let progress = Arc::clone(&progress); move |n| { ( - Vec::new(), // object data buffer - Vec::new(), // object data buffer 2 to hold two objects at a time - make_caches(), // cache to speed up pack and tree traveral operations + Vec::new(), // object data buffer + Vec::new(), // object data buffer 2 to hold two objects at a time + make_object_cache(), // cache to speed up pack and tree traveral operations { let mut p = progress.lock().add_child(format!("thread {}", n)); p.init(None, git_features::progress::count("objects")); @@ -91,7 +90,7 @@ where } }, { - move |oids: Vec>, (buf1, buf2, (pack_cache, object_cache), progress)| { + move |oids: Vec>, (buf1, buf2, object_cache, progress)| { expand::this( &db, input_object_expansion, @@ -99,7 +98,6 @@ where oids, buf1, buf2, - pack_cache, object_cache, progress, should_interrupt, @@ -114,7 +112,7 @@ where /// Like [`objects()`] but using a single thread only to mostly save on the otherwise required overhead. pub fn objects_unthreaded( db: Find, - (pack_cache, obj_cache): (&mut impl crate::cache::DecodeEntry, &mut impl crate::cache::Object), + obj_cache: &mut impl crate::cache::Object, object_ids: impl Iterator>, mut progress: impl Progress, should_interrupt: &AtomicBool, @@ -135,7 +133,6 @@ where object_ids, &mut buf1, &mut buf2, - pack_cache, obj_cache, &mut progress, should_interrupt, @@ -168,7 +165,6 @@ mod expand { oids: impl IntoIterator>, buf1: &mut Vec, buf2: &mut Vec, - cache: &mut impl crate::cache::DecodeEntry, obj_cache: &mut impl crate::cache::Object, progress: &mut impl Progress, should_interrupt: &AtomicBool, @@ -196,7 +192,7 @@ mod expand { } let id = id.map(|oid| oid.into()).map_err(Error::InputIteration)?; - let (obj, location) = db.find(id, buf1, cache)?; + let (obj, location) = db.find(id, buf1)?; stats.input_objects += 1; match input_object_expansion { TreeAdditionsComparedToAncestor => { @@ -213,7 +209,7 @@ mod expand { id = TagRefIter::from_bytes(obj.data) .target_id() .expect("every tag has a target"); - let tmp = db.find(id, buf1, cache)?; + let tmp = db.find(id, buf1)?; obj = tmp.0; location = tmp.1; @@ -235,7 +231,7 @@ mod expand { Err(err) => return Err(Error::CommitDecode(err)), } } - let (obj, location) = db.find(tree_id, buf1, cache)?; + let (obj, location) = db.find(tree_id, buf1)?; push_obj_count_unique( &mut out, seen_objs, &tree_id, location, progress, stats, true, ); @@ -249,7 +245,7 @@ mod expand { &mut tree_traversal_state, |oid, buf| { stats.decoded_objects += 1; - match db.find(oid, buf, cache).ok() { + match db.find(oid, buf).ok() { Some((obj, location)) => { progress.inc(); stats.expanded_objects += 1; @@ -266,7 +262,7 @@ mod expand { } else { for commit_id in &parent_commit_ids { let parent_tree_id = { - let (parent_commit_obj, location) = db.find(commit_id, buf2, cache)?; + let (parent_commit_obj, location) = db.find(commit_id, buf2)?; push_obj_count_unique( &mut out, seen_objs, commit_id, location, progress, stats, true, @@ -276,7 +272,7 @@ mod expand { .expect("every commit has a tree") }; let parent_tree = { - let (parent_tree_obj, location) = db.find(parent_tree_id, buf2, cache)?; + let (parent_tree_obj, location) = db.find(parent_tree_id, buf2)?; push_obj_count_unique( &mut out, seen_objs, @@ -299,7 +295,7 @@ mod expand { let id = oid.to_owned(); match obj_cache.get(&id, buf) { Some(_kind) => git_object::TreeRefIter::from_bytes(buf).into(), - None => match db.find_tree_iter(oid, buf, cache).ok() { + None => match db.find_tree_iter(oid, buf).ok() { Some(_) => { obj_cache.put(id, git_object::Kind::Tree, buf); git_object::TreeRefIter::from_bytes(buf).into() @@ -336,7 +332,7 @@ mod expand { &mut tree_traversal_state, |oid, buf| { stats.decoded_objects += 1; - match db.find(oid, buf, cache).ok() { + match db.find(oid, buf).ok() { Some((obj, location)) => { progress.inc(); stats.expanded_objects += 1; @@ -359,7 +355,7 @@ mod expand { .tree_id() .expect("every commit has a tree"); stats.expanded_objects += 1; - obj = db.find(id, buf1, cache)?; + obj = db.find(id, buf1)?; continue; } Blob => break, @@ -368,7 +364,7 @@ mod expand { .target_id() .expect("every tag has a target"); stats.expanded_objects += 1; - obj = db.find(id, buf1, cache)?; + obj = db.find(id, buf1)?; continue; } } diff --git a/git-pack/src/data/output/entry/iter_from_counts.rs b/git-pack/src/data/output/entry/iter_from_counts.rs index 0b1f682dd18..94f20b48099 100644 --- a/git-pack/src/data/output/entry/iter_from_counts.rs +++ b/git-pack/src/data/output/entry/iter_from_counts.rs @@ -22,11 +22,6 @@ use crate::data::{output, output::ChunkId}; /// /// ## Discussion /// -/// ### Caches -/// -/// `make_cache` is only suitable for speeding up cache access of blobs as these are not looked up during counting anymore - it only -/// sees trees as only these are needed for traversal/object counting. -/// /// ### Advantages /// /// * Begins writing immediately and supports back-pressure. @@ -38,10 +33,9 @@ use crate::data::{output, output::ChunkId}; /// so with minimal overhead (especially compared to `gix index-from-pack`)~~ Probably works now by chaining Iterators /// or keeping enough state to write a pack and then generate an index with recorded data. /// -pub fn iter_from_counts( +pub fn iter_from_counts( mut counts: Vec, db: Find, - make_cache: impl Fn() -> Cache + Send + Clone + 'static, mut progress: impl Progress, Options { version, @@ -55,7 +49,6 @@ pub fn iter_from_counts( where Find: crate::Find + Send + Clone + 'static, ::Error: Send, - Cache: crate::cache::DecodeEntry, { assert!( matches!(version, crate::data::Version::V2), @@ -152,15 +145,14 @@ where let progress = Arc::clone(&progress); move |n| { ( - Vec::new(), // object data buffer - make_cache(), // cache to speed up pack operations + Vec::new(), // object data buffer progress.lock().add_child(format!("thread {}", n)), ) } }, { let counts = Arc::clone(&counts); - move |(chunk_id, chunk_range): (ChunkId, std::ops::Range), (buf, cache, progress)| { + move |(chunk_id, chunk_range): (ChunkId, std::ops::Range), (buf, progress)| { let mut out = Vec::new(); let chunk = &counts[chunk_range]; let mut stats = Outcome::default(); @@ -220,7 +212,7 @@ where stats.objects_copied_from_pack += 1; entry } - None => match db.try_find(count.id, buf, cache).map_err(Error::FindExisting)? { + None => match db.try_find(count.id, buf).map_err(Error::FindExisting)? { Some((obj, _location)) => { stats.decoded_and_recompressed_objects += 1; output::Entry::from_data(count, &obj) @@ -232,7 +224,7 @@ where }, } } - None => match db.try_find(count.id, buf, cache).map_err(Error::FindExisting)? { + None => match db.try_find(count.id, buf).map_err(Error::FindExisting)? { Some((obj, _location)) => { stats.decoded_and_recompressed_objects += 1; output::Entry::from_data(count, &obj) diff --git a/git-pack/src/find_traits.rs b/git-pack/src/find_traits.rs index 9c4dd4c126c..e94daf84fca 100644 --- a/git-pack/src/find_traits.rs +++ b/git-pack/src/find_traits.rs @@ -25,7 +25,6 @@ pub trait Find { &self, id: impl AsRef, buffer: &'a mut Vec, - pack_cache: &mut impl crate::cache::DecodeEntry, // TODO: remove this one once as soon as it's clear we have thread-local handles ) -> Result, Option)>, Self::Error>; /// Find the packs location where an object with `id` can be found in the database, or `None` if there is no pack @@ -64,10 +63,9 @@ mod ext { &self, id: impl AsRef, buffer: &'a mut Vec, - pack_cache: &mut impl crate::cache::DecodeEntry, ) -> Result<($object_type, Option), find::existing_object::Error> { let id = id.as_ref(); - self.try_find(id, buffer, pack_cache) + self.try_find(id, buffer) .map_err(find::existing_object::Error::Find)? .ok_or_else(|| find::existing_object::Error::NotFound { oid: id.as_ref().to_owned(), @@ -95,10 +93,9 @@ mod ext { &self, id: impl AsRef, buffer: &'a mut Vec, - pack_cache: &mut impl crate::cache::DecodeEntry, ) -> Result<($object_type, Option), find::existing_iter::Error> { let id = id.as_ref(); - self.try_find(id, buffer, pack_cache) + self.try_find(id, buffer) .map_err(find::existing_iter::Error::Find)? .ok_or_else(|| find::existing_iter::Error::NotFound { oid: id.as_ref().to_owned(), @@ -121,11 +118,10 @@ mod ext { &self, id: impl AsRef, buffer: &'a mut Vec, - pack_cache: &mut impl crate::cache::DecodeEntry, ) -> Result<(git_object::Data<'a>, Option), find::existing::Error> { let id = id.as_ref(); - self.try_find(id, buffer, pack_cache) + self.try_find(id, buffer) .map_err(find::existing::Error::Find)? .ok_or_else(|| find::existing::Error::NotFound { oid: id.as_ref().to_owned(), @@ -166,9 +162,8 @@ mod find_impls { &self, id: impl AsRef, buffer: &'a mut Vec, - pack_cache: &mut impl crate::cache::DecodeEntry, ) -> Result, Option)>, Self::Error> { - self.deref().try_find(id, buffer, pack_cache) + self.deref().try_find(id, buffer) } fn location_by_oid(&self, id: impl AsRef, buf: &mut Vec) -> Option { @@ -198,9 +193,8 @@ mod find_impls { &self, id: impl AsRef, buffer: &'a mut Vec, - pack_cache: &mut impl crate::cache::DecodeEntry, ) -> Result, Option)>, Self::Error> { - self.deref().try_find(id, buffer, pack_cache) + self.deref().try_find(id, buffer) } fn location_by_oid(&self, id: impl AsRef, buf: &mut Vec) -> Option { diff --git a/git-pack/src/lib.rs b/git-pack/src/lib.rs index b505ed36e12..a1d100482eb 100755 --- a/git-pack/src/lib.rs +++ b/git-pack/src/lib.rs @@ -13,8 +13,6 @@ //! When traversing all objects in a pack, a _delta tree acceleration structure_ can be built from pack data or an index //! in order to decompress packs in parallel and without any waste. -pub use find_traits::{Find, FindExt}; - /// pub mod bundle; /// A bundle of pack data and the corresponding pack index @@ -32,6 +30,9 @@ pub mod find; pub mod cache; /// pub mod data; + mod find_traits; +pub use find_traits::{Find, FindExt}; + /// pub mod index; diff --git a/git-pack/tests/pack/data/output/count_and_entries.rs b/git-pack/tests/pack/data/output/count_and_entries.rs index 160e28a338c..6626e6c5883 100644 --- a/git-pack/tests/pack/data/output/count_and_entries.rs +++ b/git-pack/tests/pack/data/output/count_and_entries.rs @@ -239,7 +239,7 @@ fn traversals() -> crate::Result { let head = hex_to_id("dfcb5e39ac6eb30179808bbab721e8a28ce1b52e"); let mut commits = commit::Ancestors::new(Some(head), commit::ancestors::State::default(), { let db = Arc::clone(&db); - move |oid, buf| db.find_commit_iter(oid, buf, &mut pack::cache::Never).ok().map(|t| t.0) + move |oid, buf| db.find_commit_iter(oid, buf).ok().map(|t| t.0) }) .map(Result::unwrap) .collect::>(); @@ -250,7 +250,7 @@ fn traversals() -> crate::Result { let deterministic_count_needs_single_thread = Some(1); let (counts, stats) = output::count::objects( db.clone(), - || (pack::cache::Never, pack::cache::object::Never), + || pack::cache::object::Never, commits .into_iter() .chain(std::iter::once(hex_to_id(if take.is_some() { @@ -270,7 +270,7 @@ fn traversals() -> crate::Result { )?; let actual_count = counts.iter().fold(ObjectCount::default(), |mut c, e| { let mut buf = Vec::new(); - if let Some((obj, _location)) = db.find(e.id, &mut buf, &mut pack::cache::Never).ok() { + if let Some((obj, _location)) = db.find(e.id, &mut buf).ok() { c.add(obj.kind); } c @@ -285,7 +285,6 @@ fn traversals() -> crate::Result { let mut entries_iter = output::entry::iter_from_counts( counts, db.clone(), - || pack::cache::Never, progress::Discard, output::entry::iter_from_counts::Options { allow_thin_pack, @@ -373,9 +372,7 @@ fn write_and_verify( Some(tmp_dir.path()), progress::Discard, &should_interrupt, - Some(Box::new(move |oid, buf| { - db.find(oid, buf, &mut git_pack::cache::Never).ok().map(|t| t.0) - })), + Some(Box::new(move |oid, buf| db.find(oid, buf).ok().map(|t| t.0))), pack::bundle::write::Options::default(), )? .data_path diff --git a/git-ref/tests/file/reference.rs b/git-ref/tests/file/reference.rs index a8427aed21f..a25511ab062 100644 --- a/git-ref/tests/file/reference.rs +++ b/git-ref/tests/file/reference.rs @@ -134,7 +134,7 @@ mod peel { let mut r: Reference = store.find_loose("dt1")?.into(); assert_eq!( r.peel_to_id_in_place(&store, |oid, buf| { - odb.try_find(oid, buf, &mut git_odb::pack::cache::Never) + odb.try_find(oid, buf) .map(|obj| obj.map(|(obj, _)| (obj.kind, obj.data))) })?, commit, diff --git a/git-repository/src/easy/ext/object.rs b/git-repository/src/easy/ext/object.rs index cd7e2019d75..615502a67b2 100644 --- a/git-repository/src/easy/ext/object.rs +++ b/git-repository/src/easy/ext/object.rs @@ -43,12 +43,7 @@ pub trait ObjectAccessExt: easy::Access + Sized { return ObjectRef::from_current_buf(id, kind, self).map_err(Into::into); } } - let kind = self - .repo()? - .odb - .find(&id, &mut buf, state.try_borrow_mut_pack_cache()?.deref_mut())? - .0 - .kind; + let kind = self.repo()?.odb.find(&id, &mut buf)?.0.kind; if let Some(c) = object_cache.deref_mut() { c.put(id, kind, &buf); @@ -77,11 +72,7 @@ pub trait ObjectAccessExt: easy::Access + Sized { return Ok(Some(ObjectRef::from_current_buf(id, kind, self)?)); } } - match self - .repo()? - .odb - .try_find(&id, &mut buf, state.try_borrow_mut_pack_cache()?.deref_mut())? - { + match self.repo()?.odb.try_find(&id, &mut buf)? { Some((obj, _location)) => { let kind = obj.kind; drop(obj); diff --git a/git-repository/src/easy/oid.rs b/git-repository/src/easy/oid.rs index e4eec9c9404..523a7fb7b79 100644 --- a/git-repository/src/easy/oid.rs +++ b/git-repository/src/easy/oid.rs @@ -117,14 +117,7 @@ pub mod ancestors { .repo .deref() .odb - .try_find( - oid, - buf, - state - .try_borrow_mut_pack_cache() - .expect("BUG: pack cache is already borrowed") - .deref_mut(), - ) + .try_find(oid, buf) .ok() .flatten() .and_then(|(obj, _location)| obj.try_into_commit_iter()) diff --git a/git-repository/src/easy/reference/iter.rs b/git-repository/src/easy/reference/iter.rs index 074bdc75bcb..56957127144 100644 --- a/git-repository/src/easy/reference/iter.rs +++ b/git-repository/src/easy/reference/iter.rs @@ -1,5 +1,5 @@ //! -use std::{ops::DerefMut, path::Path}; +use std::path::Path; use git_odb::pack::Find; use git_ref::file::ReferenceExt; @@ -79,11 +79,8 @@ where if self.peel { let repo = self.access.repo()?; let state = self.access.state(); - let mut pack_cache = state.try_borrow_mut_pack_cache()?; r.peel_to_id_in_place(&state.refs, |oid, buf| { - repo.odb - .try_find(oid, buf, pack_cache.deref_mut()) - .map(|po| po.map(|(o, _l)| (o.kind, o.data))) + repo.odb.try_find(oid, buf).map(|po| po.map(|(o, _l)| (o.kind, o.data))) }) .map_err(|err| Box::new(err) as Box) .map(|_| r) diff --git a/git-repository/src/easy/reference/mod.rs b/git-repository/src/easy/reference/mod.rs index 4164fed9e02..6659bee7275 100644 --- a/git-repository/src/easy/reference/mod.rs +++ b/git-repository/src/easy/reference/mod.rs @@ -1,5 +1,4 @@ //! -use std::ops::DerefMut; use git_odb::pack::Find; use git_ref::file::ReferenceExt; @@ -63,11 +62,8 @@ where pub fn peel_to_id_in_place(&mut self) -> Result, peel::Error> { let repo = self.access.repo()?; let state = self.access.state(); - let mut pack_cache = state.try_borrow_mut_pack_cache()?; let oid = self.inner.peel_to_id_in_place(&state.refs, |oid, buf| { - repo.odb - .try_find(oid, buf, pack_cache.deref_mut()) - .map(|po| po.map(|(o, _l)| (o.kind, o.data))) + repo.odb.try_find(oid, buf).map(|po| po.map(|(o, _l)| (o.kind, o.data))) })?; Ok(Oid::from_id(oid, self.access)) } diff --git a/git-traverse/tests/commit/mod.rs b/git-traverse/tests/commit/mod.rs index cfd276d61bf..fb1a86808d9 100644 --- a/git-traverse/tests/commit/mod.rs +++ b/git-traverse/tests/commit/mod.rs @@ -1,6 +1,6 @@ mod ancestor { use git_hash::{oid, ObjectId}; - use git_odb::{linked::Store, pack, pack::FindExt}; + use git_odb::{linked::Store, pack::FindExt}; use git_traverse::commit; use crate::hex_to_id; @@ -19,7 +19,7 @@ mod ancestor { commit::Ancestors::filtered( tips, commit::ancestors::State::default(), - move |oid, buf| db.find_commit_iter(oid, buf, &mut pack::cache::Never).ok().map(|t| t.0), + move |oid, buf| db.find_commit_iter(oid, buf).ok().map(|t| t.0), predicate, ) } @@ -45,7 +45,7 @@ mod ancestor { ) -> impl Iterator> { let db = db().expect("db instantiation works as its definitely valid"); commit::Ancestors::new(tips, commit::ancestors::State::default(), move |oid, buf| { - db.find_commit_iter(oid, buf, &mut pack::cache::Never).ok().map(|t| t.0) + db.find_commit_iter(oid, buf).ok().map(|t| t.0) }) .mode(mode) } diff --git a/git-traverse/tests/tree/mod.rs b/git-traverse/tests/tree/mod.rs index 704ad968654..7aaf0b96687 100644 --- a/git-traverse/tests/tree/mod.rs +++ b/git-traverse/tests/tree/mod.rs @@ -1,4 +1,4 @@ -use git_odb::{linked::Store, pack, pack::FindExt}; +use git_odb::{linked::Store, pack::FindExt}; use git_traverse::tree; use crate::hex_to_id; @@ -15,22 +15,14 @@ fn basic_nesting() -> crate::Result<()> { let mut buf = Vec::new(); let mut buf2 = Vec::new(); let mut commit = db - .find_commit_iter( - hex_to_id("85df34aa34848b8138b2b3dcff5fb5c2b734e0ce"), - &mut buf, - &mut pack::cache::Never, - )? + .find_commit_iter(hex_to_id("85df34aa34848b8138b2b3dcff5fb5c2b734e0ce"), &mut buf)? .0; let mut recorder = tree::Recorder::default(); git_traverse::tree::breadthfirst( - db.find_tree_iter( - commit.tree_id().expect("a tree is available in a commit"), - &mut buf2, - &mut pack::cache::Never, - )? - .0, + db.find_tree_iter(commit.tree_id().expect("a tree is available in a commit"), &mut buf2)? + .0, tree::breadthfirst::State::default(), - |oid, buf| db.find_tree_iter(oid, buf, &mut pack::cache::Never).ok().map(|t| t.0), + |oid, buf| db.find_tree_iter(oid, buf).ok().map(|t| t.0), &mut recorder, )?; diff --git a/gitoxide-core/src/hours.rs b/gitoxide-core/src/hours.rs index eb2d46b23e2..803631c12f8 100644 --- a/gitoxide-core/src/hours.rs +++ b/gitoxide-core/src/hours.rs @@ -9,9 +9,7 @@ use std::{ }; use anyhow::{anyhow, bail}; -use git_repository::{ - actor, bstr::BString, interrupt, objs, odb, odb::pack, prelude::*, progress, refs::file::ReferenceExt, Progress, -}; +use git_repository::{actor, bstr::BString, interrupt, objs, prelude::*, progress, refs::file::ReferenceExt, Progress}; use itertools::Itertools; use rayon::prelude::*; @@ -52,7 +50,7 @@ where .find(refname.to_string_lossy().as_ref())? .peel_to_id_in_place(&repo.refs, |oid, buf| { repo.odb - .try_find(oid, buf, &mut pack::cache::Never) + .try_find(oid, buf) .map(|obj| obj.map(|(obj, _location)| (obj.kind, obj.data))) })? .to_owned(); @@ -61,12 +59,11 @@ where let start = Instant::now(); let mut progress = progress.add_child("Traverse commit graph"); progress.init(None, progress::count("commits")); - let mut pack_cache = odb::pack::cache::Never; let mut commits: Vec> = Vec::new(); for c in interrupt::Iter::new( commit_id.ancestors(|oid, buf| { progress.inc(); - repo.odb.find(oid, buf, &mut pack_cache).ok().map(|(o, _l)| { + repo.odb.find(oid, buf).ok().map(|(o, _l)| { commits.push(o.data.to_owned()); objs::CommitRefIter::from_bytes(o.data) }) diff --git a/gitoxide-core/src/pack/create.rs b/gitoxide-core/src/pack/create.rs index 24a34f4dec9..3da91f94d15 100644 --- a/gitoxide-core/src/pack/create.rs +++ b/gitoxide-core/src/pack/create.rs @@ -7,10 +7,7 @@ use git_repository::{ hash::ObjectId, interrupt, objs::bstr::ByteVec, - odb::{ - pack, - pack::{cache::DecodeEntry, FindExt}, - }, + odb::{pack, pack::FindExt}, prelude::{Finalize, ReferenceAccessExt}, progress, traverse, Progress, }; @@ -110,7 +107,7 @@ pub fn create( thin, thread_limit, statistics, - pack_cache_size_in_bytes, + pack_cache_size_in_bytes: _, object_cache_size_in_bytes, mut out, }: Context, @@ -145,7 +142,7 @@ where let iter = Box::new( traverse::commit::Ancestors::new(tips, traverse::commit::ancestors::State::default(), { let db = Arc::clone(&odb); - move |oid, buf| db.find_commit_iter(oid, buf, &mut pack::cache::Never).ok().map(|t| t.0) + move |oid, buf| db.find_commit_iter(oid, buf).ok().map(|t| t.0) }) .map(|res| res.map_err(Into::into)) .inspect(move |_| progress.inc()), @@ -181,7 +178,7 @@ where Some(1) }; let (_, _, thread_count) = git::parallel::optimize_chunk_size_and_thread_limit(50, None, thread_limit, None); - let make_caches = move || { + let make_object_cache = move || { let per_thread_object_cache_size = object_cache_size_in_bytes / thread_count; let object_cache: Box = if per_thread_object_cache_size < 10_000 { Box::new(pack::cache::object::Never) as Box @@ -190,20 +187,21 @@ where per_thread_object_cache_size, )) }; - let per_thread_object_pack_size = pack_cache_size_in_bytes / thread_count; - let pack_cache: Box = if per_thread_object_pack_size < 10_000 { - Box::new(pack::cache::Never) as Box - } else { - Box::new(pack::cache::lru::MemoryCappedHashmap::new(per_thread_object_pack_size)) - }; - (pack_cache, object_cache) + // TODO: bring this cache configuration back to where it belongs. + // let per_thread_object_pack_size = pack_cache_size_in_bytes / thread_count; + // let pack_cache: Box = if per_thread_object_pack_size < 10_000 { + // Box::new(pack::cache::Never) as Box + // } else { + // Box::new(pack::cache::lru::MemoryCappedHashmap::new(per_thread_object_pack_size)) + // }; + object_cache }; let progress = progress::ThroughputOnDrop::new(progress); let input_object_expansion = expansion.into(); let (mut counts, count_stats) = if may_use_multiple_threads { pack::data::output::count::objects( Arc::clone(&odb), - make_caches, + make_object_cache, input, progress, &interrupt::IS_INTERRUPTED, @@ -214,10 +212,10 @@ where }, )? } else { - let mut caches = make_caches(); + let mut object_cache = make_object_cache(); pack::data::output::count::objects_unthreaded( Arc::clone(&odb), - (&mut caches.0, &mut caches.1), + &mut object_cache, input, progress, &interrupt::IS_INTERRUPTED, @@ -236,7 +234,6 @@ where pack::data::output::InOrderIter::from(pack::data::output::entry::iter_from_counts( counts, Arc::clone(&odb), - || pack::cache::Never, progress, pack::data::output::entry::iter_from_counts::Options { thread_limit,