Skip to content

Improve next_cache_entry cache traversal performance #429

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.

Sign up for GitHub

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
vdye merged 2 commits into from
Sep 21, 2021
Merged

Improve next_cache_entry cache traversal performance #429

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aed0843
fixup! p2000: add new commands
vdye Sep 21, 2021
c5fbbad
unpack-trees: improve performance of `next_cache_entry`
vdye Sep 21, 2021
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
1 change: 1 addition & 0 deletions t/perf/p2000-sparse-operations.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ test_perf_on_all git commit -a -m A
test_perf_on_all git checkout -f -
test_perf_on_all git reset
test_perf_on_all git reset --hard
test_perf_on_all git reset -- does-not-exist
Comment thread
vdye marked this conversation as resolved.
test_perf_on_all git read-tree -mu HEAD
test_perf_on_all git checkout-index -f --all
test_perf_on_all git update-index --add --remove
Expand Down
23 changes: 17 additions & 6 deletions unpack-trees.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -665,17 +665,24 @@ static void mark_ce_used_same_name(struct cache_entry *ce,
}
}

static struct cache_entry *next_cache_entry(struct unpack_trees_options *o)
static struct cache_entry *next_cache_entry(struct unpack_trees_options *o, int *hint)
{
const struct index_state *index = o->src_index;
int pos = o->cache_bottom;

@ldennington ldennington Sep 22, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know cache_bottom is critical to this change, but a little unsure what it means?

@derrickstolee derrickstolee Sep 22, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we scan the index, we are also drilling down the trees, so the cache-tree extension (if available) is giving us some idea of the range of index entries corresponding to a given tree. This can be computed dynamically, too. The cache_bottom value is storing the expected lower-end of this range. As we recurse into subtrees, the cache_bottom is stored in a stack variable somewhere and then the o->cache_bottom value is rewritten as the recursion ends.

In general: the cache_bottom thing is necessary to make these index scans work properly, but it is complicated because the index is a flat list of files. It's just one more thing that is added on top to try and make sense of that difference with the trees in the object database.


if (*hint > pos)
pos = *hint;

while (pos < index->cache_nr) {

@ldennington ldennington Sep 22, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the retroactive review comments - if it would be better to discuss on a call lmk!

I see index->cache_nr frequently but am having some trouble understanding it/finding docs on what it means (I think you mentioned "cache number," but I'm still not sure why that's significant.

@derrickstolee derrickstolee Sep 22, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cache_nr is the number of entries in the cache array.

In the Git codebase, you'll see a lot of X_nr and X_alloc representing that there are X_nr elements in an array currently allocated to have X_alloc entries.

@ldennington ldennington Sep 22, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

struct cache_entry *ce = index->cache[pos];
if (!(ce->ce_flags & CE_UNPACKED))
if (!(ce->ce_flags & CE_UNPACKED)) {
*hint = pos + 1;
return ce;
}
pos++;
}

*hint = pos;
return NULL;
}

Expand Down Expand Up @@ -1386,12 +1393,13 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, str

/* Are we supposed to look at the index too? */
if (o->merge) {
int hint = -1;
while (1) {
int cmp;
struct cache_entry *ce;

if (o->diff_index_cached)
ce = next_cache_entry(o);
ce = next_cache_entry(o, &hint);
Comment on lines +1396 to +1402

@derrickstolee derrickstolee Sep 21, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I look closely at your implementation, this old code had quadratic complexity! That helps me understand why having ~70,000 index entries still took 5 seconds to iterate.

I suppose I was expecting hint to be initialized with some information other than -1, but this seems to be a good idea regardless of any issues with the sparse index.

else
ce = find_cache_entry(info, p);

Expand Down Expand Up @@ -1720,7 +1728,7 @@ static int verify_absent(const struct cache_entry *,
int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options *o)
{
struct repository *repo = the_repository;
int i, ret;
int i, hint, ret;
static struct cache_entry *dfc;
struct pattern_list pl;
int free_pattern_list = 0;
Expand Down Expand Up @@ -1852,13 +1860,15 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
info.pathspec = o->pathspec;

if (o->prefix) {
hint = -1;

/*
* Unpack existing index entries that sort before the
* prefix the tree is spliced into. Note that o->merge
* is always true in this case.
*/
while (1) {
struct cache_entry *ce = next_cache_entry(o);
struct cache_entry *ce = next_cache_entry(o, &hint);
if (!ce)
break;
if (ce_in_traverse_path(ce, &info))
Expand All @@ -1879,8 +1889,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options

/* Any left-over entries in the index? */
if (o->merge) {
hint = -1;
while (1) {
struct cache_entry *ce = next_cache_entry(o);
struct cache_entry *ce = next_cache_entry(o, &hint);
if (!ce)
break;
if (unpack_index_entry(ce, o) < 0)
Expand Down