]> git.ipfire.org Git - thirdparty/git.git/commit - unpack-trees.c
unpack-trees: correctly compute result count
authorDerrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Fri, 10 Jan 2020 01:59:30 +0000 (01:59 +0000)
committerJunio C Hamano <gitster@pobox.com>
Fri, 10 Jan 2020 19:34:36 +0000 (11:34 -0800)
commit4c6c7971e0e42f0f1ce7e9472bc3aec8e0c96650
tree669d16fd615c46335e245fe613c40e20ceb2b3fc
parent757ff352bd66de31715580f8c525e54ad30b22eb
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>
unpack-trees.c