[PATCH] Use access instead of stat in hashdir

[PATCH] Use access instead of stat in hashdir

[Raghavendra D Prabhu]

[-- Attachment #1: Type: text/plain, Size: 839 bytes --]

Hi,

I found that using access instead of two stat calls results in 
faster rehash when it is done. I came across this when I noticed 
too many stat calls while 'strace -c' 

diff --git a/Src/hashtable.c b/Src/hashtable.c
index 775b6a2..43d2e2e 100644
--- a/Src/hashtable.c
+++ b/Src/hashtable.c
@@ -664,8 +664,7 @@ hashdir(char **dirp)
             * executable plain files.
             */
             if (unset(HASHEXECUTABLESONLY) ||
-                   (stat(pathbuf, &statbuf) == 0 &&
-                    S_ISREG(statbuf.st_mode) && (statbuf.st_mode & S_IXUGO)))
+                   !access(pathbuf,X_OK))
                 add = 1;
         }
         if (add) {





Regards,
-- 
Raghavendra Prabhu
GPG Id : 0xD72BE977
Fingerprint: B93F EBCB 8E05 7039 CD3C A4B8 A616 DCA1 D72B E977
www: wnohang.net

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH] Use access instead of stat in hashdir

[Bart Schaefer]

Thanks for the suggestion, but ...

On Feb 6,  6:43pm, Raghavendra D Prabhu wrote:
} 
} I found that using access instead of two stat calls

There's only one stat() call in the lines that you changed ...?

} results in 
} faster rehash when it is done. I came across this when I noticed 
} too many stat calls while 'strace -c' 

It also results in treating non-regular files as candidates for being
in the hash table, unless there's something about access() that is
implicitly performing the S_ISREG() test.

So you've broken the correctness of the HASH_EXECUTABLES_ONLY option.
Why not just leave it unset instead?  That's why it's an option.
 
}              if (unset(HASHEXECUTABLESONLY) ||
} -                   (stat(pathbuf, &statbuf) == 0 &&
} -                    S_ISREG(statbuf.st_mode) && (statbuf.st_mode & S_IXUGO)))
} +                   !access(pathbuf,X_OK))
}                  add = 1;
}          }

Re: [PATCH] Use access instead of stat in hashdir

[Bart Schaefer]

On Feb 6,  8:20am, Bart Schaefer wrote:
>
> On Feb 6,  6:43pm, Raghavendra D Prabhu wrote:
> } 
> } I found that using access instead of two stat calls results in 
> } faster rehash when it is done. I came across this when I noticed 
> } too many stat calls while 'strace -c' 
> 
> }              if (unset(HASHEXECUTABLESONLY) ||
> } -                   (stat(pathbuf, &statbuf) == 0 &&
> } -                    S_ISREG(statbuf.st_mode) && (statbuf.st_mode & S_IXUGO)))
> } +                   !access(pathbuf,X_OK))
> }                  add = 1;
> }          }

In Src/exec.c the function iscom():

int
iscom(char *s)
{
    struct stat statbuf;
    char *us = unmeta(s);

    return (access(us, X_OK) == 0 && stat(us, &statbuf) >= 0 &&
	    S_ISREG(statbuf.st_mode));
}

So let's see if we can get some speed out of this:

Index: Src/hashtable.c
===================================================================
diff -c -r1.20 Src/hashtable.c
--- hashtable.c	1 Jun 2011 06:40:00 -0000	1.20
+++ hashtable.c	6 Feb 2012 16:49:24 -0000
@@ -663,8 +663,10 @@
 		 * This is the same test as for the glob qualifier for
 		 * executable plain files.
 		 */
-		if (stat(pathbuf, &statbuf) == 0 &&
-		    S_ISREG(statbuf.st_mode) && (statbuf.st_mode & S_IXUGO))
+		if (unset(HASHEXECUTABLESONLY) ||
+		    (access(pathbuf, X_OK) == 0 &&
+		     stat(pathbuf, &statbuf) == 0 &&
+		     S_ISREG(statbuf.st_mode) && (statbuf.st_mode & S_IXUGO)))
 		    add = 1;
 	    }
 	    if (add) {

The S_IXUGO test may now be redundant, but hardly worth skipping.

Re: Re: [PATCH] Use access instead of stat in hashdir

[Raghavendra D Prabhu]

[-- Attachment #1: Type: text/plain, Size: 1450 bytes --]

Hi,


* On Mon, Feb 06, 2012 at 08:20:08AM -0800, Bart Schaefer <schaefer@brasslantern.com> wrote:
>Thanks for the suggestion, but ...
>
>On Feb 6,  6:43pm, Raghavendra D Prabhu wrote:
>}
>} I found that using access instead of two stat calls
>
>There's only one stat() call in the lines that you changed ...?
Sorry, I meant the other checks.
>
>} results in
>} faster rehash when it is done. I came across this when I noticed
>} too many stat calls while 'strace -c'
>
>It also results in treating non-regular files as candidates for being
>in the hash table, unless there's something about access() that is
>implicitly performing the S_ISREG() test.

Need to verify this but for X_OK the file will need to be regular 
right ?
>
>So you've broken the correctness of the HASH_EXECUTABLES_ONLY option.
>Why not just leave it unset instead?  That's why it's an option.
>
>}              if (unset(HASHEXECUTABLESONLY) ||
>} -                   (stat(pathbuf, &statbuf) == 0 &&
>} -                    S_ISREG(statbuf.st_mode) && (statbuf.st_mode & S_IXUGO)))
>} +                   !access(pathbuf,X_OK))
>}                  add = 1;
>}          }
>
Yes, I recently merged the tree with mine which I was using 
before HASHEXECUTABLESONLY was there, need to check on that.



Regards,
-- 
Raghavendra Prabhu
GPG Id : 0xD72BE977
Fingerprint: B93F EBCB 8E05 7039 CD3C A4B8 A616 DCA1 D72B E977
www: wnohang.net

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]

Re: Re: [PATCH] Use access instead of stat in hashdir

[Bart Schaefer]

On Tue, Feb 7, 2012 at 2:46 PM, Raghavendra D Prabhu
<raghu.prabhu13@gmail.com> wrote:
> * On Mon, Feb 06, 2012 at 08:20:08AM -0800, Bart Schaefer
> <schaefer@brasslantern.com> wrote:
>> } faster rehash when it is done. I came across this when I noticed
>> } too many stat calls while 'strace -c'
>>
>> It also results in treating non-regular files as candidates for being
>> in the hash table, unless there's something about access() that is
>> implicitly performing the S_ISREG() test.
>
> Need to verify this but for X_OK the file will need to be regular right ?

DESCRIPTION
       access()  checks whether the process would be allowed to read, write or
       test for existence of the file (or other file system object) whose name
       is  pathname.   If  pathname is a symbolic link permissions of the file
       referred to by this symbolic link are tested. [...]

       Only  access  bits  are checked, not the file type or contents. [...]

       If  the process has appropriate privileges, an implementation may indi-
       cate success for X_OK even if none of the execute file permission  bits
       are set.

Note "or other file system object."