Re: git status OOM on mmap of large file

[Joey Hess <id@joeyh.name>]

Jeff King wrote:
> Looking at apply_single_file_filter(), it's not the _original_ file that
> it's trying to store, but rather the data coming back from the filter.
> It's just that we use the original file size as a hint!

Thanks much for working that out!

> In other words, I think this patch fixes your problem:
> 
> diff --git a/convert.c b/convert.c
> index 0d89ae7c23..85aebe2ed3 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -732,7 +732,7 @@ static int apply_single_file_filter(const char *path, const char *src, size_t le
>  	if (start_async(&async))
>  		return 0;	/* error was already reported */
>  
> -	if (strbuf_read(&nbuf, async.out, len) < 0) {
> +	if (strbuf_read(&nbuf, async.out, 0) < 0) {
>  		err = error(_("read from external filter '%s' failed"), cmd);
>  	}
>  	if (close(async.out)) {

Yes, confirmed that does fix it.

> though possibly we should actually continue to use the file size as a
> hint up to a certain point, which avoids reallocations for more "normal"
> filters where the input and output sizes are in the same ballpark.
> 
> Just off the top of my head, something like:
> 
>   /* guess that the filtered output will be the same size as the original */
>   hint = len;
> 
>   /* allocate 10% extra in case the clean size is slightly larger */
>   hint *= 1.1;
> 
>   /*
>    * in any case, never go higher than half of core.bigfileThreshold.
>    * We'd like to avoid allocating more bytes than that, and that still
>    * gives us room for our strbuf to preemptively double if our guess is
>    * just a little on the low side.
>    */
>   if (hint > big_file_threshold / 2)
> 	hint = big_file_threshold / 2;
> 
> But to be honest, I have no idea if that would even produce measurable
> benefits over simply growing the strbuf from scratch (i.e., hint==0).

Half of 512 MB is still quite a lot of memory to default to using in
this situation. Eg smaller VPS's still often only have a GB or two of ram.

When the clean filter is being used in a way that doesn't involve hashes
of large files, it will mostly be operating on typically sized source
code files. So capping the maximum hint size around the size of a typical
source code file would be plenty for both common cases for the clean filter.

I did some benchmarking, using cat as the clean filter:

git status 32 kb file, hint == len
   time                 3.865 ms   (3.829 ms .. 3.943 ms)
                        0.994 R²   (0.987 R² .. 0.999 R²)
   mean                 3.934 ms   (3.889 ms .. 4.021 ms)
   std dev              191.8 μs   (106.8 μs .. 291.8 μs)
git status 32 kb file, hint == 0
   time                 3.887 ms   (3.751 ms .. 4.064 ms)
                        0.992 R²   (0.986 R² .. 0.998 R²)
   mean                 4.002 ms   (3.931 ms .. 4.138 ms)
   std dev              292.2 μs   (189.0 μs .. 498.3 μs)
git status 1 mb file, hint == len
   time                 3.942 ms   (3.816 ms .. 4.078 ms)
                        0.995 R²   (0.991 R² .. 0.999 R²)
   mean                 3.969 ms   (3.916 ms .. 4.054 ms)
   std dev              220.1 μs   (155.1 μs .. 304.3 μs)
git status 1 mb file, hint == 0
   time                 3.869 ms   (3.836 ms .. 3.911 ms)
                        0.998 R²   (0.995 R² .. 1.000 R²)
   mean                 3.895 ms   (3.868 ms .. 3.947 ms)
   std dev              112.3 μs   (47.93 μs .. 182.7 μs)
git status 1 gb file, hint == len
   time                 7.173 s    (6.834 s .. 7.564 s)
                        0.999 R²   (0.999 R² .. 1.000 R²)
   mean                 7.560 s    (7.369 s .. 7.903 s)
   std dev              333.2 ms   (27.65 ms .. 412.2 ms)
git status 1 gb file, hint == 0
   time                 7.652 s    (6.307 s .. 8.263 s)
                        0.996 R²   (0.992 R² .. 1.000 R²)
   mean                 8.082 s    (7.843 s .. 8.202 s)
   std dev              232.3 ms   (2.362 ms .. 277.1 ms)

From this, it looks like the file has to be quite large before the
preallocation makes a sizable improvement to runtime, and the
smudge/clean filters have to be used for actual content filtering
(not for hash generation purposes as git-annex and git-lfs use it).
An unusual edge case I think. So hint == 0 seems fine.

-- 
see shy jo