Skip to content

Commit

Permalink
refactor!: remove pack-cache from Find::try_find(…) (#266)
Browse files Browse the repository at this point in the history 
With the new architecture this can be an implementation detail without
forcing it to be Sync.
  • Loading branch information
@Byron
Byron committed Dec 2, 2021
1 parent 108c77b commit 87930a1
Show file tree
Hide file tree
Showing 20 changed files with 103 additions and 177 deletions.
19 changes: 8 additions & 11 deletions experiments/diffing/src/main.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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::<Result<Vec<_>, _>>()?;
let num_diffs = all_commits.len();
let elapsed = start.elapsed();
Expand All @@ -67,7 +63,6 @@ fn main() -> anyhow::Result<()> {
buf: &'b mut Vec<u8>,
obj_cache: &mut memory_lru::MemoryLruCache<ObjectId, ObjectInfo>,
db: &odb::linked::Store,
pack_cache: &mut impl odb::pack::cache::DecodeEntry,
) -> Option<git_repository::objs::Data<'b>> {
let oid = oid.to_owned();
match obj_cache.get(&oid) {
Expand All @@ -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,
Expand All @@ -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<u8>| find_with_lru_mem_cache(oid, buf, &mut obj_cache, db, &mut pack_cache)
move |oid, buf: &mut Vec<u8>| find_with_lru_mem_cache(oid, buf, &mut obj_cache, db)
},
Computation::MultiThreaded,
)?;
Expand All @@ -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<u8>| find_with_lru_mem_cache(oid, buf, &mut obj_cache, db, &mut pack_cache)
move |oid, buf: &mut Vec<u8>| find_with_lru_mem_cache(oid, buf, &mut obj_cache, db)
},
Computation::SingleThreaded,
)?;
Expand Down
3 changes: 1 addition & 2 deletions experiments/odb-redesign/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -581,7 +581,6 @@ mod odb {
&self,
id: impl AsRef<git_hash::oid>,
buffer: &'a mut Vec<u8>,
pack_cache: &mut impl DecodeEntry,
) -> Result<Option<(git_object::Data<'a>, Option<git_pack::bundle::Location>)>, 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
Expand Down
35 changes: 12 additions & 23 deletions experiments/traversal/src/main.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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::<Result<Vec<_>, _>>()?;
let elapsed = start.elapsed();
println!(
Expand Down Expand Up @@ -143,13 +139,13 @@ fn main() -> anyhow::Result<()> {
fn do_gitoxide_commit_graph_traversal<C>(
tip: ObjectId,
db: &odb::linked::Store,
new_cache: impl FnOnce() -> C,
// TODO: make use of the cache
_new_cache: impl FnOnce() -> C,
) -> anyhow::Result<usize>
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?;
Expand All @@ -167,7 +163,7 @@ enum Computation {
fn do_gitoxide_tree_dag_traversal<C>(
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
Expand Down Expand Up @@ -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();
Expand All @@ -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;
Expand Down Expand Up @@ -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::<u8>::new(),
Vec::<u8>::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);
Expand Down
20 changes: 10 additions & 10 deletions git-diff/tests/visit/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -24,7 +24,7 @@ mod changes {
buf: &'a mut Vec<u8>,
) -> crate::Result<TreeRefIter<'a>> {
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()?
Expand All @@ -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()
Expand All @@ -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())
Expand All @@ -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()?
Expand All @@ -79,19 +79,19 @@ 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()
.expect("id to be a tree");
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())
};

Expand All @@ -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())
Expand Down Expand Up @@ -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())
Expand Down
6 changes: 2 additions & 4 deletions git-odb/src/store/linked/find.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ impl crate::pack::Find for linked::Store {
&self,
id: impl AsRef<oid>,
buffer: &'a mut Vec<u8>,
pack_cache: &mut impl pack::cache::DecodeEntry,
) -> Result<Option<(git_object::Data<'a>, Option<pack::bundle::Location>)>, Self::Error> {
let id = id.as_ref();
for db in self.dbs.iter() {
Expand All @@ -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);
}
Expand Down Expand Up @@ -117,9 +116,8 @@ impl crate::pack::Find for &linked::Store {
&self,
id: impl AsRef<oid>,
buffer: &'a mut Vec<u8>,
pack_cache: &mut impl pack::cache::DecodeEntry,
) -> Result<Option<(git_object::Data<'a>, Option<pack::bundle::Location>)>, Self::Error> {
(*self).try_find(id, buffer, pack_cache)
(*self).try_find(id, buffer)
}

fn location_by_oid(&self, id: impl AsRef<oid>, buf: &mut Vec<u8>) -> Option<Location> {
Expand Down
4 changes: 2 additions & 2 deletions git-odb/tests/odb/store/linked/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,15 +48,15 @@ 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};

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());
}
Expand Down
Loading

0 comments on commit 87930a1

Please sign in to comment.