Re: [PATCH/RFC v3 7/9] write_entry(): use fstat() instead of lstat() when file is open

Re: [PATCH/RFC v3 7/9] write_entry(): use fstat() instead of lstat() when file is open

[Kjetil Barvik]

* Kjetil Barvik schrieb:
| And, yes, since each lstat() call cost approximately 44 microseconds
| compared to 12-16 for each lstat() on my Linux box, there was a little
                             ^^^^^^^ fstat()?

  Yes, is is the fstat() call which takes 12-16 microseconds on my Linux
  box.

| performance gain from this patch.

* Johannes Sixt <j.sixt@viscovery.net>
| This does look like a good gain. But do you have hard numbers that back
| the claim?
|
| If you can squeeze out a 10% improvement on Linux with this patch, we
| should take it, and if it turns out that it doesn't work on Windows, we
| could trivially throw in an #ifdef MINGW (or even #ifdef WIN32 if Cygwin
| is affected, too) that skips the fstat() optimization.
| 
| But we really should have numbers that justify this effort.

  I have been working on a long running test script latly, but then I
  started to play with the 'git repack' command, so it was not top
  priority.  But, I can finish the script today, and run it while I am
  sleeping tonight.

| BTW, the statement from the Windows documentation was:
|
|  "The only guarantee about a file timestamp is that the file time is
|   correctly reflected when the handle that makes the change is closed."
|
| In the case of this patch, the timestamp is queried via the handle
| that made the change, and in this case special case the timestamp
| could be correct nevertheless. The guarantee doesn't cover this case,
| but it would be natural, and perhaps it Just Works?

  Yes it could perhaps "just works".  But, then I guess that when it
  does not work, the user would not notice it _except_ for more time
  used.  Since I can to this:

kjetil@localhost ~/git/git $ git status 
# On branch lstat_fstat_v6
nothing to commit (working directory clean)
kjetil@localhost ~/git/git $ touch var.c
kjetil@localhost ~/git/git $ git status 
# On branch lstat_fstat_v6
nothing to commit (working directory clean)
kjetil@localhost ~/git/git $ 

  ... I think that git will have to check the content and read each byte
  and do a SHA1 of the file var.c in this case (since the timestamps do
  not match), which is a more time and CPU hungry opperation, to decide
  if there is a difference inside the file or not.

  And, maybe some other platform also have problems with this trick?

  OK, I do the time tests and let the numbers talk.

  -- kjetil

[PATCH/RFC v3 0/9] git checkout: more cleanups, optimisation, less lstat() calls

[Kjetil Barvik]

Changes since v2

--- patch */9 ---
- Changed the order of some patches.
- Some updates to the commit log messages.

--- patch 2/9 ---
- New patch which let us later use longest_match_lstat_cache(), now
  renamed to longest_path_match(), inside patch 4/9.

--- patch 3/9 ---
- New patch which update the symlinks.c file to be more in line with
  the GIT source code (s/length,string/string,length/ for function
  arguments).

-- patch 4/9 ---
- The new function schedule_dir_for_removal() is placed inside
  symlinks.c instead of unpack-trees.c

-- patch 9/9 ---
- NOTE/NB: this patch is only a debug patch, not be included in the final
  GIT release version.



Kjetil Barvik (9):
  lstat_cache(): small cleanup and optimisation
  lstat_cache(): generalise longest_match_lstat_cache()
  lstat_cache(): swap func(length, string) into func(string, length)
  unlink_entry(): introduce schedule_dir_for_removal()
  create_directories(): remove some memcpy() and strchr() calls
  write_entry(): cleanup of some duplicated code
  write_entry(): use fstat() instead of lstat() when file is open
  show_patch_diff(): remove a call to fstat()
  lstat_cache(): print a warning if doing ping-pong between cache types

 Documentation/CodingGuidelines |    3 +
 builtin-add.c                  |    2 +-
 builtin-apply.c                |    2 +-
 builtin-update-index.c         |    2 +-
 cache.h                        |   10 ++-
 combine-diff.c                 |    4 +-
 diff-lib.c                     |    2 +-
 dir.c                          |    2 +-
 entry.c                        |  108 ++++++++++++-------------
 symlinks.c                     |  177 ++++++++++++++++++++++++++++++----------
 unpack-trees.c                 |   34 ++------
 11 files changed, 208 insertions(+), 138 deletions(-)

[PATCH/RFC v3 7/9] write_entry(): use fstat() instead of lstat() when file is open

[Kjetil Barvik]

Currently inside write_entry() we do an lstat(path, &st) call on a
file which have just been opened inside the exact same function.  It
should be better to call fstat(fd, &st) on the file while it is open,
and it should be at least as fast as the lstat() method.

Signed-off-by: Kjetil Barvik <barvik@broadpark.no>
---
 entry.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/entry.c b/entry.c
index 1f53588..5daacc2 100644
--- a/entry.c
+++ b/entry.c
@@ -94,11 +94,12 @@ static void *read_blob_entry(struct cache_entry *ce, unsigned long *size)
 static int write_entry(struct cache_entry *ce, char *path, const struct checkout *state, int to_tempfile)
 {
 	unsigned int ce_mode_s_ifmt = ce->ce_mode & S_IFMT;
-	int fd, ret;
+	int fd, ret, fstat_done = 0;
 	char *new;
 	struct strbuf buf = STRBUF_INIT;
 	unsigned long size;
 	size_t wrote, newsize = 0;
+	struct stat st;
 
 	switch (ce_mode_s_ifmt) {
 	case S_IFREG:
@@ -145,6 +146,11 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout
 		}
 
 		wrote = write_in_full(fd, new, size);
+		/* use fstat() only when path == ce->name */
+		if (state->refresh_cache && !to_tempfile && !state->base_dir_len) {
+			fstat(fd, &st);
+			fstat_done = 1;
+		}
 		close(fd);
 		free(new);
 		if (wrote != size)
@@ -161,8 +167,8 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout
 	}
 
 	if (state->refresh_cache) {
-		struct stat st;
-		lstat(ce->name, &st);
+		if (!fstat_done)
+			lstat(ce->name, &st);
 		fill_stat_cache_info(ce, &st);
 	}
 	return 0;
-- 
1.6.1.349.g99fa5

Re: [PATCH/RFC v3 7/9] write_entry(): use fstat() instead of lstat() when file is open

[Johannes Sixt]

Kjetil Barvik schrieb:
> Currently inside write_entry() we do an lstat(path, &st) call on a
> file which have just been opened inside the exact same function.  It
> should be better to call fstat(fd, &st) on the file while it is open,
> and it should be at least as fast as the lstat() method.
...
> @@ -145,6 +146,11 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout
>  		}
>  
>  		wrote = write_in_full(fd, new, size);
> +		/* use fstat() only when path == ce->name */
> +		if (state->refresh_cache && !to_tempfile && !state->base_dir_len) {
> +			fstat(fd, &st);
> +			fstat_done = 1;
> +		}
>  		close(fd);

I've a bad gut feeling about this: It may not work as expected on Windows
because there is this statement in the documentation:

  "The only guarantee about a file timestamp is that the file time is
   correctly reflected when the handle that makes the change is closed."

(http://msdn.microsoft.com/en-us/library/ms724290(VS.85).aspx)

We are operating on a temporary file. It could happen that the fstat()
returns the time when the file was created, as opposed to when the
write_in_full() was completed successfully. The fstat()ed time ends up in
the index, but it can be different from what later lstat() calls report
(and the file would be regarded as modified).

I have the suspicion that the gain from this patch is minimal. Would you
mind playing it safe and drop this patch?

-- Hannes

Re: [PATCH/RFC v3 7/9] write_entry(): use fstat() instead of lstat() when file is open

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Kjetil Barvik schrieb:
>> Currently inside write_entry() we do an lstat(path, &st) call on a
>> file which have just been opened inside the exact same function.  It
>> should be better to call fstat(fd, &st) on the file while it is open,
>> and it should be at least as fast as the lstat() method.
> ...
>> @@ -145,6 +146,11 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout
>>  		}
>>  
>>  		wrote = write_in_full(fd, new, size);
>> +		/* use fstat() only when path == ce->name */
>> +		if (state->refresh_cache && !to_tempfile && !state->base_dir_len) {
>> +			fstat(fd, &st);
>> +			fstat_done = 1;
>> +		}
>>  		close(fd);
>
> I've a bad gut feeling about this: It may not work as expected on Windows
> because there is this statement in the documentation:
>
>   "The only guarantee about a file timestamp is that the file time is
>    correctly reflected when the handle that makes the change is closed."
>
> (http://msdn.microsoft.com/en-us/library/ms724290(VS.85).aspx)
>
> We are operating on a temporary file. It could happen that the fstat()
> returns the time when the file was created, as opposed to when the
> write_in_full() was completed successfully. The fstat()ed time ends up in
> the index, but it can be different from what later lstat() calls report
> (and the file would be regarded as modified).
>
> I have the suspicion that the gain from this patch is minimal. Would you
> mind playing it safe and drop this patch?

Hmm, write_entry() is actually called once per one path we write out, and
the fstat() is added to the common case (no --tempfile, no --prefix=<dir>
checkout), which would mean that if there were any performance gain from
this change, it was obtained by trading correctness away.  Sad.

Re: [PATCH/RFC v3 7/9] write_entry(): use fstat() instead of lstat() when file is open

[Kjetil Barvik]

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Sixt <j.sixt@viscovery.net> writes:
<snip>
>> I've a bad gut feeling about this: It may not work as expected on Windows
>> because there is this statement in the documentation:
>>
>>   "The only guarantee about a file timestamp is that the file time is
>>    correctly reflected when the handle that makes the change is closed."
>>
>> (http://msdn.microsoft.com/en-us/library/ms724290(VS.85).aspx)

  Ok, I admit I was not aware of this Windows fact.

>> We are operating on a temporary file. It could happen that the fstat()
>> returns the time when the file was created, as opposed to when the
>> write_in_full() was completed successfully. The fstat()ed time ends up in
>> the index, but it can be different from what later lstat() calls report
>> (and the file would be regarded as modified).
>>
>> I have the suspicion that the gain from this patch is minimal. Would you
>> mind playing it safe and drop this patch?
>
> Hmm, write_entry() is actually called once per one path we write out, and
> the fstat() is added to the common case (no --tempfile, no --prefix=<dir>
> checkout), 

  Yes, I had to make sure that the path string and ce->name was the
  exact same string, so therefore I had to add the test "&& !to_tempfile
  && !state->base_dir_len" to the if-test.

> which would mean that if there were any performance gain from
> this change, it was obtained by trading correctness away.  Sad.

  Sorry about this.  Yes, I agree that we should drop this patch.

  And, yes, since each lstat() call cost approximately 44 microseconds
  compared to 12-16 for each lstat() on my Linux box, there was a little
  performance gain from this patch.

  Junio, is it OK to ask that you drop this patch if/when you update the
  pu branch, such that I do not have to resend the patch series almost
  unchanged to the mailinglist (except for one missing patch)?

  Ok, maybe wait one day, just in case there will be more comments.

  And, thanks for the review!

  -- kjetil

Re: [PATCH/RFC v3 7/9] write_entry(): use fstat() instead of lstat() when file is open

[Junio C Hamano]

Kjetil Barvik <barvik@broadpark.no> writes:

>   Junio, is it OK to ask that you drop this patch if/when you update the
>   pu branch, such that I do not have to resend the patch series almost
>   unchanged to the mailinglist (except for one missing patch)?

Surely.  Although I've replaced the topic with the updated series, I am
not in the reintegration phase yet, so not much work is lost.  I was
planning to merge this to 'next' today after one last round of eyeballing.

Thanks.

Re: [PATCH/RFC v3 7/9] write_entry(): use fstat() instead of lstat() when file is open

[Johannes Sixt]

Kjetil Barvik schrieb:
>   And, yes, since each lstat() call cost approximately 44 microseconds
>   compared to 12-16 for each lstat() on my Linux box, there was a little
                               ^^^^^^^ fstat()?
>   performance gain from this patch.

This does look like a good gain. But do you have hard numbers that back
the claim?

If you can squeeze out a 10% improvement on Linux with this patch, we
should take it, and if it turns out that it doesn't work on Windows, we
could trivially throw in an #ifdef MINGW (or even #ifdef WIN32 if Cygwin
is affected, too) that skips the fstat() optimization.

But we really should have numbers that justify this effort.

BTW, the statement from the Windows documentation was:

  "The only guarantee about a file timestamp is that the file time is
   correctly reflected when the handle that makes the change is closed."

In the case of this patch, the timestamp is queried via the handle that
made the change, and in this case special case the timestamp could be
correct nevertheless. The guarantee doesn't cover this case, but it would
be natural, and perhaps it Just Works?

-- Hannes

Re: [PATCH/RFC v3 7/9] write_entry(): use fstat() instead of lstat() when file is open

[Johannes Schindelin]

Hi,

On Thu, 5 Feb 2009, Johannes Sixt wrote:

> Kjetil Barvik schrieb:
> >   And, yes, since each lstat() call cost approximately 44 microseconds
> >   compared to 12-16 for each lstat() on my Linux box, there was a little
>                                ^^^^^^^ fstat()?
> >   performance gain from this patch.
> 
> This does look like a good gain. But do you have hard numbers that back
> the claim?
> 
> If you can squeeze out a 10% improvement on Linux with this patch, we
> should take it, and if it turns out that it doesn't work on Windows, we
> could trivially throw in an #ifdef MINGW (or even #ifdef WIN32 if Cygwin
> is affected, too) that skips the fstat() optimization.

Of course, what we _really_ would do is to provide a flag, say, 
FSTAT_UNRELIABLE and test for _that_ (after defining it in the Makefile 
for the appropriate platforms).

Ciao,
Dscho

Re: [PATCH/RFC v3 7/9] write_entry(): use fstat() instead of lstat() when file is open

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Of course, what we _really_ would do is to provide a flag, say, 
> FSTAT_UNRELIABLE and test for _that_ (after defining it in the Makefile 
> for the appropriate platforms).

Sounds sensible.  POSIX of course requires write(), pwrite() and friends
to mark the mtime for update and fstat() to be reliable, but there may be
"not quite POSIX" systems.

Re: [PATCH/RFC v3 7/9] write_entry(): use fstat() instead of lstat() when file is open

[Kjetil Barvik]

* On Thu, 5 Feb 2009, Johannes Sixt wrote:
|
|> Kjetil Barvik schrieb:
|> >   And, yes, since each lstat() call cost approximately 44 microseconds
|> >   compared to 12-16 for each lstat() on my Linux box, there was a little
|>                                ^^^^^^^ fstat()?
|> >   performance gain from this patch.
|> 
|> This does look like a good gain. But do you have hard numbers that back
|> the claim?

  __________________
  Test description

  I have used the following 8 git binaries:

$ for g in ./git_t*; do printf "$g => $($g --version)\n"; done
./git_t0 => git version 1.6.1.80.g7eb5
./git_t1 => git version 1.6.1.85.gbda6       <= added lstat_cache()
./git_t2 => git version 1.6.1.2.306.gc0f6f
./git_t3 => git version 1.6.1.2.307.g55385
./git_t4 => git version 1.6.1.2.308.g052a
./git_t5 => git version 1.6.1.2.310.g40dd2   <= added schedule_dir_for_removal()
./git_t6 => git version 1.6.1.2.313.g9deee
./git_t7 => git version 1.6.1.2.314.g0ad9    <= added this patch (7/9)

  Except for git_t7 all commits should be in origin/pu, so people should
  be able to do git show/diff/log on the last hex chars on the version-
  strings to see the differences.

  CFLAGS="-mtune=core2 -O2 -fomit-frame-pointer -fno-stack-protector -g0 -s"

  Each git_t* binary is run with args "checkout -q my-v.2.6.2[57]" for a
  total of 100 times (50/50 to my-v2.6.25/my-v2.6.27).  Before each run
  the test script sleeps 20 seconds to allow the disk to finish and
  being idle.  Time is collected by /usr/bin/time -o output-file prog.

  While the test script was run, the laptop with an Intel Core2 Duo 2
  GHz processor, was run without X and was otherwise idle.  The test
  script took 9 hours and 45 minutes to complete.

  __________________
  Test numbers

$ for ((t=0; $t<=7; t++)); do echo "git_t$t => $(parse_usr_bin_time_lines.pl git_t$t*)"; done
git_t0 =>  5.646 user  8.322 sys  25.579 real  54.6% CPU  faults:  0 major 39587 minor
git_t1 =>  5.631 user  6.826 sys  23.941 real  52.1% CPU  faults:  0 major 39901 minor
git_t2 =>  5.622 user  6.854 sys  24.036 real  52.1% CPU  faults:  0 major 39298 minor
git_t3 =>  5.636 user  6.867 sys  24.088 real  52.0% CPU  faults:  0 major 39786 minor
git_t4 =>  5.640 user  6.818 sys  24.006 real  52.0% CPU  faults:  0 major 39629 minor
git_t5 =>  5.642 user  6.552 sys  23.805 real  51.4% CPU  faults:  0 major 39707 minor
git_t6 =>  5.629 user  6.518 sys  22.981 real  53.0% CPU  faults:  0 major 40029 minor
git_t7 =>  5.629 user  6.051 sys  23.013 real  50.9% CPU  faults:  0 major 39451 minor

|> If you can squeeze out a 10% improvement on Linux with this patch, we
|> should take it, and if it turns out that it doesn't work on Windows, we
|> could trivially throw in an #ifdef MINGW (or even #ifdef WIN32 if Cygwin
|> is affected, too) that skips the fstat() optimization.

  For the system used time, the improvement was (- 6.518 6.051) = 0.467
  seconds, or (/ (* (- 6.518 6.051) 100.0) 6.518) = 7.2%, so not 10%.

  Funny to see that in:
     http://article.gmane.org/gmane.comp.version-control.git/107281

  I guessed the improvement to be (quote):
     "(* 14403 (- 44 12)) = 460 896 microseconds system time"

  So I missed only by a little over 6ms. :-)

* Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
| Of course, what we _really_ would do is to provide a flag, say, 
| FSTAT_UNRELIABLE and test for _that_ (after defining it in the Makefile 
| for the appropriate platforms).

  Or, maybe

     #define FSTAT_RELIABLE 1

  for Linux only?  Then we can change the if-test inside this patch to
  the following:

-  if (state->refresh_cache && !to_tempfile && !state->base_dir_len) {
+  if (state->refresh_cache && !to_tempfile && !state->base_dir_len && 
+      FSTAT_RELIABLE) {

  Then we do not have to have #if-defines inside the source code, only
  in one header file.

  But, question: is this patch worth the extra lines added to the source
  code?

  -- kjetil

  PS!  Junio, I think this patch series should be inside pu some days
       more, since things obviously needs more refinement and tests.

Re: [PATCH/RFC v3 7/9] write_entry(): use fstat() instead of lstat() when file is open

[Johannes Schindelin]

Hi,

On Fri, 6 Feb 2009, Kjetil Barvik wrote:

> * Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> | Of course, what we _really_ would do is to provide a flag, say, 
> | FSTAT_UNRELIABLE and test for _that_ (after defining it in the Makefile 
> | for the appropriate platforms).
> 
>   Or, maybe
> 
>      #define FSTAT_RELIABLE 1
> 
>   for Linux only?

No, I think that would be wrong.  Especially after Junio's remarks that 
fstat() is actually required to behave as you expected it, and only 
Windows (surprise, surprise) has problems following the standard.

> Then we can change the if-test inside this patch to the following:
> 
> -  if (state->refresh_cache && !to_tempfile && !state->base_dir_len) {
> +  if (state->refresh_cache && !to_tempfile && !state->base_dir_len && 
> +      FSTAT_RELIABLE) {
> 
>   Then we do not have to have #if-defines inside the source code, only
>   in one header file.

In the spirit of consistency, I would not do that.

>   But, question: is this patch worth the extra lines added to the source
>   code?

You seemed to get a nice speedup on Linux.  If Windows cannot follow suit, 
too bad.  But I do not want to be punished because other people's OS is 
not as good as mine, so I _want_ fstat().

And those few lines will not hurt, they'll be readable enough.

Ciao,
Dscho

Re: [PATCH/RFC v3 7/9] write_entry(): use fstat() instead of lstat() when file is open

[Kjetil Barvik]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Kjetil Barvik schrieb:
>>   And, yes, since each lstat() call cost approximately 44 microseconds
>>   compared to 12-16 for each lstat() on my Linux box, there was a little
>                                ^^^^^^^ fstat()?
>>   performance gain from this patch.
>
> This does look like a good gain. But do you have hard numbers that back
> the claim?

  OK, I have done some testing/profiling with oprofile(1), and one thing
  I found out was that my Linux kernel was built with SLUB debug, and of
  course it cost some system time to run the VM debug code.  After I
  turned this off, the total system time when down from aprox 6 to 3
  seconds for the 'git checkout -q my-v2.6.25/7' test.

  Also, from strace output each lstat() call now take around 16
  microseconds, and each fstat() call around 12 microseconds, so for
  aprox 14000 changed calls (lstat() => fstat()) the performance gain
  should now only be (* 14000 (- 16 12)) = 56 ms, compared to 467 ms,
  which I reported before.

  -- kjetil

  1) http://oprofile.sourceforge.net/about/