[re-adding list to cc; I think there are some interesting bits for
discussion here]
On Mon, Jun 01, 2020 at 08:41:08AM +0200, Jan Christoph Uhde wrote:
quoted
Is it possible that your local repository has large number of packs? Git
will leave open maps to each pack's index file, plus some packs
themselves (ones we're accessing, plus we map+close small ones), plus
whatever maps are used by libc to malloc. The kernel default limit for
the number of maps is 65530. If you have on the order of 30,000 packs
you might run into this limit.
You can check the number of packs with "git count-objects -v", and the
» git count-objects -v
count: 0
size: 0
in-pack: 2399361
packs: 1
size-pack: 919395
prune-packable: 0
garbage: 0
size-garbage: 0
OK, so that's just one pack (with a few million objects, which is
expected).
quoted
If that's the problem, the solution is to repack (which should also
generally improve performance). If you have trouble repacking due to the
limits, you can overcome the chicken and egg with:
sysctl -w vm.max_map_count=131060
This fixes the problem indeed!
Now that surprises me. However, I can reproduce the issue with a fresh
clone of gcc:
$ git clone https://github.com/gcc-mirror/gcc
$ cd gcc
$ git diff --quiet ;# works!
$ find . -type f | xargs touch ;# dirty the index entry for each file
$ git diff --quiet
fatal: mmap failed: Cannot allocate memory
There are ~100k files in that repo. If we mmap each one as part of
diff_populate_filespec(), we're going to end up with too many maps.
So we can easily reproduce this case without having to download the huge
gcc repo:
git init repo
cd repo
for i in $(seq 70); do
mkdir $i
for j in $(seq 1000); do
echo "foo" >$i/$j
done
done
git add .
git commit -m 'add a bunch of files'
git diff --quiet
git ls-files | xargs touch
git diff --quiet
What could be the reason that the problem only occurs with the `--quiet` flag
that I use in my prompt command, but not when using `git diff` without the flag.
That is curious, and I can reproduce it here.
In the non-quiet case, we queue each filepair that we find to be
stat-dirty. And then we call diffcore_skip_stat_unmatch() from
diffcore_std() on the whole list. We load each file singly, see that
it's not really a change (the index entries are just stat-dirty because
of our touch), and then discard it.
Whereas in the --quiet case, we hit this code in diff_change when
queuing each:
if (options->flags.quick && options->skip_stat_unmatch &&
!diff_filespec_check_stat_unmatch(options->repo, p))
return;
options->flags.has_changes = 1;
That code is trying to make sure we accurately set the has_changes flag
in quick/quiet mode (because we can stop diffing as soon as we see a
single change). Since none of these changes is interesting (they're all
just stat-dirty entries), we have to keep going and look at each one.
But this code leaves the populated filespec in the queue. Doing this
makes "diff --quiet" succeed in this case:
diff --git a/diff.c b/diff.c
index d1ad6a3c4a..2535614735 100644
--- a/diff.c
+++ b/diff.c
@@ -6758,8 +6758,15 @@ void diff_change(struct diff_options *options,
return;
if (options->flags.quick && options->skip_stat_unmatch &&
- !diff_filespec_check_stat_unmatch(options->repo, p))
+ !diff_filespec_check_stat_unmatch(options->repo, p)) {
+ /*
+ * discard any populated data; this entry is uninteresting;
+ * we probably ought to avoid queuing the pair at all!
+ */
+ diff_free_filespec_data(p->one);
+ diff_free_filespec_data(p->two);
return;
+ }
options->flags.has_changes = 1;
}
But that's only because these aren't "real" changes. If we swap out our
"touch" for:
git ls-files | while read fn; do
echo bar >$fn
done
then we have real changes. Our --quiet case will exit immediately once
it sees a change, so it's OK. But now the non-quiet one will fail,
because it's going to load each file to confirm that it's an actual
change, but leave the mmap in place for when we do the actual
content-level diff.
So I think in general we do have problems diffing more than 65k working
tree files in a single process because of this limit. It may still be
worth doing something along the lines of the patch above, though,
because it seems more likely for somebody to have 65k stat-dirty files
than 65k actual changes.
-Peff
On Mon, Jun 01, 2020 at 12:54:08PM -0400, Jeff King wrote:
quoted
What could be the reason that the problem only occurs with the `--quiet` flag
that I use in my prompt command, but not when using `git diff` without the flag.
That is curious, and I can reproduce it here.
This turned out to be quite an interesting bug to look at. The fix is
simple, but I tried to summarize my explorations below.
I'm still curious why all of your files were stat-dirty. :) As a
workaround until you have a version of Git with the fix, you can
manually refresh the index with:
git update-index --refresh
before running git-diff.
-- >8 --
Subject: [PATCH] diff: discard blob data from stat-unmatched pairs
When performing a tree-level diff against the working tree, we may find
that our index stat information is dirty, so we queue a filepair to be
examined later. If the actual content hasn't changed, we call this a
stat-unmatch; the stat information was out of date, but there's no
actual diff. Normally diffcore_std() would detect and remove these
identical filepairs via diffcore_skip_stat_unmatch(). However, when
"--quiet" is used, we want to stop the diff as soon as we see any
changes, so we check for stat-unmatches immediately in diff_change().
That check may require us to actually load the file contents into the
pair of diff_filespecs. If we find that the pair isn't a stat-unmatch,
then no big deal; we'd likely load the contents later anyway to generate
a patch, do rename detection, etc, so we want to hold on to it. But if
it a stat-unmatch, then we have no more use for that data; the whole
point is that we're going discard the pair. However, we never free the
allocated diff_filespec data.
In most cases, keeping that data isn't a problem. We don't expect a lot
of stat-unmatch entries, and since we're using --quiet, we'd quit as
soon as we saw such a real change anyway. However, there are extreme
cases where it makes a big difference:
1. We'd generally mmap() the working tree half of the pair. And since
the OS may limit the total number of maps, we can run afoul of this
in large repositories. E.g.:
$ cd linux
$ git ls-files | wc -l
67959
$ sysctl vm.max_map_count
vm.max_map_count = 65530
$ git ls-files | xargs touch ;# everything is stat-dirty!
$ git diff --quiet
fatal: mmap failed: Cannot allocate memory
It should be unusual to have so many files stat-dirty, but it's
possible if you've just run a script like "sed -i" or similar.
After this patch, the above correctly exits with code 0.
2. Even if you don't hit mmap limits, the index half of the pair will
have been pulled from the object database into heap memory. Again
in a clone of linux.git, running:
$ git ls-files | head -n 10000 | xargs touch
$ git diff --quiet
peaks at 145MB heap before this patch, and 94MB after.
This patch solves the problem by freeing any diff_filespec data we
picked up during the "--quiet" stat-unmatch check in diff_changes.
Nobody is going to need that data later, so there's no point holding on
to it. There are a few things to note:
- we could skip queueing the pair entirely, which could in theory save
a little work. But there's not much to save, as we need a
diff_filepair to feed to diff_filespec_check_stat_unmatch() anyway.
And since we cache the result of the stat-unmatch checks, a later
call to diffcore_skip_stat_unmatch() call will quickly skip over
them. The diffcore code also counts up the number of stat-unmatched
pairs as it removes them. It's doubtful any callers would care about
that in combination with --quiet, but we'd have to reimplement the
logic here to be on the safe side. So it's not really worth the
trouble.
- I didn't write a test, because we always produce the correct output
unless we run up against system mmap limits, which are both
unportable and expensive to test against. Measuring peak heap
would be interesting, but our perf suite isn't yet capable of that.
- note that diff without "--quiet" does not suffer from the same
problem. In diffcore_skip_stat_unmatch(), we detect the stat-unmatch
entries and drop them immediately, so we're not carrying their data
around.
- you _can_ still trigger the mmap limit problem if you truly have
that many files with actual changes. But it's rather unlikely. The
stat-unmatch check avoids loading the file contents if the sizes
don't match, so you'd need a pretty trivial change in every single
file. Likewise, inexact rename detection might load the data for
many files all at once. But you'd need not just 64k changes, but
that many deletions and additions. The most likely candidate is
perhaps break-detection, which would load the data for all pairs and
keep it around for the content-level diff. But again, you'd need 64k
actually changed files in the first place.
So it's still possible to trigger this case, but it seems like "I
accidentally made all my files stat-dirty" is the most likely case
in the real world.
Reported-by: Jan Christoph Uhde <redacted>
Signed-off-by: Jeff King <redacted>
---
diff.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/diff.c b/diff.c
index d1ad6a3c4a..4d46fab5d3 100644
--- a/diff.c
+++ b/diff.c
@@ -6758,8 +6758,11 @@ void diff_change(struct diff_options *options,
return;
if (options->flags.quick && options->skip_stat_unmatch &&
- !diff_filespec_check_stat_unmatch(options->repo, p))
+ !diff_filespec_check_stat_unmatch(options->repo, p)) {
+ diff_free_filespec_data(p->one);
+ diff_free_filespec_data(p->two);
return;
+ }
options->flags.has_changes = 1;
}--
2.27.0.rc2.689.g04a94059a7
On Mon, Jun 1, 2020 at 4:22 PM Jeff King [off-list ref] wrote:
Subject: [PATCH] diff: discard blob data from stat-unmatched pairs
When performing a tree-level diff against the working tree, we may find
that our index stat information is dirty, so we queue a filepair to be
examined later. If the actual content hasn't changed, we call this a
stat-unmatch; the stat information was out of date, but there's no
actual diff. Normally diffcore_std() would detect and remove these
identical filepairs via diffcore_skip_stat_unmatch(). However, when
"--quiet" is used, we want to stop the diff as soon as we see any
changes, so we check for stat-unmatches immediately in diff_change().
That check may require us to actually load the file contents into the
pair of diff_filespecs. If we find that the pair isn't a stat-unmatch,
then no big deal; we'd likely load the contents later anyway to generate
a patch, do rename detection, etc, so we want to hold on to it. But if
it a stat-unmatch, then we have no more use for that data; the whole
s/it/& is/
point is that we're going discard the pair. However, we never free the
allocated diff_filespec data.
In most cases, keeping that data isn't a problem. We don't expect a lot
of stat-unmatch entries, and since we're using --quiet, we'd quit as
soon as we saw such a real change anyway. However, there are extreme
cases where it makes a big difference:
1. We'd generally mmap() the working tree half of the pair. And since
the OS may limit the total number of maps, we can run afoul of this
in large repositories. E.g.:
$ cd linux
$ git ls-files | wc -l
67959
$ sysctl vm.max_map_count
vm.max_map_count = 65530
$ git ls-files | xargs touch ;# everything is stat-dirty!
$ git diff --quiet
fatal: mmap failed: Cannot allocate memory
It should be unusual to have so many files stat-dirty, but it's
possible if you've just run a script like "sed -i" or similar.
After this patch, the above correctly exits with code 0.
2. Even if you don't hit mmap limits, the index half of the pair will
have been pulled from the object database into heap memory. Again
in a clone of linux.git, running:
$ git ls-files | head -n 10000 | xargs touch
$ git diff --quiet
peaks at 145MB heap before this patch, and 94MB after.
This patch solves the problem by freeing any diff_filespec data we
picked up during the "--quiet" stat-unmatch check in diff_changes.
Nobody is going to need that data later, so there's no point holding on
to it. There are a few things to note:
- we could skip queueing the pair entirely, which could in theory save
a little work. But there's not much to save, as we need a
diff_filepair to feed to diff_filespec_check_stat_unmatch() anyway.
And since we cache the result of the stat-unmatch checks, a later
call to diffcore_skip_stat_unmatch() call will quickly skip over
them. The diffcore code also counts up the number of stat-unmatched
pairs as it removes them. It's doubtful any callers would care about
that in combination with --quiet, but we'd have to reimplement the
logic here to be on the safe side. So it's not really worth the
trouble.
- I didn't write a test, because we always produce the correct output
unless we run up against system mmap limits, which are both
unportable and expensive to test against. Measuring peak heap
would be interesting, but our perf suite isn't yet capable of that.
- note that diff without "--quiet" does not suffer from the same
problem. In diffcore_skip_stat_unmatch(), we detect the stat-unmatch
entries and drop them immediately, so we're not carrying their data
around.
- you _can_ still trigger the mmap limit problem if you truly have
that many files with actual changes. But it's rather unlikely. The
stat-unmatch check avoids loading the file contents if the sizes
don't match, so you'd need a pretty trivial change in every single
file. Likewise, inexact rename detection might load the data for
many files all at once. But you'd need not just 64k changes, but
that many deletions and additions. The most likely candidate is
perhaps break-detection, which would load the data for all pairs and
keep it around for the content-level diff. But again, you'd need 64k
actually changed files in the first place.
So it's still possible to trigger this case, but it seems like "I
accidentally made all my files stat-dirty" is the most likely case
in the real world.
Reported-by: Jan Christoph Uhde <redacted>
Signed-off-by: Jeff King <redacted>