Skip to content

Commit 8084dd2

Browse files
committed
pack-objects: refactor path-walk delta phase
Previously, the --path-walk option to 'git pack-objects' would compute deltas inline with the path-walk logic. This would make the progress indicator look like it is taking a long time to enumerate objects, and then very quickly computed deltas. Instead of computing deltas on each region of objects organized by tree, store a list of regions corresponding to these groups. These can later be pulled from the list for delta compression before doing the "global" delta search. This presents a new progress indicator that can be used in tests to verify that this stage is happening. The current implementation is not integrated with threads, but could be done in a future update. Since we do not attempt to sort objects by size until after exploring all trees, we can remove the previous change to t5530 due to a different error message appearing first. Signed-off-by: Derrick Stolee <[email protected]>
1 parent af3c37b commit 8084dd2

File tree

4 files changed

+74
-33
lines changed

4 files changed

+74
-33
lines changed

builtin/pack-objects.c

Lines changed: 56 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -3178,6 +3178,50 @@ static int should_attempt_deltas(struct object_entry *entry)
31783178
return 1;
31793179
}
31803180

3181+
static void find_deltas_for_region(struct object_entry *list,
3182+
struct packing_region *region,
3183+
unsigned int *processed)
3184+
{
3185+
struct object_entry **delta_list;
3186+
uint32_t delta_list_nr = 0;
3187+
3188+
ALLOC_ARRAY(delta_list, region->nr);
3189+
for (uint32_t i = 0; i < region->nr; i++) {
3190+
struct object_entry *entry = to_pack.objects + region->start + i;
3191+
if (should_attempt_deltas(entry))
3192+
delta_list[delta_list_nr++] = entry;
3193+
}
3194+
3195+
QSORT(delta_list, delta_list_nr, type_size_sort);
3196+
find_deltas(delta_list, &delta_list_nr, window, depth, processed);
3197+
free(delta_list);
3198+
}
3199+
3200+
static void find_deltas_by_region(struct object_entry *list,
3201+
struct packing_region *regions,
3202+
uint32_t start, uint32_t nr)
3203+
{
3204+
unsigned int processed = 0;
3205+
uint32_t progress_nr;
3206+
3207+
if (!nr)
3208+
return;
3209+
3210+
progress_nr = regions[nr - 1].start + regions[nr - 1].nr;
3211+
3212+
if (progress)
3213+
progress_state = start_progress(_("Compressing objects by path"),
3214+
progress_nr);
3215+
3216+
while (nr--)
3217+
find_deltas_for_region(list,
3218+
&regions[start++],
3219+
&processed);
3220+
3221+
display_progress(progress_state, progress_nr);
3222+
stop_progress(&progress_state);
3223+
}
3224+
31813225
static void prepare_pack(int window, int depth)
31823226
{
31833227
struct object_entry **delta_list;
@@ -3202,6 +3246,10 @@ static void prepare_pack(int window, int depth)
32023246
if (!to_pack.nr_objects || !window || !depth)
32033247
return;
32043248

3249+
if (path_walk)
3250+
find_deltas_by_region(to_pack.objects, to_pack.regions,
3251+
0, to_pack.nr_regions);
3252+
32053253
ALLOC_ARRAY(delta_list, to_pack.nr_objects);
32063254
nr_deltas = n = 0;
32073255

@@ -4138,10 +4186,8 @@ static int add_objects_by_path(const char *path,
41384186
enum object_type type,
41394187
void *data)
41404188
{
4141-
struct object_entry **delta_list;
41424189
size_t oe_start = to_pack.nr_objects;
41434190
size_t oe_end;
4144-
unsigned int sub_list_size;
41454191
unsigned int *processed = data;
41464192

41474193
/*
@@ -4174,32 +4220,17 @@ static int add_objects_by_path(const char *path,
41744220
if (oe_end == oe_start || !window)
41754221
return 0;
41764222

4177-
sub_list_size = 0;
4178-
ALLOC_ARRAY(delta_list, oe_end - oe_start);
4223+
ALLOC_GROW(to_pack.regions,
4224+
to_pack.nr_regions + 1,
4225+
to_pack.nr_regions_alloc);
41794226

4180-
for (size_t i = 0; i < oe_end - oe_start; i++) {
4181-
struct object_entry *entry = to_pack.objects + oe_start + i;
4227+
to_pack.regions[to_pack.nr_regions].start = oe_start;
4228+
to_pack.regions[to_pack.nr_regions].nr = oe_end - oe_start;
4229+
to_pack.nr_regions++;
41824230

4183-
if (!should_attempt_deltas(entry))
4184-
continue;
4231+
*processed += oids->nr;
4232+
display_progress(progress_state, *processed);
41854233

4186-
delta_list[sub_list_size++] = entry;
4187-
}
4188-
4189-
/*
4190-
* Find delta bases among this list of objects that all match the same
4191-
* path. This causes the delta compression to be interleaved in the
4192-
* object walk, which can lead to confusing progress indicators. This is
4193-
* also incompatible with threaded delta calculations. In the future,
4194-
* consider creating a list of regions in the full to_pack.objects array
4195-
* that could be picked up by the threaded delta computation.
4196-
*/
4197-
if (sub_list_size && window) {
4198-
QSORT(delta_list, sub_list_size, type_size_sort);
4199-
find_deltas(delta_list, &sub_list_size, window, depth, processed);
4200-
}
4201-
4202-
free(delta_list);
42034234
return 0;
42044235
}
42054236

pack-objects.h

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,11 +118,23 @@ struct object_entry {
118118
unsigned ext_base:1; /* delta_idx points outside packlist */
119119
};
120120

121+
/**
122+
* A packing region is a section of the packing_data.objects array
123+
* as given by a starting index and a number of elements.
124+
*/
125+
struct packing_region {
126+
uint32_t start;
127+
uint32_t nr;
128+
};
129+
121130
struct packing_data {
122131
struct repository *repo;
123132
struct object_entry *objects;
124133
uint32_t nr_objects, nr_alloc;
125134

135+
struct packing_region *regions;
136+
uint32_t nr_regions, nr_regions_alloc;
137+
126138
int32_t *index;
127139
uint32_t index_size;
128140

t/t5300-pack-object.sh

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -692,7 +692,9 @@ test_expect_success '--full-name-hash and --write-bitmap-index are incompatible'
692692
# Basic "repack everything" test
693693
test_expect_success '--path-walk pack everything' '
694694
git -C server rev-parse HEAD >in &&
695-
git -C server pack-objects --stdout --revs --path-walk <in >out.pack &&
695+
GIT_PROGRESS_DELAY=0 git -C server pack-objects \
696+
--stdout --revs --path-walk --progress <in >out.pack 2>err &&
697+
grep "Compressing objects by path" err &&
696698
git -C server index-pack --stdin <out.pack
697699
'
698700

@@ -702,7 +704,9 @@ test_expect_success '--path-walk thin pack' '
702704
$(git -C server rev-parse HEAD)
703705
^$(git -C server rev-parse HEAD~2)
704706
EOF
705-
git -C server pack-objects --thin --stdout --revs --path-walk <in >out.pack &&
707+
GIT_PROGRESS_DELAY=0 git -C server pack-objects \
708+
--thin --stdout --revs --path-walk --progress <in >out.pack 2>err &&
709+
grep "Compressing objects by path" err &&
706710
git -C server index-pack --fix-thin --stdin <out.pack
707711
'
708712

t/t5530-upload-pack-error.sh

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -35,12 +35,6 @@ test_expect_success 'upload-pack fails due to error in pack-objects packing' '
3535
hexsz=$(test_oid hexsz) &&
3636
printf "%04xwant %s\n00000009done\n0000" \
3737
$(($hexsz + 10)) $head >input &&
38-
39-
# The current implementation of path-walk causes a different
40-
# error message. This will be changed by a future refactoring.
41-
GIT_TEST_PACK_PATH_WALK=0 &&
42-
export GIT_TEST_PACK_PATH_WALK &&
43-
4438
test_must_fail git upload-pack . <input >/dev/null 2>output.err &&
4539
test_grep "unable to read" output.err &&
4640
test_grep "pack-objects died" output.err

0 commit comments

Comments
 (0)