Skip to content

Commit 724601d

Browse files
committed
fix: improve handling of overflows when there are more pack than we can hold.
Internally, there is a statically allocated vec which holds opened packs and indices. When reconciling the disk-state with what's currently loaded, it was possible to get into a situation where there were more files than we could fit into the slotmap and got into an invalid state. The system now generously trashes existing slots to be able to load more rencent ones, which seems to help in this situation. It's probably still leading to strange situations where not all objects can be read.
1 parent af8f201 commit 724601d

File tree

2 files changed

+10
-4
lines changed

2 files changed

+10
-4
lines changed

gix-odb/src/lib.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ pub struct Store {
119119

120120
/// The below state acts like a slot-map with each slot is mutable when the write lock is held, but readable independently of it.
121121
/// This allows multiple file to be loaded concurrently if there is multiple handles requesting to load packs or additional indices.
122-
/// The map is static and cannot typically change.
122+
/// The map is static and cannot change.
123123
/// It's read often and changed rarely.
124124
pub(crate) files: Vec<types::MutableIndexAndPack>,
125125

gix-odb/src/store_impls/dynamic/load_index.rs

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -266,7 +266,7 @@ impl super::Store {
266266
Option::as_ref(&files_guard).expect("slot is set or we wouldn't know it points to this file");
267267
if index_info.is_multi_index() && files.mtime() != mtime {
268268
// we have a changed multi-pack index. We can't just change the existing slot as it may alter slot indices
269-
// that are currently available. Instead we have to move what's there into a new slot, along with the changes,
269+
// that are currently available. Instead, we have to move what's there into a new slot, along with the changes,
270270
// and later free the slot or dispose of the index in the slot (like we do for removed/missing files).
271271
index_paths_to_add.push_back((index_info, mtime, Some(slot_idx)));
272272
// If the current slot is loaded, the soon-to-be copied multi-index path will be loaded as well.
@@ -304,6 +304,12 @@ impl super::Store {
304304
needed: index_paths_to_add.len() + 1, /*the one currently popped off*/
305305
});
306306
}
307+
// Don't allow duplicate indicates, we need a 1:1 mapping.
308+
if new_slot_map_indices.contains(&next_possibly_free_index) {
309+
next_possibly_free_index = (next_possibly_free_index + 1) % self.files.len();
310+
num_indices_checked += 1;
311+
continue 'increment_slot_index;
312+
}
307313
let slot_index = next_possibly_free_index;
308314
let slot = &self.files[slot_index];
309315
next_possibly_free_index = (next_possibly_free_index + 1) % self.files.len();
@@ -502,7 +508,7 @@ impl super::Store {
502508
}
503509
// Unlike libgit2, do not sort by modification date, but by size and put the biggest indices first. That way
504510
// the chance to hit an object should be higher. We leave it to the handle to sort by LRU.
505-
// Git itself doesn't change the order which may safe time, but we want it to be stable which also helps some tests.
511+
// Git itself doesn't change the order which may save time, but we want it to be stable which also helps some tests.
506512
// NOTE: this will work well for well-packed repos or those using geometric repacking, but force us to open a lot
507513
// of files when dealing with new objects, as there is no notion of recency here as would be with unmaintained
508514
// repositories. Different algorithms should be provided, like newest packs first, and possibly a mix of both
@@ -512,7 +518,7 @@ impl super::Store {
512518
Ok(indices_by_modification_time)
513519
}
514520

515-
/// returns Ok<dest slot was empty> if the copy could happen because dest-slot was actually free or disposable , and Some(true) if it was empty
521+
/// returns `Ok(dest_slot_was_empty)` if the copy could happen because dest-slot was actually free or disposable.
516522
#[allow(clippy::too_many_arguments)]
517523
fn try_set_index_slot(
518524
lock: &parking_lot::MutexGuard<'_, ()>,

0 commit comments

Comments
 (0)