Skip to content

Commit 73a7d15

Browse files
authored
Merge pull request #1603 from Byron/freelist
freelist control
2 parents 35c7213 + 0cac690 commit 73a7d15

File tree

8 files changed

+136
-39
lines changed

8 files changed

+136
-39
lines changed

gix/src/object/tree/mod.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ impl<'repo> Tree<'repo> {
6161
I: IntoIterator<Item = P>,
6262
P: PartialEq<BStr>,
6363
{
64-
let mut buf = self.repo.shared_empty_buf();
64+
let mut buf = self.repo.empty_reusable_buffer();
6565
buf.clear();
6666

6767
let mut path = path.into_iter().peekable();

gix/src/repository/freelist.rs

+97
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
use std::cell::RefCell;
2+
use std::ops::{Deref, DerefMut};
3+
4+
/// A buffer that is returned to the free-list after usage.
5+
#[derive(Clone)]
6+
pub struct Buffer<'repo> {
7+
/// The buffer that would be returned to the freelist of `repo`.
8+
/// Note that buffers without capacity (i.e. without allocation) aren't returned.
9+
pub inner: Vec<u8>,
10+
/// The repository from whose free-list the `inner` buffer was taken, and to which it will be returned.
11+
pub repo: &'repo crate::Repository,
12+
}
13+
14+
impl From<Buffer<'_>> for Vec<u8> {
15+
fn from(mut value: Buffer<'_>) -> Self {
16+
std::mem::take(&mut value.inner)
17+
}
18+
}
19+
20+
impl Deref for Buffer<'_> {
21+
type Target = Vec<u8>;
22+
23+
fn deref(&self) -> &Self::Target {
24+
&self.inner
25+
}
26+
}
27+
28+
impl DerefMut for Buffer<'_> {
29+
fn deref_mut(&mut self) -> &mut Self::Target {
30+
&mut self.inner
31+
}
32+
}
33+
34+
impl Drop for Buffer<'_> {
35+
fn drop(&mut self) {
36+
self.repo.reuse_buffer(&mut self.inner);
37+
}
38+
}
39+
40+
/// Internal
41+
impl crate::Repository {
42+
/// Note that the returned buffer might still have data in it.
43+
#[inline]
44+
pub(crate) fn free_buf(&self) -> Vec<u8> {
45+
self.bufs
46+
.as_ref()
47+
.and_then(|bufs| bufs.borrow_mut().pop())
48+
.unwrap_or_default()
49+
}
50+
51+
/// This method is commonly called from the destructor of objects that previously claimed an entry
52+
/// in the free-list with [crate::Repository::free_buf].
53+
/// They are welcome to take out the data themselves, for instance when the object is detached, to avoid
54+
/// it to be reclaimed.
55+
#[inline]
56+
pub(crate) fn reuse_buffer(&self, data: &mut Vec<u8>) {
57+
if data.capacity() > 0 {
58+
if let Some(bufs) = self.bufs.as_ref() {
59+
bufs.borrow_mut().push(std::mem::take(data));
60+
}
61+
}
62+
}
63+
}
64+
65+
/// Freelist configuration
66+
///
67+
/// The free-list is an internal and 'transparent' mechanism for obtaining and re-using memory buffers when
68+
/// reading objects. That way, trashing is avoided as buffers are re-used and re-written.
69+
///
70+
/// However, there are circumstances when releasing memory early is preferred, for instance on the server side.
71+
///
72+
/// Also note that the free-list isn't cloned, so each clone of this instance starts with an empty one.
73+
impl crate::Repository {
74+
/// Return an empty buffer which is tied to this repository instance, and reuse its memory allocation by
75+
/// keeping it around even after it drops.
76+
pub fn empty_reusable_buffer(&self) -> Buffer<'_> {
77+
let mut inner = self.free_buf();
78+
inner.clear();
79+
Buffer { inner, repo: self }
80+
}
81+
82+
/// Set the currently used freelist to `list`. If `None`, it will be disabled entirely.
83+
///
84+
/// Return the currently previously allocated free-list, a list of reusable buffers typically used when reading objects.
85+
/// May be `None` if there was no free-list.
86+
pub fn set_freelist(&mut self, list: Option<Vec<Vec<u8>>>) -> Option<Vec<Vec<u8>>> {
87+
let previous = self.bufs.take();
88+
self.bufs = list.map(RefCell::new);
89+
previous.map(RefCell::into_inner)
90+
}
91+
92+
/// A builder method to disable the free-list on a newly created instance.
93+
pub fn without_freelist(mut self) -> Self {
94+
self.bufs.take();
95+
self
96+
}
97+
}

gix/src/repository/impls.rs

+8-2
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
impl Clone for crate::Repository {
22
fn clone(&self) -> Self {
3-
crate::Repository::from_refs_and_objects(
3+
let mut new = crate::Repository::from_refs_and_objects(
44
self.refs.clone(),
55
self.objects.clone(),
66
self.work_tree.clone(),
@@ -12,7 +12,13 @@ impl Clone for crate::Repository {
1212
self.shallow_commits.clone(),
1313
#[cfg(feature = "attributes")]
1414
self.modules.clone(),
15-
)
15+
);
16+
17+
if self.bufs.is_none() {
18+
new.bufs.take();
19+
}
20+
21+
new
1622
}
1723
}
1824

gix/src/repository/init.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ impl crate::Repository {
1515
) -> Self {
1616
setup_objects(&mut objects, &config);
1717
crate::Repository {
18-
bufs: RefCell::new(Vec::with_capacity(4)),
18+
bufs: Some(RefCell::new(Vec::with_capacity(4))),
1919
work_tree,
2020
common_dir,
2121
objects,

gix/src/repository/mod.rs

+2-19
Original file line numberDiff line numberDiff line change
@@ -17,25 +17,6 @@ pub enum Kind {
1717
},
1818
}
1919

20-
/// Internal
21-
impl crate::Repository {
22-
#[inline]
23-
pub(crate) fn free_buf(&self) -> Vec<u8> {
24-
self.bufs.borrow_mut().pop().unwrap_or_default()
25-
}
26-
27-
/// This method is commonly called from the destructor of objects that previously claimed an entry
28-
/// in the free-list with `free_buf()`.
29-
/// They are welcome to take out the data themselves, for instance when the object is detached, to avoid
30-
/// it to be reclaimed.
31-
#[inline]
32-
pub(crate) fn reuse_buffer(&self, data: &mut Vec<u8>) {
33-
if data.capacity() > 0 {
34-
self.bufs.borrow_mut().push(std::mem::take(data));
35-
}
36-
}
37-
}
38-
3920
#[cfg(any(feature = "attributes", feature = "excludes"))]
4021
pub mod attributes;
4122
mod cache;
@@ -49,6 +30,8 @@ mod dirwalk;
4930
///
5031
#[cfg(feature = "attributes")]
5132
pub mod filter;
33+
///
34+
pub mod freelist;
5235
mod graph;
5336
pub(crate) mod identity;
5437
mod impls;

gix/src/repository/object.rs

+2-14
Original file line numberDiff line numberDiff line change
@@ -158,24 +158,12 @@ impl crate::Repository {
158158

159159
/// Write objects of any type.
160160
impl crate::Repository {
161-
pub(crate) fn shared_empty_buf(&self) -> std::cell::RefMut<'_, Vec<u8>> {
162-
let mut bufs = self.bufs.borrow_mut();
163-
if bufs.last().is_none() {
164-
bufs.push(Vec::with_capacity(512));
165-
}
166-
std::cell::RefMut::map(bufs, |bufs| {
167-
let buf = bufs.last_mut().expect("we assure one is present");
168-
buf.clear();
169-
buf
170-
})
171-
}
172-
173161
/// Write the given object into the object database and return its object id.
174162
///
175163
/// Note that we hash the object in memory to avoid storing objects that are already present. That way,
176164
/// we avoid writing duplicate objects using slow disks that will eventually have to be garbage collected.
177165
pub fn write_object(&self, object: impl gix_object::WriteTo) -> Result<Id<'_>, object::write::Error> {
178-
let mut buf = self.shared_empty_buf();
166+
let mut buf = self.empty_reusable_buffer();
179167
object.write_to(buf.deref_mut()).expect("write to memory works");
180168

181169
self.write_object_inner(&buf, object.kind())
@@ -219,7 +207,7 @@ impl crate::Repository {
219207
&self,
220208
mut bytes: impl std::io::Read + std::io::Seek,
221209
) -> Result<Id<'_>, object::write::Error> {
222-
let mut buf = self.shared_empty_buf();
210+
let mut buf = self.empty_reusable_buffer();
223211
std::io::copy(&mut bytes, buf.deref_mut()).expect("write to memory works");
224212

225213
self.write_blob_stream_inner(&buf)

gix/src/types.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ pub struct Repository {
151151
/// The path to the resolved common directory if this is a linked worktree repository or it is otherwise set.
152152
pub(crate) common_dir: Option<PathBuf>,
153153
/// A free-list of reusable object backing buffers
154-
pub(crate) bufs: RefCell<Vec<Vec<u8>>>,
154+
pub(crate) bufs: Option<RefCell<Vec<Vec<u8>>>>,
155155
/// A pre-assembled selection of often-accessed configuration values for quick access.
156156
pub(crate) config: crate::config::Cache,
157157
/// the options obtained when instantiating this repository.

gix/tests/repository/object.rs

+24-1
Original file line numberDiff line numberDiff line change
@@ -255,6 +255,7 @@ mod write_blob {
255255
let (_tmp, repo) = empty_bare_repo()?;
256256
let mut cursor = std::io::Cursor::new(b"hello world");
257257
let mut seek_cursor = cursor.clone();
258+
let mut repo = repo.without_freelist();
258259
let oid = repo.write_blob_stream(&mut cursor)?;
259260
assert_eq!(oid, hex_to_id("95d09f2b10159347eece71399a7e2e907ea3df4f"));
260261

@@ -271,13 +272,15 @@ mod write_blob {
271272
&b"world"[..],
272273
"the seek position is taken into account, so only part of the input data is written"
273274
);
275+
276+
assert!(repo.set_freelist(None).is_none(), "previous list was already dropped");
274277
Ok(())
275278
}
276279
}
277280

278281
#[test]
279282
fn writes_avoid_io_using_duplicate_check() -> crate::Result {
280-
let repo = crate::named_repo("make_packed_and_loose.sh")?;
283+
let mut repo = crate::named_repo("make_packed_and_loose.sh")?;
281284
let store = gix::odb::loose::Store::at(repo.git_dir().join("objects"), repo.object_hash());
282285
let loose_count = store.iter().count();
283286
assert_eq!(loose_count, 3, "there are some loose objects");
@@ -326,6 +329,26 @@ fn writes_avoid_io_using_duplicate_check() -> crate::Result {
326329
loose_count,
327330
"no new object was written as all of them already existed"
328331
);
332+
333+
{
334+
let buf = repo.empty_reusable_buffer();
335+
assert!(buf.is_empty(), "the freelist buffer must be clearerd");
336+
let mut other_buf = buf.clone();
337+
other_buf.inner = Vec::new();
338+
}
339+
340+
let freelist = repo.set_freelist(None).expect("free list is present by default");
341+
assert_eq!(
342+
freelist.len(),
343+
2,
344+
"only one object was read at a time, and one is written"
345+
);
346+
347+
let mut repo_clone = repo.clone();
348+
assert!(
349+
repo_clone.set_freelist(None).is_none(),
350+
"new instances inherit the free-list configuration of their parent"
351+
);
329352
Ok(())
330353
}
331354

0 commit comments

Comments
 (0)