Skip to content

Commit

Permalink
unpack-trees: correctly compute result count
Browse files Browse the repository at this point in the history
The clear_ce_flags_dir() method processes the cache entries within
a common directory. The returned int is the number of cache entries
processed by that directory. When using the sparse-checkout feature
in cone mode, we can skip the pattern matching for entries in the
directories that are entirely included or entirely excluded.

eb42fec (unpack-trees: hash less in cone mode, 2019-11-21)
introduced this performance feature. The old mechanism relied on
the counts returned by calling clear_ce_flags_1(), but the new
mechanism calculated the number of rows by subtracting "cache_end"
from "cache" to find the size of the range. However, the equation
is wrong because it divides by sizeof(struct cache_entry *). This
is not how pointer arithmetic works!

A coverity build of Git for Windows in preparation for the 2.25.0
release found this issue with the warning, "Pointer differences,
such as cache_end - cache, are automatically scaled down by the
size (8 bytes) of the pointed-to type (struct cache_entry *).
Most likely, the division by sizeof(struct cache_entry *) is
extraneous and should be eliminated." This warning is correct.

This leaves us with the question "how did this even work?" The
problem that occurs with this incorrect pointer arithmetic is
a performance-only bug, and a very slight one at that. Since
the entry count returned by clear_ce_flags_dir() is reduced by
a factor of 8, the loop in clear_ce_flags_1() will re-process
entries from those directories.

By inserting global counters into unpack-tree.c and tracing
them with trace2_data_intmax() (in a private change, for
testing), I was able to see count how many times the loop inside
clear_ce_flags_1() processed an entry and how many times
clear_ce_flags_dir() was called. Each of these are reduced by at
least a factor of 8 with the current change. A factor larger
than 8 happens when multiple levels of directories are repeated.

Specifically, in the Linux kernel repo, the command

	git sparse-checkout set LICENSES

restricts the working directory to only the files at root and
in the LICENSES directory. Here are the measured counts:

clear_ce_flags_1 loop blocks:
	Before: 11,520
	After:   1,621

clear_ce_flags_dir calls:
	Before: 7,048
	After:    606

While these are dramatic counts, the time spent in
clear_ce_flags_1() is under one millisecond in each case, so
the improvement is not measurable as an end-to-end time.

Reported-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
  • Loading branch information
Derrick Stolee via GitGitGadget authored and gitster committed Jan 10, 2020
1 parent 757ff35 commit 4c6c797
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions unpack-trees.c
Expand Up @@ -1305,14 +1305,14 @@ static int clear_ce_flags_dir(struct index_state *istate,

if (pl->use_cone_patterns && orig_ret == MATCHED_RECURSIVE) {
struct cache_entry **ce = cache;
rc = (cache_end - cache) / sizeof(struct cache_entry *);
rc = cache_end - cache;

while (ce < cache_end) {
(*ce)->ce_flags &= ~clear_mask;
ce++;
}
} else if (pl->use_cone_patterns && orig_ret == NOT_MATCHED) {
rc = (cache_end - cache) / sizeof(struct cache_entry *);
rc = cache_end - cache;
} else {
rc = clear_ce_flags_1(istate, cache, cache_end - cache,
prefix,
Expand Down

0 comments on commit 4c6c797

Please sign in to comment.