mirror of
https://github.com/git/git.git
synced 2024-11-24 10:26:17 +08:00
unpack-trees: correctly compute result count
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.
eb42feca
(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>
This commit is contained in:
parent
757ff352bd
commit
4c6c7971e0
@ -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,
|
||||
|
Loading…
Reference in New Issue
Block a user