Re: cvs commit: src/usr.bin/xinstall xinstall.c

[Bruce Evans]

On Wed, 20 Mar 2002, Ruslan Ermilov wrote:

> On Mon, Mar 18, 2002 at 03:26:13PM -0800, Dag-Erling Smorgrav wrote:
> > des 2002/03/18 15:26:13 PST
> >
> >   Modified files:
> > usr.bin/xinstall xinstall.c
> >   Log:
> >   Bump the cutoff mark for comparing files from 8 MB to 16 MB.
> >
> >   Revision  ChangesPath
> >   1.48  +3 -1  src/usr.bin/xinstall/xinstall.c
> >
> OK, I think it's finally the time to borrow mmap(2) comparison in
> chunks from OpenBSD (better viewed as -w diff):

Using mmap(2) at all is over-engineered IMO.  Does it make even 1 1%
difference for heavy uses like installworld?  I get the following times
on an Athlon1600 for installing an 8MB file 16 times after it has
already been installed and caches warmed up:

install -C:0.48 real 0.38 user 0.09 sys
install -MC:   0.86 real 0.26 user 0.59 sys

and for installing a copy of /usr/bin/*:

install -C:0.29 real 0.24 user 0.04 sys
install -MC:   0.54 real 0.10 user 0.42 sys

Using mmap(2) in cmp(1) makes more difference because the non-mmap
case of cmp(1) is poorly implemented.  cmp(1) uses a getc() loop, but
install(1) reads MAXBSIZE at a time).

> %%%
> Index: xinstall.c
> ===
> RCS file: /home/ncvs/src/usr.bin/xinstall/xinstall.c,v
> retrieving revision 1.48
> diff -u -p -r1.48 xinstall.c
> --- xinstall.c18 Mar 2002 23:26:13 -  1.48
> +++ xinstall.c20 Mar 2002 09:26:11 -
> @@ -74,12 +74,11 @@ static const char sccsid[] = "From: @(#)
> ...
> +out:
> + if (!done_compare && from_len <= MAX_CMP_SIZE) {
> + char buf1[MAXBSIZE];
> + char buf2[MAXBSIZE];
> + int n1, n2;

This is cleaner than before, but still has style bugs (nested declarations),
and still attempts to pessimize the !mmap case by using misaligned buffers
(copy() is missing the style bugs but not the misaligment attempt).

> ...
>   } else
>   rv = 1; /* don't bother in this case */

We should bother now, to give the documented semantics for -C (remove the
"from_len <= MAX_CMP_SIZE" check above).

Bruce


To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message

Re: cvs commit: src/usr.bin/xinstall xinstall.c

[Dag-Erling Smorgrav]

Ruslan Ermilov <[EMAIL PROTECTED]> writes:
> OK, I think it's finally the time to borrow mmap(2) comparison in
> chunks from OpenBSD (better viewed as -w diff):

What are you waiting for?  Commit! :)

DES
-- 
Dag-Erling Smorgrav - [EMAIL PROTECTED]

To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message

Re: cvs commit: src/usr.bin/xinstall xinstall.c

[Ruslan Ermilov]

On Mon, Mar 18, 2002 at 03:26:13PM -0800, Dag-Erling Smorgrav wrote:
> des 2002/03/18 15:26:13 PST
> 
>   Modified files:
> usr.bin/xinstall xinstall.c 
>   Log:
>   Bump the cutoff mark for comparing files from 8 MB to 16 MB.
>   
>   Revision  ChangesPath
>   1.48  +3 -1  src/usr.bin/xinstall/xinstall.c
> 
OK, I think it's finally the time to borrow mmap(2) comparison in
chunks from OpenBSD (better viewed as -w diff):

%%%
Index: xinstall.c
===
RCS file: /home/ncvs/src/usr.bin/xinstall/xinstall.c,v
retrieving revision 1.48
diff -u -p -r1.48 xinstall.c
--- xinstall.c  18 Mar 2002 23:26:13 -  1.48
+++ xinstall.c  20 Mar 2002 09:26:11 -
@@ -74,12 +74,11 @@ static const char sccsid[] = "From: @(#)
 #define MAP_FAILED ((void *)-1)/* from  */
 #endif
 
-#define MAX_CMP_SIZE   (16 * 1024 * 1024)
-
 #defineDIRECTORY   0x01/* Tell install it's a directory. */
 #defineSETFLAGS0x02/* Tell install to set flags. */
 #defineNOCHANGEBITS(UF_IMMUTABLE | UF_APPEND | SF_IMMUTABLE | SF_APPEND)
 #defineBACKUP_SUFFIX   ".old"
+#defineMAX_CMP_SIZE(16 * 1024 * 1024)
 
 struct passwd *pp;
 struct group *gp;
@@ -523,6 +522,8 @@ compare(int from_fd, const char *from_na
int to_fd, const char *to_name __unused, size_t to_len)
 {
char *p, *q;
+   size_t len, remainder;
+   off_t from_off, to_off;
int rv;
int done_compare;
 
@@ -530,48 +531,60 @@ compare(int from_fd, const char *from_na
if (from_len != to_len)
return 1;
 
-   if (from_len <= MAX_CMP_SIZE) {
-   done_compare = 0;
-   if (trymmap(from_fd) && trymmap(to_fd)) {
-   p = mmap(NULL, from_len, PROT_READ, MAP_SHARED, from_fd, 
(off_t)0);
+   done_compare = 0;
+   if (trymmap(from_fd) && trymmap(to_fd)) {
+   /*
+* Compare two files being careful not to mmap
+* more than 8M at a time.
+*/
+   from_off = to_off = (off_t)0;
+   remainder = from_len;
+   while (!rv && remainder > 0) {
+   len = MIN(remainder, 8 * 1024 * 1024);
+
+   p = mmap(NULL, len, PROT_READ, MAP_SHARED, from_fd, from_off);
if (p == (char *)MAP_FAILED)
goto out;
-   q = mmap(NULL, from_len, PROT_READ, MAP_SHARED, to_fd, 
(off_t)0);
+   q = mmap(NULL, len, PROT_READ, MAP_SHARED, to_fd, to_off);
if (q == (char *)MAP_FAILED) {
-   munmap(p, from_len);
+   munmap(p, len);
goto out;
}
 
-   rv = memcmp(p, q, from_len);
-   munmap(p, from_len);
-   munmap(q, from_len);
-   done_compare = 1;
-   }
-   out:
-   if (!done_compare) {
-   char buf1[MAXBSIZE];
-   char buf2[MAXBSIZE];
-   int n1, n2;
-
-   rv = 0;
-   lseek(from_fd, 0, SEEK_SET);
-   lseek(to_fd, 0, SEEK_SET);
-   while (rv == 0) {
-   n1 = read(from_fd, buf1, sizeof(buf1));
-   if (n1 == 0)
-   break;  /* EOF */
-   else if (n1 > 0) {
-   n2 = read(to_fd, buf2, n1);
-   if (n2 == n1)
-   rv = memcmp(buf1, buf2, n1);
-   else
-   rv = 1; /* out of sync */
-   } else
-   rv = 1; /* read failure */
-   }
-   lseek(from_fd, 0, SEEK_SET);
-   lseek(to_fd, 0, SEEK_SET);
+   rv = memcmp(p, q, len);
+   munmap(p, len);
+   munmap(q, len);
+
+   from_off += len;
+   to_off += len;
+   remainder -= len;
+   }
+   done_compare = 1;
+   }
+out:
+   if (!done_compare && from_len <= MAX_CMP_SIZE) {
+   char buf1[MAXBSIZE];
+   char buf2[MAXBSIZE];
+   int n1, n2;
+
+   rv = 0;
+   lseek(from_fd, 0, SEEK_SET);
+   lseek(to_fd, 0, SEEK_SET);
+   while (rv == 0) {
+   n1 = read(from_fd, buf1, sizeof(buf1));
+   if (n1 == 0)
+