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

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

[Neeraj K. Singh via GitGitGadget]

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(-)

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

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

[Jeff Hostetler]

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 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:

```
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

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

[Junio C Hamano]

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.

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

[Neeraj Singh]

On Fri, Feb 19, 2021 at 11:46 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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

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

[Junio C Hamano]

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.

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

[Neeraj Singh]

On Sun, Feb 21, 2021 at 4:51 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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

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

[Junio C Hamano]

Neeraj Singh <nksingh85@gmail.com> writes:

> 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 ;-)

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

[Chris Torek]

> 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:
> 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

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

[Junio C Hamano]

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...

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

[Neeraj Singh]

On Wed, Feb 24, 2021 at 11:16 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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

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

[Chris Torek]

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