patchsha1_file: use strbuf_add() instead of strbuf_addf()

Replace use of strbuf_addf() with strbuf_add() when enumerating loose objects in for_each_file_in_obj_subdir(). Since we already check the length and hex-values of the string before consuming the path, we can prevent extra computation by using the lower- level method.

One consumer of for_each_file_in_obj_subdir() is the abbreviation code. OID abbreviations use a cached list of loose objects (per object subdirectory) to make repeated queries fast, but there is significant cache load time when there are many loose objects.

Most repositories do not have many loose objects before repacking, but in the GVFS case the repos can grow to have millions of loose objects. Profiling 'git log' performance in GitForWindows on a GVFS-enabled repo with ~2.5 million loose objects revealed 12% of the CPU time was spent in strbuf_addf().

Add a new performance test to p4211-line-log.sh that is more sensitive to this cache-loading. By limiting to 1000 commits, we more closely resemble user wait time when reading history into a pager.

For a copy of the Linux repo with two ~512 MB packfiles and ~572K loose objects, running 'git log --oneline --raw --parents -1000' had the following performance:

 HEAD~1            HEAD
----------------------------------------
 7.70(7.15+0.54)   7.44(7.09+0.29) -3.4%

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 sha1_file.c              | 7 ++++---
 t/perf/p4211-line-log.sh | 4 ++++
 2 files changed, 8 insertions(+), 3 deletions(-)

On Fri, Dec 01, 2017 at 12:49:56PM -0500, Derrick Stolee wrote:

Makes sense. It's a shame that "addf" is turning out to be so slow (I'm still mildly curious if that differs between Windows and glibc, but there are so many other variables between the two platforms it's hard to test).

Yeah, we haven't heavily micro-optimized the case for having lots of loose objects. The general philosophy about having lots of loose objects is: don't. You're generally going to pay a system call for each access, which is much heavier-weight than packfiles.

I think abbreviation-finding is the exception there, though, because we literally just readdir() the entries and never do anything else with them.

Thanks for including numbers. I think the setup here highlights a weakness of the perf suite that we've discussed: if there's a performance regression for your case, nobody is likely to notice because they won't test on a repo with 500k loose objects.

Probably "repo with a bunch of loose objects" ought to be a stock repository profile for doing regression runs.

This overall looks good, but I noticed one bug and a few cosmetic improvements.

Maybe:

  strbuf_addch(path, '/');

would be a bit more readable (it's also a tiny bit faster, but this isn't part of the tight loop).

We set this here so that the '/' is included as part of the base. Makes sense, but can we now drop the earlier setting of baselen before the opendir() call?

Moving this code into the conditional makes sense here, since that's where we know the actual size. But what about the case when we _don't_ hit this conditional. We still do:

  if (cruft_cb)
        cruft_cb(path->buf);

which is now broken (it will just get the directory name without the entry appended).

I think the optimized versions probably needs to be something more like:

  while (de = readdir(...)) {
      strbuf_setlen(path, baselen);
      if (size is HEXSZ - 2) {
          strbuf_add(path, de->d_name, HEXSZ - 2);
	  obj_cb(path->buf);
      } else if (cruft_cb) {
          strbuf_addstr(path, de->d_name);
	  cruft_cb(path->buf);
      }
  }

Two other thoughts:

  - from past discussions, I suspect most of your performance
    improvement actually comes from avoiding addf(), and the difference
    between addstr() and add() may be negligible here. It might be worth
    timing strbuf_addstr(). If that's similarly fast, that means we
    could keep the logic out of the conditional entirely.

  - there's an extra micro-optimization there, which is that if there's
    no obj_cb, we have no need to assemble the full path at all. I doubt
    it makes much of a difference, as most callers would pass an object
    callback (I'd be surprised if we even have one that doesn't).

-Peff

On 12/1/2017 1:22 PM, Jeff King wrote:

[snip]

Thanks for finding quality improvements to this patch. I'll let it sit over the weekend for more feedback before sending a v2.

Yeah, probably. I had briefly considered just adding the '/' before the first assignment of "baselen", but didn't want to change the error output. I also don't know if there are side effects for other platforms by calling opendir() with a '/'-terminated path.

Good catch! A big reason to pull it inside and use strbuf_add over strbuf_addstr is to avoid a duplicate strlen() calculation. However, I can store the length before the conditional.

Small change by storing the length in advance of the conditional:

while (de = readdir(...)) {
     int namelen = strlen(de->d_name);
     strbuf_setlen(path, baselen);
     strbuf_add(path, de->d_name, namelen);

     if (namelen == HEXSZ - 2)
         obj_cb(path->buf)
     else
         cruft_cb(path->buf);
}

addstr() duplicates the strlen(), which isn't much but we might as well save cycles where we can if it isn't too hard.

After doing a few 'git grep' commands, I found several that include a NULL cruft_cb but none that have a NULL obj_cb.

Thanks, -Stolee

On Fri, Dec 01, 2017 at 02:50:05PM -0500, Derrick Stolee wrote:

I noticed that, too. Since it's so easy to keep doing the opendir without the slash, I'd prefer to avoid finding out if there are such platforms. :)

I'd give 50/50 odds no whether a compiler could optimize out that strlen. We inline addstr exactly so that callsites can see that strlen (it's primarily for string literals, where it can become a compile-time constant, but I think it could apply here). But sometimes C's pointer aliasing rules can be surprising in blocking "obviously correct" optimizations like that.

The generated asm is a little dense, but I _think_ "gcc -O2" does in fact do this with a single strlen based on the following tweak on top of your patch:

Thanks for the feedback on v1. This version fixes the cruft_cb bug and streamlines the strlen(). I would include an inter-diff but it was the same size as the patch.

Thanks, -Stolee

---

Replace use of strbuf_addf() with strbuf_add() when enumerating loose objects in for_each_file_in_obj_subdir(). Since we already check the length and hex-values of the string before consuming the path, we can prevent extra computation by using the lower- level method.

One consumer of for_each_file_in_obj_subdir() is the abbreviation code. OID abbreviations use a cached list of loose objects (per object subdirectory) to make repeated queries fast, but there is significant cache load time when there are many loose objects.

Most repositories do not have many loose objects before repacking, but in the GVFS case the repos can grow to have millions of loose objects. Profiling 'git log' performance in GitForWindows on a GVFS-enabled repo with ~2.5 million loose objects revealed 12% of the CPU time was spent in strbuf_addf().

Add a new performance test to p4211-line-log.sh that is more sensitive to this cache-loading. By limiting to 1000 commits, we more closely resemble user wait time when reading history into a pager.

For a copy of the Linux repo with two ~512 MB packfiles and ~572K loose objects, running 'git log --oneline --parents --raw -1000' had the following performance:

 HEAD~1            HEAD
----------------------------------------
 7.70(7.15+0.54)   7.44(7.09+0.29) -3.4%

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 sha1_file.c              | 12 +++++++-----
 t/perf/p4211-line-log.sh |  4 ++++
 2 files changed, 11 insertions(+), 5 deletions(-)

On Mon, Dec 04, 2017 at 09:06:03AM -0500, Derrick Stolee wrote:

Thanks, this version looks good to me. One procedural nit:

When you put your cover letter first, you need to use "scissors" like:

-- >8 --

to separate it from the commit message. Using three-dashes means "git am" will include your cover letter as the commit message, and omit your real commit message entirely.

-Peff

On 12/4/2017 11:56 AM, Jeff King wrote:

Thanks. I updated our internal best-practices accordingly.

-Stolee