threads / patch / 22220

patch, 2 partsrm: only refresh entries that we may touch

Subject: [PATCH 1/2] rm: only refresh entries that we may touch

## tl;dr

10 messages between Jan 14, 2010 and Jan 17, 2010. Diffs are folded; open one to read it.

replies: 9people: 4as markdown or json

Nguyễn Thái Ngọc Duy· Jan 14, 2010, 15:02 UTC · lore

[PATCH 0/2] Improve Git performance on big trees

The discussion about using Git on Gentoo comes up again [1] and it looks like we can improve Git a bit if user only works on some subdirectories. In such cases, Git still does whole-tree check, which takes quite some time. "git status" takes 3 seconds on my machine, but I suspect lstat() is not the only culprit, "git diff --exit-code" is about 1 sec.

These patches makes "git rm <path>" and "git status <path>" a bit faster. Almost 1 sec for "git rm foo" still seems too long though, probably due to writing a 9MB index.

[1] http://article.gmane.org/gmane.linux.gentoo.devel/64522
Nguyễn Thái Ngọc Duy (2):
  rm: only refresh entries that we may touch
  status: only touch path we may need to check
 builtin-commit.c |    2 +-
 builtin-rm.c     |    2 +-
 wt-status.c      |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)
Nguyễn Thái Ngọc Duy· Jan 14, 2010, 15:02 UTC · re: Nguyễn Thái Ngọc Duy · lore

This gets rid of the whole tree cache refresh. Instead only path that we touch will get refreshed. We may still lstat() more than needed, but it'd be better playing safe.

This potentially reduces a large number of lstat() on big trees. Take gentoo-x86 tree for example, which has roughly 80k files:

Unmodified Git:

$ time git rm --cached skel.ChangeLog rm 'skel.ChangeLog'

real 0m1.323s user 0m0.806s sys 0m0.517s

Modified Git:

$ time git rm --cached skel.ChangeLog rm 'skel.ChangeLog'

real 0m0.903s user 0m0.780s sys 0m0.100s

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin-rm.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to builtin-rm.c +1 −1
diff --git a/builtin-rm.c b/builtin-rm.c
index 57975db..4cac3d1 100644
--- a/builtin-rm.c
+++ b/builtin-rm.c
@@ -169,7 +169,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 
 	if (read_cache() < 0)
 		die("index file corrupt");
-	refresh_cache(REFRESH_QUIET);
 
 	pathspec = get_pathspec(prefix, argv);
 	seen = NULL;
@@ -181,6 +180,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 		struct cache_entry *ce = active_cache[i];
 		if (!match_pathspec(pathspec, ce->name, ce_namelen(ce), 0, seen))
 			continue;
+		refresh_cache_entry(ce, 1);
 		add_list(ce->name);
 	}
 

-- 
1.6.6.181.g5ee6
Junio C Hamano· Jan 16, 2010, 04:39 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 1/2] rm: only refresh entries that we may touch

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 17 quoted lines
> diff --git a/builtin-rm.c b/builtin-rm.c
> index 57975db..4cac3d1 100644
> --- a/builtin-rm.c
> +++ b/builtin-rm.c
> @@ -169,7 +169,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
>  
>  	if (read_cache() < 0)
>  		die("index file corrupt");
> -	refresh_cache(REFRESH_QUIET);
>  
>  	pathspec = get_pathspec(prefix, argv);
>  	seen = NULL;
> @@ -181,6 +180,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
>  		struct cache_entry *ce = active_cache[i];
>  		if (!match_pathspec(pathspec, ce->name, ce_namelen(ce), 0, seen))
>  			continue;
> +		refresh_cache_entry(ce, 1);

Why does this pass "1" instead of "0"? The existing code does not give refresh_cache() REFRESH_REALLY bit, and a patch that is marked as a pure optimization should pass 0. If you really mean it, please spell it as CE_MATCH_IGNORE_VALID and justify why it is a good change in a separate patch.

Thanks.
Nguyen Thai Ngoc Duy· Jan 16, 2010, 10:58 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] rm: only refresh entries that we may touch

On 1/16/10, Junio C Hamano <gitster@pobox.com> wrote:
Show 26 quoted lines
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>  > diff --git a/builtin-rm.c b/builtin-rm.c
>  > index 57975db..4cac3d1 100644
>  > --- a/builtin-rm.c
>  > +++ b/builtin-rm.c
>  > @@ -169,7 +169,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
>  >
>  >       if (read_cache() < 0)
>  >               die("index file corrupt");
>  > -     refresh_cache(REFRESH_QUIET);
>  >
>  >       pathspec = get_pathspec(prefix, argv);
>  >       seen = NULL;
>  > @@ -181,6 +180,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
>  >               struct cache_entry *ce = active_cache[i];
>  >               if (!match_pathspec(pathspec, ce->name, ce_namelen(ce), 0, seen))
>  >                       continue;
>  > +             refresh_cache_entry(ce, 1);
>
>
> Why does this pass "1" instead of "0"?  The existing code does not give
>  refresh_cache() REFRESH_REALLY bit, and a patch that is marked as a pure
>  optimization should pass 0.  If you really mean it, please spell it as
>  CE_MATCH_IGNORE_VALID and justify why it is a good change in a separate
>  patch.

"refresh_cache(REFRESH_QUIET);" does pass "0" indeed. Can you please change it to "0"? Thanks.

-- 
Duy
Nguyễn Thái Ngọc Duy· Jan 14, 2010, 15:02 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 2/2] status: only touch path we may need to check

This patch gets rid of whole-tree cache refresh and untracked file search. Instead only specified path will be looked at.

Again some numbers on gentoo-x86, ~80k files:
Unmodified Git:

$ time git st eclass/ nothing to commit (working directory clean)

real 0m3.211s user 0m1.977s sys 0m1.135s

Modified Git:

$ time ~/w/git/git st eclass/ nothing to commit (working directory clean)

real 0m1.587s user 0m1.426s sys 0m0.111s

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin-commit.c |    2 +-
 wt-status.c      |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
Show changes to 2 files +2 −2

builtin-commit.c, wt-status.c

diff --git a/builtin-commit.c b/builtin-commit.c
index 6199db7..dae6cb3 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -1024,7 +1024,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 		s.pathspec = get_pathspec(prefix, argv);
 
 	read_cache();
-	refresh_cache(REFRESH_QUIET|REFRESH_UNMERGED);
+	refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, s.pathspec, NULL, NULL);
 	s.is_initial = get_sha1(s.reference, sha1) ? 1 : 0;
 	s.in_merge = in_merge;
 	wt_status_collect(&s);
diff --git a/wt-status.c b/wt-status.c
index 5d56988..65feb29 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -343,7 +343,7 @@ static void wt_status_collect_untracked(struct wt_status *s)
 			DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;
 	setup_standard_excludes(&dir);
 
-	fill_directory(&dir, NULL);
+	fill_directory(&dir, s->pathspec);
 	for (i = 0; i < dir.nr; i++) {
 		struct dir_entry *ent = dir.entries[i];
 		if (!cache_name_is_other(ent->name, ent->len))

-- 
1.6.6.181.g5ee6
Sverre Rabbelier· Jan 14, 2010, 16:02 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 2/2] status: only touch path we may need to check

Heya,
2010/1/14 Nguyễn Thái Ngọc Duy <pclouds@gmail.com>:
> $ time git st eclass/
> real    0m3.211s
> $ time ~/w/git/git st eclass/
> real    0m1.587s
Wow, nice reduction!
-- 
Cheers,

Sverre Rabbelier
Martin Langhoff· Jan 14, 2010, 15:21 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 0/2] Improve Git performance on big trees

2010/1/14 Nguyễn Thái Ngọc Duy <pclouds@gmail.com>:
> Almost 1 sec for "git rm foo" still seems too long though,
> probably due to writing a 9MB index.

One of the main issues there is that the Gentoo dir tree seems to be very flat. The kernel tree is huge, but much deeper, and does not have a huge top-level directory -- and git handles it fairly well.

Perhaps the Gentoo tree can be rearranged to be more nested? If your devs strongly prefer a flat view of it, that could be arranged with symlinks.

Alternatively, each port can be in its own repo, and you can make a "top level repo" using git submodules -- this is what Fedora/RH is exploring at the moment.

cheers,
m
-- 
 martin.langhoff@gmail.com
 martin@laptop.org -- School Server Architect
 - ask interesting questions
 - don't get distracted with shiny stuff  - working code first
 - http://wiki.laptop.org/go/User:Martinlanghoff
Nguyen Thai Ngoc Duy· Jan 15, 2010, 13:36 UTC · re: Martin Langhoff · lore

Re: [PATCH 0/2] Improve Git performance on big trees

On 1/14/10, Martin Langhoff <martin.langhoff@gmail.com> wrote:
Show 9 quoted lines
> 2010/1/14 Nguyễn Thái Ngọc Duy <pclouds@gmail.com>:
>
> > Almost 1 sec for "git rm foo" still seems too long though,
>  > probably due to writing a 9MB index.
>
>
> One of the main issues there is that the Gentoo dir tree seems to be
>  very flat. The kernel tree is huge, but much deeper, and does not have
>  a huge top-level directory -- and git handles it fairly well.

Kernel tree is about one third the size of gentoo-x86 in terms of worktree. It takes ~1 sec to do "git status" on kernel tree and 3 secs on gentoo-x86, quite proportional. Except that 3 secs feel much longer than 1 :-)

Directory structure may also affect performance though. Kernel tree only has ~2k directories while gentoo-x86 has ~20k shallow dirs.

Show 7 quoted lines
>  Perhaps the Gentoo tree can be rearranged to be more nested? If your
>  devs strongly prefer a flat view of it, that could be arranged with
>  symlinks.
>
>  Alternatively, each port can be in its own repo, and you can make a
>  "top level repo" using git submodules -- this is what Fedora/RH is
>  exploring at the moment.

There were discussions of these in the past. And to my experience, these ideas won't go anywhere. The number of worktree files may drop significantly after Git migration because I believe all ChangeLog (~13k files on total ~80k) will be gone.

-- 
Duy
Nguyễn Thái Ngọc Duy· Jan 17, 2010, 08:43 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH] rm: only refresh entries that we may touch

This gets rid of the whole tree cache refresh. Instead only path that we touch will get refreshed. We may still lstat() more than needed, but it'd be better playing safe.

This potentially reduces a large number of lstat() on big trees. Take gentoo-x86 tree for example, which has roughly 80k files:

Unmodified Git:

$ time git rm --cached skel.ebuild rm 'skel.ebuild'

real 0m1.441s user 0m0.821s sys 0m0.531s

Modified Git:

$ time ~/w/git/git rm --cached skel.ebuild rm 'skel.ebuild'

real 0m0.941s user 0m0.828s sys 0m0.091s

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 In the previous attempt, refresh_cache_entry() returns a new cache_entry.
 It does not modify the_index, so tests failed.
 builtin-rm.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
Show changes to builtin-rm.c +2 −1
diff --git a/builtin-rm.c b/builtin-rm.c
index 57975db..f3772c8 100644
--- a/builtin-rm.c
+++ b/builtin-rm.c
@@ -169,9 +169,10 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 
 	if (read_cache() < 0)
 		die("index file corrupt");
-	refresh_cache(REFRESH_QUIET);
 
 	pathspec = get_pathspec(prefix, argv);
+	refresh_index(&the_index, REFRESH_QUIET, pathspec, NULL, NULL);
+
 	seen = NULL;
 	for (i = 0; pathspec[i] ; i++)
 		/* nothing */;

-- 
1.6.6.181.g5ee6

← back to recent threads