On Sun, Feb 10, 2013 at 4:47 PM, Duy Nguyen <pclo...@gmail.com> wrote:
> On Sun, Feb 10, 2013 at 12:24:58PM +0700, Duy Nguyen wrote:
>> On Sun, Feb 10, 2013 at 12:10 AM, Ramkumar Ramachandra
>> <artag...@gmail.com> wrote:
>> > Finn notes in the commit message that it offers no speedup, because
>> > .gitignore files in every directory still have to be read.  I think
>> > this is silly: we really should be caching .gitignore, and touching it
>> > only when lstat() reports that the file has changed.
>> >
>> > ...
>> >
>> > Really, the elephant in the room right now seems to be .gitignore.
>> > Until that is fixed, there is really no use of writing this inotify
>> > daemon, no?  Can someone enlighten me on how exactly .gitignore files
>> > are processed?
>>
>> .gitignore is a different issue. I think it's mainly used with
>> read_directory/fill_directory to collect ignored files (or not-ignored
>> files). And it's not always used (well, status and add does, but diff
>> should not). I think wee need to measure how much mass lstat
>> elimination gains us (especially on big repos) and how much
>> .gitignore/.gitattributes caching does.
>
> OK let's count. I start with a "standard" repository, linux-2.6. This
> is the number from strace -T on "git status" (*). The first column is
> accumulated time, the second the number of syscalls.
>
> top syscalls sorted     top syscalls sorted
> by acc. time            by number
> ----------------------------------------------
> 0.401906 40950 lstat    0.401906 40950 lstat
> 0.190484 5343 getdents  0.150055 5374 open
> 0.150055 5374 open      0.190484 5343 getdents
> 0.074843 2806 close     0.074843 2806 close
> 0.003216 157 read       0.003216 157 read
>
> The following patch pretends every entry is uptodate without
> lstat. With the patch, we can see refresh code is the cause of mass
> lstat, as lstat disappears:
>
> 0.185347 5343 getdents  0.144173 5374 open
> 0.144173 5374 open      0.185347 5343 getdents
> 0.071844 2806 close     0.071844 2806 close
> 0.004918 135 brk        0.003378 157 read
> 0.003378 157 read       0.004918 135 brk

Okay, we're saving 40k lstat() calls.

> -- 8< --
> diff --git a/read-cache.c b/read-cache.c
> index 827ae55..94d8ed8 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1018,6 +1018,10 @@ static struct cache_entry *refresh_cache_ent(struct 
> index_state *istate,
>         if (ce_uptodate(ce))
>                 return ce;
>
> +#if 1
> +       ce_mark_uptodate(ce);
> +       return ce;
> +#endif
>         /*
>          * CE_VALID or CE_SKIP_WORKTREE means the user promised us
>          * that the change to the work tree does not matter and told
> -- 8< --

So you're skipping the rest of refresh_cache_ent(), which contains our
lstat() and returning immediately.  Instead of marking paths with the
"assume unchanged" bit, as core.ignoreStat does, you're directly
attacking the function that refreshes the index and bypassing the
lstat() call.  How are they different?  read-cache.c:1030 checks
ce->flags & CE_VALID (which is set in read-cache.c:88 if
assume_unchanged) and bypasses the lstat() call anyway.  So why didn't
you just set core.ignoreStat for your test?

> The following patch eliminates untracked search code. As we can see,
> open+getdents also disappears with this patch:
>
> 0.462909 40950 lstat   0.462909 40950 lstat
> 0.003417 129 brk       0.003417 129 brk
> 0.000762 53 read       0.000762 53 read
> 0.000720 36 open       0.000720 36 open
> 0.000544 12 munmap     0.000454 33 close

Okay, 5k open and 5k getdents calls are gone, but what does this mean?

Pulling the patch from your next email to figure this out:
> -- 8< --
> diff --git a/dir.c b/dir.c
> index 57394e4..1963c6f 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1439,8 +1439,10 @@ int read_directory(struct dir_struct *dir, const char 
> *path, int len, const char
>                 return dir->nr;
>
>         simplify = create_simplify(pathspec);
> +#if 0
>         if (!len || treat_leading_path(dir, path, len, simplify))
>                 read_directory_recursive(dir, path, len, 0, simplify);
> +#endif
>         free_simplify(simplify);
>         qsort(dir->entries, dir->nr, sizeof(struct dir_entry *), cmp_name);
>         qsort(dir->ignored, dir->ignored_nr, sizeof(struct dir_entry *), 
> cmp_name);
> -- 8< --

Ah, read_directory(), from the .gitignore/ exclude angle.  Yes,
read_directory() seems to be the main culprit there, from my reading
of Documentation/technical/api-directory-listing.txt.

So, what did you do?  You short-circuited the function into never
executing read_directory_recursive(), so the opendir() and readdir()
are gone.  I'm confused about what this means: will new directories
fail to appear as "untracked" now?  Either way, I understand that
you've factored out the .gitignore/ excludes.  Let's look at the
timings now.

> So from syscalls point of view, we know what code issues most of
> them. Let's see how much time we gain be these patches, which is an
> approximate of the gain by inotify support. This time I measure on
> gentoo-x86.git [1] because this one has really big worktree (100k
> files)
>
>         unmodified  read-cache.c  dir.c     both
> real    0m0.550s    0m0.479s      0m0.287s  0m0.213s
> user    0m0.305s    0m0.315s      0m0.201s  0m0.182s
> sys     0m0.240s    0m0.157s      0m0.084s  0m0.030s

So, the .gitignore/ exclude does seem to be the elephant in the room,
after all!  There are only minor gains from not updating the index.

> and the syscall picture on gentoo-x86.git:
>
> 1.106615 101942 lstat    1.106615 101942 lstat
> 0.667235 47083 getdents  0.641604 47114 open
> 0.641604 47114 open      0.667235 47083 getdents
> 0.286711 23573 close     0.286711 23573 close
> 0.005842 350 brk         0.005842 350 brk

The lstat to getdents/ open is higher than in linux-2.6.git here, but
the profit in eliminating lstat is still very small.

> We can see that shortcuting untracked code gives bigger gain than
> index refresh code. So I have to agree that .gitignore may be the big
> elephant in this particular case.

Yes :)

> Bear in mind though this is Linux, where lstat is fast. On systems
> with slow lstat, these timings could look very different due to the
> large number of lstat calls compared to open+getdents. I really like
> to see similar numbers on Windows.

I see.

> read_directory/fill_directory code is mostly used by "git add" (not
> with -u) and "git status", while refresh code is executed in add,
> checkout, commit/status, diff, merge. So while smaller gain, reducing
> lstat calls could benefit in more cases.

Good point, although my major complaint with big repositories is
status and diff.  Checkout isn't too bad if the branches don't diverge
much, add is fast enough, but diff is a big problem.

> A relatively slow "git add" is acceptable. "git status" should be
> fast. Although in my workflow, I do "git diff [--stat] [--cached]"
> much more often than "git status" so relatively slow "git status" does
> not hurt me much. But people may do it differently.

Hm.

> On speeding up read_directory with inotify support. I haven't thought
> it through, but I think we could save (or get it via socket) a list of
> untracked files in .git, regardless ignore status, with the help from
> inotify. When this list is verified valid, read_directory could be
> modified to traverse the tree using this list (plus the index) instead
> of opendir+readdir. Not sure how the change might look though.

We could even tell if .gitignore has changed with inotify support, and
tell exactly when we need to update our path treatment.  And yes, we
can have inotify to tell us about new files and directories directly,
so we can traverse them.  I think we should go ahead with the
system-wide inotify daemon for now: its design needs to be discussed,
so that it's generic enough for all our usecases.  I'm thinking there
should be atleast two distinct calls to:
1. Report all changed paths, for use with read-cache.c.
2. Report only new paths, for use with dir.c.
Or can we figure out which of the changed paths are new ourselves?

Kudos to your great work on getting getting these numbers, Duy!
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to