threads / patch / 55168

patchread-cache: make the index write buffer size 128K

Subject: [PATCH] read-cache: make the index write buffer size 128K

## tl;dr

11 messages between Feb 18, 2021 and Feb 25, 2021. Diffs are folded; open one to read it.

replies: 10people: 5as markdown or json

Neeraj K. Singh via GitGitGadget· Feb 18, 2021, 02:48 UTC · lore
From: Neeraj Singh <neerajsi@ntdev.microsoft.com>

Writing an index 8K at a time invokes the OS filesystem and caching code very frequently, introducing noticeable overhead while writing large indexes. When experimenting with different write buffer sizes on Windows writing the Windows OS repo index (260MB), most of the benefit came by bumping the index write buffer size to 64K. I picked 128K to ensure that we're past the knee of the curve.

With this change, the time under do_write_index for an index with 3M files goes from ~1.02s to ~0.72s.

Signed-off-by: Neeraj Singh <neerajsi@ntdev.microsoft.com>
---
    read-cache: make the index write buffer size 128K
    
    Writing an index 8K at a time invokes the OS filesystem and caching code
    very frequently, introducing noticeable overhead while writing large
    indexes. When experimenting with different write buffer sizes on Windows
    writing the Windows OS repo index (260MB), most of the benefit came by
    bumping the index write buffer size to 64K. I picked 128K to ensure that
    we're past the knee of the curve.
    
    With this change, the time under do_write_index for an index with 3M
    files goes from ~1.02s to ~0.72s.
    
    Signed-off-by: Neeraj Singh neerajsi@ntdev.microsoft.com
    
    Note: This was previously discussed on the mailing list in 2016 at:
    https://lore.kernel.org/git/1458350341-12276-1-git-send-email-dturner@twopensource.com/.
    
    Since then, I believe we have a couple changes:
    
     * 'small' development platforms like raspberry pi have gotten larger
       (4GB RAM).
     * spectre and meltdown make individual system calls more expensive when
       mitigations are enabled
     * there have been many investments to make very large repos scale well
       in git, so huge repos are more common now.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-877%2Fneerajsi-msft%2Fneerajsi%2Findex-buffer-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-877/neerajsi-msft/neerajsi/index-buffer-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/877
 read-cache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to read-cache.c +1 −1
diff --git a/read-cache.c b/read-cache.c
index 29144cf879e7..a5b2779b9586 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2447,7 +2447,7 @@ int repo_index_has_changes(struct repository *repo,
 	}
 }
 
-#define WRITE_BUFFER_SIZE 8192
+#define WRITE_BUFFER_SIZE (128 * 1024)
 static unsigned char write_buffer[WRITE_BUFFER_SIZE];
 static unsigned long write_buffer_len;
 

base-commit: 45526154a57d15947cad7262230d0b935cedb9d3

-- 
gitgitgadget
Jeff Hostetler· Feb 19, 2021, 19:12 UTC · re: Neeraj K. Singh via GitGitGadget · lore

Re: [PATCH] read-cache: make the index write buffer size 128K

On 2/17/21 9:48 PM, Neeraj K. Singh via GitGitGadget wrote:
Show 11 quoted lines
> From: Neeraj Singh <neerajsi@ntdev.microsoft.com>
> 
> Writing an index 8K at a time invokes the OS filesystem and caching code
> very frequently, introducing noticeable overhead while writing large
> indexes. When experimenting with different write buffer sizes on Windows
> writing the Windows OS repo index (260MB), most of the benefit came by
> bumping the index write buffer size to 64K. I picked 128K to ensure that
> we're past the knee of the curve.
> 
> With this change, the time under do_write_index for an index with 3M
> files goes from ~1.02s to ~0.72s.
[...]
Show 5 quoted lines
>   
> -#define WRITE_BUFFER_SIZE 8192
> +#define WRITE_BUFFER_SIZE (128 * 1024)
>   static unsigned char write_buffer[WRITE_BUFFER_SIZE];
>   static unsigned long write_buffer_len;
[...]
Very nice.

I can confirm that this gives nice gains on Windows. (I'm using the Office repo which has a 188MB index file (2.1M files at HEAD). Running "git status" shows a gain of about 200ms.

We get a smaller gain on Mac of about 50ms (again, using the Office repo).

So, you may add my sign-off or ACK to this.
     Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>

FWIW, You might take a look at `t/perf/p0007-write-cache.sh` Update it as follows:

```
Show changes to t/perf/p0007-write-cache.sh +1 −1
diff --git a/t/perf/p0007-write-cache.sh b/t/perf/p0007-write-cache.sh
index 09595264f0..337280ff1c 100755
--- a/t/perf/p0007-write-cache.sh
+++ b/t/perf/p0007-write-cache.sh
@@ -4,7 +4,8 @@ test_description="Tests performance of writing the index"

  . ./perf-lib.sh

-test_perf_default_repo
+test_perf_large_repo

  test_expect_success "setup repo" '
         if git rev-parse --verify refs/heads/p0006-ballast^{commit}
```


Then you can run it like this:

     $ cd t/perf
     $ GIT_PERF_LARGE_REPO=/path/to/your/enlistment ./p0007-write-cache

Then you can run it with the small and then with the large buffer and
get times for essentially just the index write in isolation.

Hope this helps,
Jeff
Junio C Hamano· Feb 20, 2021, 03:28 UTC · re: Jeff Hostetler · lore

Re: [PATCH] read-cache: make the index write buffer size 128K

Jeff Hostetler <git@jeffhostetler.com> writes:
Show 22 quoted lines
> On 2/17/21 9:48 PM, Neeraj K. Singh via GitGitGadget wrote:
>> From: Neeraj Singh <neerajsi@ntdev.microsoft.com>
>> Writing an index 8K at a time invokes the OS filesystem and caching
>> code
>> very frequently, introducing noticeable overhead while writing large
>> indexes. When experimenting with different write buffer sizes on Windows
>> writing the Windows OS repo index (260MB), most of the benefit came by
>> bumping the index write buffer size to 64K. I picked 128K to ensure that
>> we're past the knee of the curve.
>> With this change, the time under do_write_index for an index with 3M
>> files goes from ~1.02s to ~0.72s.
>
> [...]
>
>>   -#define WRITE_BUFFER_SIZE 8192
>> +#define WRITE_BUFFER_SIZE (128 * 1024)
>>   static unsigned char write_buffer[WRITE_BUFFER_SIZE];
>>   static unsigned long write_buffer_len;
>
> [...]
>
> Very nice.
I wonder if we gain more by going say 4M buffer size or even larger?

Is this something we can make the system auto-tune itself? This is not about reading but writing, so we already have enough information to estimate how much we would need to write out.

Thanks.
Neeraj Singh· Feb 20, 2021, 07:56 UTC · re: Junio C Hamano · lore

Re: [PATCH] read-cache: make the index write buffer size 128K

On Fri, Feb 19, 2021 at 11:46 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 34 quoted lines
>
> Jeff Hostetler <git@jeffhostetler.com> writes:
>
> > On 2/17/21 9:48 PM, Neeraj K. Singh via GitGitGadget wrote:
> >> From: Neeraj Singh <neerajsi@ntdev.microsoft.com>
> >> Writing an index 8K at a time invokes the OS filesystem and caching
> >> code
> >> very frequently, introducing noticeable overhead while writing large
> >> indexes. When experimenting with different write buffer sizes on Windows
> >> writing the Windows OS repo index (260MB), most of the benefit came by
> >> bumping the index write buffer size to 64K. I picked 128K to ensure that
> >> we're past the knee of the curve.
> >> With this change, the time under do_write_index for an index with 3M
> >> files goes from ~1.02s to ~0.72s.
> >
> > [...]
> >
> >>   -#define WRITE_BUFFER_SIZE 8192
> >> +#define WRITE_BUFFER_SIZE (128 * 1024)
> >>   static unsigned char write_buffer[WRITE_BUFFER_SIZE];
> >>   static unsigned long write_buffer_len;
> >
> > [...]
> >
> > Very nice.
>
> I wonder if we gain more by going say 4M buffer size or even larger?
>
> Is this something we can make the system auto-tune itself?  This is
> not about reading but writing, so we already have enough information
> to estimate how much we would need to write out.
>
> Thanks.
>

Hi Junio, At some point the cost of the memcpy into the filesystem cache begins to dominate the cost of the system call, so increasing the buffer size has diminishing returns.

An alternate approach would be to mmap the index file we are trying to write and thereby copy the data directly into the filesystem cache pages. That's a much more difficult change to make and verify, so I'd rather leave that as an exercise to the reader for now :).

Thanks, -Neeraj

Junio C Hamano· Feb 21, 2021, 12:51 UTC · re: Neeraj Singh · lore

Re: [PATCH] read-cache: make the index write buffer size 128K

Neeraj Singh <nksingh85@gmail.com> writes:
Show 22 quoted lines
>> >>   -#define WRITE_BUFFER_SIZE 8192
>> >> +#define WRITE_BUFFER_SIZE (128 * 1024)
>> >>   static unsigned char write_buffer[WRITE_BUFFER_SIZE];
>> >>   static unsigned long write_buffer_len;
>> >
>> > [...]
>> >
>> > Very nice.
>>
>> I wonder if we gain more by going say 4M buffer size or even larger?
>>
>> Is this something we can make the system auto-tune itself?  This is
>> not about reading but writing, so we already have enough information
>> to estimate how much we would need to write out.
>>
>> Thanks.
>>
>
> Hi Junio,
> At some point the cost of the memcpy into the filesystem cache begins to
> dominate the cost of the system call, so increasing the buffer size
> has diminishing returns.
Yes, I know that kind of "general principle".

If I recall correctly, we used to pass too large a buffer to a single write(2) system call (I do not know if it was for the index---I suspect it was for some other data), and found out that it made response to ^C take too long, and tuned the buffer size down.

I was asking where the sweet spot for this codepath would be, and if we can take a measurement to make a better decision than "8k feels too small and 128k turns out to be better than 8k". It does not tell us if 128k would always do better than 64k or 256k, for example.

I suspect that the sweet spot would be dependent on many parameters (not just the operating system, but also relative speed among memory, "disk", and cpu, and also the size of the index) and if we can devise a way to auto-tune it so that we do not have to worry about it.

Thanks.
Neeraj Singh· Feb 24, 2021, 20:56 UTC · re: Junio C Hamano · lore

Re: [PATCH] read-cache: make the index write buffer size 128K

On Sun, Feb 21, 2021 at 4:51 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 46 quoted lines
>
> Neeraj Singh <nksingh85@gmail.com> writes:
>
> >> >>   -#define WRITE_BUFFER_SIZE 8192
> >> >> +#define WRITE_BUFFER_SIZE (128 * 1024)
> >> >>   static unsigned char write_buffer[WRITE_BUFFER_SIZE];
> >> >>   static unsigned long write_buffer_len;
> >> >
> >> > [...]
> >> >
> >> > Very nice.
> >>
> >> I wonder if we gain more by going say 4M buffer size or even larger?
> >>
> >> Is this something we can make the system auto-tune itself?  This is
> >> not about reading but writing, so we already have enough information
> >> to estimate how much we would need to write out.
> >>
> >> Thanks.
> >>
> >
> > Hi Junio,
> > At some point the cost of the memcpy into the filesystem cache begins to
> > dominate the cost of the system call, so increasing the buffer size
> > has diminishing returns.
>
> Yes, I know that kind of "general principle".
>
> If I recall correctly, we used to pass too large a buffer to a
> single write(2) system call (I do not know if it was for the
> index---I suspect it was for some other data), and found out that it
> made response to ^C take too long, and tuned the buffer size down.
>
> I was asking where the sweet spot for this codepath would be, and if
> we can take a measurement to make a better decision than "8k feels
> too small and 128k turns out to be better than 8k".  It does not
> tell us if 128k would always do better than 64k or 256k, for
> example.
>
> I suspect that the sweet spot would be dependent on many parameters
> (not just the operating system, but also relative speed among
> memory, "disk", and cpu, and also the size of the index) and if we
> can devise a way to auto-tune it so that we do not have to worry
> about it.
>
> Thanks.

I think the main concern on a reasonably-configured machine is the speed of memcpy and the cost of the code to get to that memcpy (syscall, file system free space allocator, page allocator, mapping from file offset to cache page). Disk shouldn't matter, since we write the file with OS buffering and buffer flushing will happen asynchronously some time after the git command completes.

If we think about doing the fastest possible memcpy, I think we want to aim for maximizing the use of the CPU cache. A write buffer that's too big would result in most of the data being flushed to DRAM between when git writes it and the OS reads it. L1 caches are typically ~32K and L2 caches are on the order of 256K. We probably don't want to exceed the size of the L2 cache, and we should actually leave some room for OS code and data, so 128K is a good number from that perspective.

I collected data from an experiment with different buffer sizes on Windows on my 3.6Ghz Xeon W-2133 machine: https://docs.google.com/spreadsheets/d/1Bu6pjp53NPDK6AKQI_cry-hgxEqlicv27dptoXZYnwc/edit?usp=sharing

The timing is pretty much in the noise after we pass 32K. So I think 8K is too small, but given the flatness of the curve we can feel good about any value above 32K from a performance perspective. I still think 128K is a decent number that won't likely need to be changed for some time.

Thanks, -Neeraj

Junio C Hamano· Feb 25, 2021, 05:41 UTC · re: Neeraj Singh · lore

Re: [PATCH] read-cache: make the index write buffer size 128K

Neeraj Singh <nksingh85@gmail.com> writes:
Show 21 quoted lines
> If we think about doing the fastest possible memcpy, I think we want to aim for
> maximizing the use of the CPU cache.  A write buffer that's too big would result
> in most of the data being flushed to DRAM between when git writes it and the
> OS reads it.  L1 caches are typically ~32K and L2 caches are on the
> order of 256K.
> We probably don't want to exceed the size of the L2 cache, and we
> should actually
> leave some room for OS code and data, so 128K is a good number from
> that perspective.
>
> I collected data from an experiment with different buffer sizes on Windows on my
> 3.6Ghz Xeon W-2133 machine:
> https://docs.google.com/spreadsheets/d/1Bu6pjp53NPDK6AKQI_cry-hgxEqlicv27dptoXZYnwc/edit?usp=sharing
>
> The timing is pretty much in the noise after we pass 32K.  So I think
> 8K is too small, but
> given the flatness of the curve we can feel good about any value above
> 32K from a performance
> perspective.  I still think 128K is a decent number that won't likely
> need to be changed for
> some time.
Thanks for a supporting graph.

I can very well imagine that it would have been tempting to instead say "after we pass 128k" while explaining exactly the same graph, and doing so would have given a more coherent argument to support the choice of 128k the patch made. You knew that a "then perhaps we can reclaim 96k by sizing the buffer down a bit?" would become a reasonable response, but you still chose to be honest, which I kinda like ;-)

Chris Torek· Feb 25, 2021, 06:58 UTC · re: Junio C Hamano · lore

Re: [PATCH] read-cache: make the index write buffer size 128K

Show 12 quoted lines
> Neeraj Singh <nksingh85@gmail.com> writes:
> > I collected data from an experiment with different buffer sizes on Windows on my
> > 3.6Ghz Xeon W-2133 machine:
> > https://docs.google.com/spreadsheets/d/1Bu6pjp53NPDK6AKQI_cry-hgxEqlicv27dptoXZYnwc/edit?usp=sharing
> >
> > The timing is pretty much in the noise after we pass 32K.  So I think
> > 8K is too small, but
> > given the flatness of the curve we can feel good about any value above
> > 32K from a performance
> > perspective.  I still think 128K is a decent number that won't likely
> > need to be changed for
> > some time.

Linux/BSD/etc `stat` system calls report st_blksize values to tell user code the optimal size for read and write calls. Does Windows have one? (It's not POSIX but is XSI.)

(How *well* the OS reports `st_blksize` is another question entirely, but at least if the report says, say, 128k, and that's wrong, that's no longer Git's fault. :-) )

On Wed, Feb 24, 2021 at 10:46 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 9 quoted lines
> Thanks for a supporting graph.
>
> I can very well imagine that it would have been tempting to instead
> say "after we pass 128k" while explaining exactly the same graph,
> and doing so would have given a more coherent argument to support
> the choice of 128k the patch made.  You knew that a "then perhaps we
> can reclaim 96k by sizing the buffer down a bit?" would become a
> reasonable response, but you still chose to be honest, which I kinda
> like ;-)

128K is correct for ZFS; 64K is typically correct for UFS2; 8K is the old UFS1 size. Anything under that has been too small for a long time. :-)

Chris
Junio C Hamano· Feb 25, 2021, 07:16 UTC · re: Chris Torek · lore

Re: [PATCH] read-cache: make the index write buffer size 128K

Chris Torek <chris.torek@gmail.com> writes:
Show 11 quoted lines
> Linux/BSD/etc `stat` system calls report st_blksize values to tell
> user code the optimal size for read and write calls.  Does Windows
> have one?  (It's not POSIX but is XSI.)
>
> (How *well* the OS reports `st_blksize` is another question
> entirely, but at least if the report says, say, 128k, and that's
> wrong, that's no longer Git's fault. :-) )
> ...
> 128K is correct for ZFS; 64K is typically correct for UFS2; 8K is
> the old UFS1 size.  Anything under that has been too small for
> a long time. :-)

That's rather tempting. After opening a locked index to write things out, the value is a single fstat() away...

Neeraj Singh· Feb 25, 2021, 07:36 UTC · re: Junio C Hamano · lore

Re: [PATCH] read-cache: make the index write buffer size 128K

On Wed, Feb 24, 2021 at 11:16 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 18 quoted lines
>
> Chris Torek <chris.torek@gmail.com> writes:
>
> > Linux/BSD/etc `stat` system calls report st_blksize values to tell
> > user code the optimal size for read and write calls.  Does Windows
> > have one?  (It's not POSIX but is XSI.)
> >
> > (How *well* the OS reports `st_blksize` is another question
> > entirely, but at least if the report says, say, 128k, and that's
> > wrong, that's no longer Git's fault. :-) )
> > ...
> > 128K is correct for ZFS; 64K is typically correct for UFS2; 8K is
> > the old UFS1 size.  Anything under that has been too small for
> > a long time. :-)
>
> That's rather tempting.  After opening a locked index to write
> things out, the value is a single fstat() away...
>

From a quick perusal of freebsd, st_blksize seems to be the system PAGE_SIZE by default (4k most of the time, I assume). The Windows equivalent of this value is really tuned to what you want to send down when bypassing the cache (to avoid partial cluster/stripe writes).

https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_stat.h.html doesn't elicit much confidence. The units of st_blksize aren't even defined.

Thanks, Neeraj

Chris Torek· Feb 25, 2021, 07:57 UTC · re: Neeraj Singh · lore

Re: [PATCH] read-cache: make the index write buffer size 128K

On Wed, Feb 24, 2021 at 11:36 PM Neeraj Singh <nksingh85@gmail.com> wrote:
> From a quick perusal of freebsd, st_blksize seems to be the system
> PAGE_SIZE by default (4k most of the time, I assume). The Windows
> equivalent of this value is really tuned to what you want to send down
> when bypassing the cache (to avoid partial cluster/stripe writes).

It's page-size for pipes, sockets, etc., but for real files, it's based on a report from the underlying file system. It's actually 8k on a typical ancient UFS file system, 64K on UFS2, and 128K on ZFS, on FreeBSD.

> https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_stat.h.html
> doesn't elicit much confidence. The units of st_blksize aren't even
> defined.

Despite POSIX's rather obstreperous definition of st_blksize, the units are actually just bytes, in practice.

Chris

← back to recent threads