Skip to content

various improvements #1520

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

Merged
merged 14 commits into from
Aug 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion gitoxide-core/src/hours/core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ pub fn spawn_tree_delta_threads<'scope>(
for chunk in rx {
for (commit_idx, parent_commit, commit) in chunk {
if let Some(cache) = cache.as_mut() {
cache.clear_resource_cache();
cache.clear_resource_cache_keep_allocation();
}
commits.fetch_add(1, Ordering::Relaxed);
if gix::interrupt::is_triggered() {
Expand Down
4 changes: 2 additions & 2 deletions gitoxide-core/src/query/engine/update.rs
Original file line number Diff line number Diff line change
Expand Up @@ -204,8 +204,8 @@ pub fn update(
Some(c) => c,
None => continue,
};
rewrite_cache.clear_resource_cache();
diff_cache.clear_resource_cache();
rewrite_cache.clear_resource_cache_keep_allocation();
diff_cache.clear_resource_cache_keep_allocation();
from.changes()?
.track_path()
.track_rewrites(Some(rewrites))
Expand Down
2 changes: 2 additions & 0 deletions gix-diff/src/blob/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,8 @@ pub struct Platform {
/// That way, expensive rewrite-checks with NxM matrix checks would be as fast as possible,
/// avoiding duplicate work.
diff_cache: HashMap<platform::CacheKey, platform::CacheValue>,
/// A list of previously used buffers, ready for re-use.
free_list: Vec<Vec<u8>>,
}

mod impls {
Expand Down
40 changes: 36 additions & 4 deletions gix-diff/src/blob/platform.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use std::{io::Write, process::Stdio};

use bstr::{BStr, BString, ByteSlice};
use std::cmp::Ordering;
use std::{io::Write, process::Stdio};

use super::Algorithm;
use crate::blob::{pipeline, Pipeline, Platform, ResourceKind};
Expand Down Expand Up @@ -325,6 +325,7 @@ impl Platform {
old: None,
new: None,
diff_cache: Default::default(),
free_list: Vec::with_capacity(2),
options,
filter,
filter_mode,
Expand Down Expand Up @@ -542,7 +543,7 @@ impl Platform {

/// Every call to [set_resource()](Self::set_resource()) will keep the diffable data in memory, and that will never be cleared.
///
/// Use this method to clear the cache, releasing memory. Note that this will also loose all information about resources
/// Use this method to clear the cache, releasing memory. Note that this will also lose all information about resources
/// which means diffs would fail unless the resources are set again.
///
/// Note that this also has to be called if the same resource is going to be diffed in different states, i.e. using different
Expand All @@ -551,6 +552,37 @@ impl Platform {
self.old = None;
self.new = None;
self.diff_cache.clear();
self.free_list.clear();
}

/// Every call to [set_resource()](Self::set_resource()) will keep the diffable data in memory, and that will never be cleared.
///
/// Use this method to clear the cache, but keep the previously used buffers around for later re-use.
///
/// If there are more buffers on the free-list than there are stored sources, we half that amount each time this method is called,
/// or keep as many resources as were previously stored, or 2 buffers, whatever is larger.
/// If there are fewer buffers in the free-list than are in the resource cache, we will keep as many as needed to match the
/// number of previously stored resources.
///
/// Returns the number of available buffers.
pub fn clear_resource_cache_keep_allocation(&mut self) -> usize {
self.old = None;
self.new = None;

let diff_cache = std::mem::take(&mut self.diff_cache);
match self.free_list.len().cmp(&diff_cache.len()) {
Ordering::Less => {
let to_take = diff_cache.len() - self.free_list.len();
self.free_list
.extend(diff_cache.into_values().map(|v| v.buffer).take(to_take));
}
Ordering::Equal => {}
Ordering::Greater => {
let new_len = (self.free_list.len() / 2).max(diff_cache.len()).max(2);
self.free_list.truncate(new_len);
}
}
self.free_list.len()
}
}

Expand Down Expand Up @@ -591,7 +623,7 @@ impl Platform {
kind,
rela_path: rela_path.to_owned(),
})?;
let mut buf = Vec::new();
let mut buf = self.free_list.pop().unwrap_or_default();
let out = self.filter.convert_to_diffable(
&id,
mode,
Expand Down
24 changes: 23 additions & 1 deletion gix-diff/tests/blob/platform.rs
Original file line number Diff line number Diff line change
Expand Up @@ -121,13 +121,35 @@ fn resources_of_worktree_and_odb_and_check_link() -> crate::Result {
"Also obvious that symlinks are definitely special, but it's what git does as well"
);

platform.clear_resource_cache();
assert_eq!(
platform.clear_resource_cache_keep_allocation(),
3,
"some buffers are retained and reused"
);
assert_eq!(
platform.resources(),
None,
"clearing the cache voids resources and one has to set it up again"
);

assert_eq!(
platform.clear_resource_cache_keep_allocation(),
2,
"doing this again keeps 2 buffers"
);
assert_eq!(
platform.clear_resource_cache_keep_allocation(),
2,
"no matter what - after all we need at least two resources for a diff"
);

platform.clear_resource_cache();
assert_eq!(
platform.clear_resource_cache_keep_allocation(),
0,
"after a proper clearing, the free-list is also emptied, and it won't be recreated"
);

Ok(())
}

Expand Down
12 changes: 6 additions & 6 deletions gix-ref/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,8 +150,8 @@ pub struct Namespace(BString);
#[derive(PartialEq, Eq, Debug, Hash, Ord, PartialOrd, Clone, Copy)]
#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
pub enum Kind {
/// A ref that points to an object id
Peeled,
/// A ref that points to an object id directly.
Object,
/// A ref that points to another reference, adding a level of indirection.
///
/// It can be resolved to an id using the [`peel_in_place_to_id()`][`crate::file::ReferenceExt::peel_to_id_in_place()`] method.
Expand Down Expand Up @@ -203,8 +203,8 @@ pub enum Category<'a> {
#[derive(PartialEq, Eq, Debug, Hash, Ord, PartialOrd, Clone)]
#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
pub enum Target {
/// A ref that points to an object id
Peeled(ObjectId),
/// A ref that points directly to an object id.
Object(ObjectId),
/// A ref that points to another reference by its validated name, adding a level of indirection.
///
/// Note that this is an extension of gitoxide which will be helpful in logging all reference changes.
Expand All @@ -214,8 +214,8 @@ pub enum Target {
/// Denotes a ref target, equivalent to [`Kind`], but with immutable data.
#[derive(PartialEq, Eq, Debug, Hash, Ord, PartialOrd, Clone, Copy)]
pub enum TargetRef<'a> {
/// A ref that points to an object id
Peeled(&'a oid),
/// A ref that points directly to an object id.
Object(&'a oid),
/// A ref that points to another reference by its validated name, adding a level of indirection.
Symbolic(&'a FullNameRef),
}
26 changes: 19 additions & 7 deletions gix-ref/src/peel.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,29 @@
///
#[allow(clippy::empty_docs)]
pub mod to_id {
use std::path::PathBuf;

use gix_object::bstr::BString;

/// The error returned by [`crate::file::ReferenceExt::peel_to_id_in_place()`].
#[derive(Debug, thiserror::Error)]
#[allow(missing_docs)]
pub enum Error {
#[error(transparent)]
FollowToObject(#[from] super::to_object::Error),
#[error("An error occurred when trying to resolve an object a reference points to")]
Find(#[from] gix_object::find::Error),
#[error("Object {oid} as referred to by {name:?} could not be found")]
NotFound { oid: gix_hash::ObjectId, name: BString },
}
}

///
#[allow(clippy::empty_docs)]
pub mod to_object {
use std::path::PathBuf;

use crate::file;

/// The error returned by [`crate::file::ReferenceExt::peel_to_id_in_place()`].
/// The error returned by [`file::ReferenceExt::follow_to_object_in_place_packed()`].
#[derive(Debug, thiserror::Error)]
#[allow(missing_docs)]
pub enum Error {
Expand All @@ -17,9 +33,5 @@ pub mod to_id {
Cycle { start_absolute: PathBuf },
#[error("Refusing to follow more than {max_depth} levels of indirection")]
DepthLimitExceeded { max_depth: usize },
#[error("An error occurred when trying to resolve an object a reference points to")]
Find(#[from] gix_object::find::Error),
#[error("Object {oid} as referred to by {name:?} could not be found")]
NotFound { oid: gix_hash::ObjectId, name: BString },
}
}
5 changes: 3 additions & 2 deletions gix-ref/src/raw.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ pub struct Reference {
pub name: FullName,
/// The target of the reference, either a symbolic reference by full name or a possibly intermediate object by its id.
pub target: Target,
/// The fully peeled object to which this reference ultimately points to. Only guaranteed to be set after
/// The fully peeled object to which this reference ultimately points to after following all symbolic refs and all annotated
/// tags. Only guaranteed to be set after
/// [`Reference::peel_to_id_in_place()`](crate::file::ReferenceExt) was called or if this reference originated
/// from a packed ref.
pub peeled: Option<ObjectId>,
Expand Down Expand Up @@ -48,7 +49,7 @@ mod convert {
fn from(value: packed::Reference<'p>) -> Self {
Reference {
name: value.name.into(),
target: Target::Peeled(value.target()),
target: Target::Object(value.target()),
peeled: value
.object
.map(|hex| ObjectId::from_hex(hex).expect("parser validation")),
Expand Down
2 changes: 1 addition & 1 deletion gix-ref/src/store/file/loose/reference/decode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ impl TryFrom<MaybeUnsafeState> for Target {

fn try_from(v: MaybeUnsafeState) -> Result<Self, Self::Error> {
Ok(match v {
MaybeUnsafeState::Id(id) => Target::Peeled(id),
MaybeUnsafeState::Id(id) => Target::Object(id),
MaybeUnsafeState::UnvalidatedPath(name) => {
Target::Symbolic(match gix_validate::reference::name(name.as_ref()) {
Ok(_) => FullName(name),
Expand Down
Loading
Loading