Re: [PATCH] md5: accepts a new --threads option

[Pádraig Brady]

Jim Meyering wrote:
> Pádraig Brady wrote:
> ...
>> diff --git a/lib/copy-file.c b/lib/copy-file.c
> ...
>> +enum { IO_SIZE = 32*1024 };
> 
> Almost there.
> I like the enum.
> Did you consider making BLOCKSIZE, below, an enum, too?
> as long as you're changing it...

I did consider, but that would involve changing to
use the compiler to verify() it was a multiple of 64.
I thought that a little invasive and should be part of
a general refactoring that seems needed on those modules.

>> diff --git a/lib/md2.c b/lib/md2.c
> ...
>> +  char* buffer = malloc (BLOCKSIZE + 72);
> 
> Spacing again (search for 'char*'; there are more):
> 
>      char *buffer = malloc (BLOCKSIZE + 72);

Stupid muscle memory.
Attached.

cheers,
Pádraig.

>From d36ca2d173513e520c78217fd9a6a9907d9fa238 Mon Sep 17 00:00:00 2001
From: =?utf-8?q?P=C3=A1draig=20Brady?= <address@hidden>
Date: Thu, 22 Oct 2009 17:36:28 +0100
Subject: [PATCH] digests, copy-file: increase the IO buffer size from 4KiB to 
32KiB

This results in a significant decrease in syscall overhead
giving a 3% speedup to the digest utilities for example
(when processing large files from cache).
Storage is moved from the stack to the heap as some
threaded environments for example can have small stacks.

* lib/copy-file.c (copy_file_preserving): Use a 32KiB malloced buffer
* modules/copy-file: Depend on xalloc
* lib/md2.c: Likewise
* lib/md4.c: Likewise
* lib/md5.c: Likewise
* lib/sha1.c: Likewise
* lib/sha256.c: Likewise
* lib/sha512.c: Likewise
---
 ChangeLog         |   11 +++++++++++
 lib/copy-file.c   |    9 ++++++---
 lib/md2.c         |   14 +++++++++++---
 lib/md4.c         |   14 +++++++++++---
 lib/md5.c         |   13 ++++++++++---
 lib/sha1.c        |   14 +++++++++++---
 lib/sha256.c      |   25 ++++++++++++++++++++-----
 lib/sha512.c      |   25 ++++++++++++++++++++-----
 modules/copy-file |    1 +
 9 files changed, 101 insertions(+), 25 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 3271366..f320e99 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+2009-10-22  Pádraig Brady  <address@hidden>
+
+       Use a better IO block size for modern systems
+       * lib/copy-file.c (copy_file_preserving): Used a 32KiB malloced buffer.
+       * lib/md2.c: Likewise.
+       * lib/md4.c: Likewise.
+       * lib/md5.c: Likewise.
+       * lib/sha1.c: Likewise.
+       * lib/sha256.c: Likewise.
+       * lib/sha512.c: Likewise.
+
 2009-10-20  Eric Blake  <address@hidden>
 
        utimensat: work around Solaris 9 bug
diff --git a/lib/copy-file.c b/lib/copy-file.c
index 1587905..4d9b86a 100644
--- a/lib/copy-file.c
+++ b/lib/copy-file.c
@@ -42,6 +42,7 @@
 #include "acl.h"
 #include "binary-io.h"
 #include "gettext.h"
+#include "xalloc.h"
 
 #define _(str) gettext (str)
 
@@ -50,6 +51,7 @@
 #undef open
 #undef close
 
+enum { IO_SIZE = 32*1024 };
 
 void
 copy_file_preserving (const char *src_filename, const char *dest_filename)
@@ -58,8 +60,7 @@ copy_file_preserving (const char *src_filename, const char 
*dest_filename)
   struct stat statbuf;
   int mode;
   int dest_fd;
-  char buf[4096];
-  const size_t buf_size = sizeof (buf);
+  char *buf = xmalloc (IO_SIZE);
 
   src_fd = open (src_filename, O_RDONLY | O_BINARY);
   if (src_fd < 0 || fstat (src_fd, &statbuf) < 0)
@@ -76,7 +77,7 @@ copy_file_preserving (const char *src_filename, const char 
*dest_filename)
   /* Copy the file contents.  */
   for (;;)
     {
-      size_t n_read = safe_read (src_fd, buf, buf_size);
+      size_t n_read = safe_read (src_fd, buf, IO_SIZE);
       if (n_read == SAFE_READ_ERROR)
        error (EXIT_FAILURE, errno, _("error reading \"%s\""), src_filename);
       if (n_read == 0)
@@ -86,6 +87,8 @@ copy_file_preserving (const char *src_filename, const char 
*dest_filename)
        error (EXIT_FAILURE, errno, _("error writing \"%s\""), dest_filename);
     }
 
+  free (buf);
+
 #if !USE_ACL
   if (close (dest_fd) < 0)
     error (EXIT_FAILURE, errno, _("error writing \"%s\""), dest_filename);
diff --git a/lib/md2.c b/lib/md2.c
index cb4c63b..e585eb8 100644
--- a/lib/md2.c
+++ b/lib/md2.c
@@ -24,6 +24,7 @@
 
 #include "md2.h"
 
+#include <stdlib.h>
 #include <string.h>
 #include <sys/types.h>
 
@@ -33,7 +34,7 @@
 # include "unlocked-io.h"
 #endif
 
-#define BLOCKSIZE 4096
+#define BLOCKSIZE 32768
 #if BLOCKSIZE % 64 != 0
 # error "invalid BLOCKSIZE"
 #endif
@@ -94,9 +95,12 @@ int
 md2_stream (FILE *stream, void *resblock)
 {
   struct md2_ctx ctx;
-  char buffer[BLOCKSIZE + 72];
   size_t sum;
 
+  char *buffer = malloc (BLOCKSIZE + 72);
+  if (!buffer)
+    return 1;
+
   /* Initialize the computation context.  */
   md2_init_ctx (&ctx);
 
@@ -125,7 +129,10 @@ md2_stream (FILE *stream, void *resblock)
                 exit the loop after a partial read due to e.g., EAGAIN
                 or EWOULDBLOCK.  */
              if (ferror (stream))
-               return 1;
+               {
+                 free (buffer);
+                 return 1;
+               }
              goto process_partial_block;
            }
 
@@ -150,6 +157,7 @@ process_partial_block:;
 
   /* Construct result in desired memory.  */
   md2_finish_ctx (&ctx, resblock);
+  free (buffer);
   return 0;
 }
 
diff --git a/lib/md4.c b/lib/md4.c
index e348417..2adb82d 100644
--- a/lib/md4.c
+++ b/lib/md4.c
@@ -26,6 +26,7 @@
 
 #include <stddef.h>
 #include <stdlib.h>
+#include <stdlib.h>
 #include <string.h>
 #include <sys/types.h>
 
@@ -40,7 +41,7 @@
 # define SWAP(n) (n)
 #endif
 
-#define BLOCKSIZE 4096
+#define BLOCKSIZE 32768
 #if BLOCKSIZE % 64 != 0
 # error "invalid BLOCKSIZE"
 #endif
@@ -122,9 +123,12 @@ int
 md4_stream (FILE * stream, void *resblock)
 {
   struct md4_ctx ctx;
-  char buffer[BLOCKSIZE + 72];
   size_t sum;
 
+  char *buffer = malloc (BLOCKSIZE + 72);
+  if (!buffer)
+    return 1;
+
   /* Initialize the computation context.  */
   md4_init_ctx (&ctx);
 
@@ -153,7 +157,10 @@ md4_stream (FILE * stream, void *resblock)
                 exit the loop after a partial read due to e.g., EAGAIN
                 or EWOULDBLOCK.  */
              if (ferror (stream))
-               return 1;
+               {
+                 free (buffer);
+                 return 1;
+               }
              goto process_partial_block;
            }
 
@@ -178,6 +185,7 @@ process_partial_block:;
 
   /* Construct result in desired memory.  */
   md4_finish_ctx (&ctx, resblock);
+  free (buffer);
   return 0;
 }
 
diff --git a/lib/md5.c b/lib/md5.c
index 2213dc1..85c3553 100644
--- a/lib/md5.c
+++ b/lib/md5.c
@@ -56,7 +56,7 @@
 # define SWAP(n) (n)
 #endif
 
-#define BLOCKSIZE 4096
+#define BLOCKSIZE 32768
 #if BLOCKSIZE % 64 != 0
 # error "invalid BLOCKSIZE"
 #endif
@@ -136,9 +136,12 @@ int
 md5_stream (FILE *stream, void *resblock)
 {
   struct md5_ctx ctx;
-  char buffer[BLOCKSIZE + 72];
   size_t sum;
 
+  char *buffer = malloc (BLOCKSIZE + 72);
+  if (!buffer)
+    return 1;
+
   /* Initialize the computation context.  */
   md5_init_ctx (&ctx);
 
@@ -167,7 +170,10 @@ md5_stream (FILE *stream, void *resblock)
                 exit the loop after a partial read due to e.g., EAGAIN
                 or EWOULDBLOCK.  */
              if (ferror (stream))
-               return 1;
+               {
+                 free (buffer);
+                 return 1;
+               }
              goto process_partial_block;
            }
 
@@ -192,6 +198,7 @@ process_partial_block:
 
   /* Construct result in desired memory.  */
   md5_finish_ctx (&ctx, resblock);
+  free (buffer);
   return 0;
 }
 
diff --git a/lib/sha1.c b/lib/sha1.c
index 9c6c7ae..e72c94d 100644
--- a/lib/sha1.c
+++ b/lib/sha1.c
@@ -28,6 +28,7 @@
 #include "sha1.h"
 
 #include <stddef.h>
+#include <stdlib.h>
 #include <string.h>
 
 #if USE_UNLOCKED_IO
@@ -41,7 +42,7 @@
     (((n) << 24) | (((n) & 0xff00) << 8) | (((n) >> 8) & 0xff00) | ((n) >> 24))
 #endif
 
-#define BLOCKSIZE 4096
+#define BLOCKSIZE 32768
 #if BLOCKSIZE % 64 != 0
 # error "invalid BLOCKSIZE"
 #endif
@@ -124,9 +125,12 @@ int
 sha1_stream (FILE *stream, void *resblock)
 {
   struct sha1_ctx ctx;
-  char buffer[BLOCKSIZE + 72];
   size_t sum;
 
+  char *buffer = malloc (BLOCKSIZE + 72);
+  if (!buffer)
+    return 1;
+
   /* Initialize the computation context.  */
   sha1_init_ctx (&ctx);
 
@@ -155,7 +159,10 @@ sha1_stream (FILE *stream, void *resblock)
                 exit the loop after a partial read due to e.g., EAGAIN
                 or EWOULDBLOCK.  */
              if (ferror (stream))
-               return 1;
+               {
+                 free (buffer);
+                 return 1;
+               }
              goto process_partial_block;
            }
 
@@ -180,6 +187,7 @@ sha1_stream (FILE *stream, void *resblock)
 
   /* Construct result in desired memory.  */
   sha1_finish_ctx (&ctx, resblock);
+  free (buffer);
   return 0;
 }
 
diff --git a/lib/sha256.c b/lib/sha256.c
index 0ad9444..4bc8ac3 100644
--- a/lib/sha256.c
+++ b/lib/sha256.c
@@ -25,6 +25,7 @@
 #include "sha256.h"
 
 #include <stddef.h>
+#include <stdlib.h>
 #include <string.h>
 
 #if USE_UNLOCKED_IO
@@ -38,7 +39,7 @@
     (((n) << 24) | (((n) & 0xff00) << 8) | (((n) >> 8) & 0xff00) | ((n) >> 24))
 #endif
 
-#define BLOCKSIZE 4096
+#define BLOCKSIZE 32768
 #if BLOCKSIZE % 64 != 0
 # error "invalid BLOCKSIZE"
 #endif
@@ -169,9 +170,12 @@ int
 sha256_stream (FILE *stream, void *resblock)
 {
   struct sha256_ctx ctx;
-  char buffer[BLOCKSIZE + 72];
   size_t sum;
 
+  char *buffer = malloc (BLOCKSIZE + 72);
+  if (!buffer)
+    return 1;
+
   /* Initialize the computation context.  */
   sha256_init_ctx (&ctx);
 
@@ -200,7 +204,10 @@ sha256_stream (FILE *stream, void *resblock)
                 exit the loop after a partial read due to e.g., EAGAIN
                 or EWOULDBLOCK.  */
              if (ferror (stream))
-               return 1;
+               {
+                 free (buffer);
+                 return 1;
+               }
              goto process_partial_block;
            }
 
@@ -225,6 +232,7 @@ sha256_stream (FILE *stream, void *resblock)
 
   /* Construct result in desired memory.  */
   sha256_finish_ctx (&ctx, resblock);
+  free (buffer);
   return 0;
 }
 
@@ -233,9 +241,12 @@ int
 sha224_stream (FILE *stream, void *resblock)
 {
   struct sha256_ctx ctx;
-  char buffer[BLOCKSIZE + 72];
   size_t sum;
 
+  char *buffer = malloc (BLOCKSIZE + 72);
+  if (!buffer)
+    return 1;
+
   /* Initialize the computation context.  */
   sha224_init_ctx (&ctx);
 
@@ -264,7 +275,10 @@ sha224_stream (FILE *stream, void *resblock)
                 exit the loop after a partial read due to e.g., EAGAIN
                 or EWOULDBLOCK.  */
              if (ferror (stream))
-               return 1;
+               {
+                 free (buffer);
+                 return 1;
+               }
              goto process_partial_block;
            }
 
@@ -289,6 +303,7 @@ sha224_stream (FILE *stream, void *resblock)
 
   /* Construct result in desired memory.  */
   sha224_finish_ctx (&ctx, resblock);
+  free (buffer);
   return 0;
 }
 
diff --git a/lib/sha512.c b/lib/sha512.c
index 261a7bb..becafae 100644
--- a/lib/sha512.c
+++ b/lib/sha512.c
@@ -25,6 +25,7 @@
 #include "sha512.h"
 
 #include <stddef.h>
+#include <stdlib.h>
 #include <string.h>
 
 #if USE_UNLOCKED_IO
@@ -45,7 +46,7 @@
                         u64shr (n, 56))))
 #endif
 
-#define BLOCKSIZE 4096
+#define BLOCKSIZE 32768
 #if BLOCKSIZE % 128 != 0
 # error "invalid BLOCKSIZE"
 #endif
@@ -177,9 +178,12 @@ int
 sha512_stream (FILE *stream, void *resblock)
 {
   struct sha512_ctx ctx;
-  char buffer[BLOCKSIZE + 72];
   size_t sum;
 
+  char *buffer = malloc (BLOCKSIZE + 72);
+  if (!buffer)
+    return 1;
+
   /* Initialize the computation context.  */
   sha512_init_ctx (&ctx);
 
@@ -208,7 +212,10 @@ sha512_stream (FILE *stream, void *resblock)
                 exit the loop after a partial read due to e.g., EAGAIN
                 or EWOULDBLOCK.  */
              if (ferror (stream))
-               return 1;
+               {
+                 free (buffer);
+                 return 1;
+               }
              goto process_partial_block;
            }
 
@@ -233,6 +240,7 @@ sha512_stream (FILE *stream, void *resblock)
 
   /* Construct result in desired memory.  */
   sha512_finish_ctx (&ctx, resblock);
+  free (buffer);
   return 0;
 }
 
@@ -241,9 +249,12 @@ int
 sha384_stream (FILE *stream, void *resblock)
 {
   struct sha512_ctx ctx;
-  char buffer[BLOCKSIZE + 72];
   size_t sum;
 
+  char *buffer = malloc (BLOCKSIZE + 72);
+  if (!buffer)
+    return 1;
+
   /* Initialize the computation context.  */
   sha384_init_ctx (&ctx);
 
@@ -272,7 +283,10 @@ sha384_stream (FILE *stream, void *resblock)
                 exit the loop after a partial read due to e.g., EAGAIN
                 or EWOULDBLOCK.  */
              if (ferror (stream))
-               return 1;
+               {
+                 free (buffer);
+                 return 1;
+               }
              goto process_partial_block;
            }
 
@@ -297,6 +311,7 @@ sha384_stream (FILE *stream, void *resblock)
 
   /* Construct result in desired memory.  */
   sha384_finish_ctx (&ctx, resblock);
+  free (buffer);
   return 0;
 }
 
diff --git a/modules/copy-file b/modules/copy-file
index 5f1c6e9..6941e5a 100644
--- a/modules/copy-file
+++ b/modules/copy-file
@@ -16,6 +16,7 @@ gettext-h
 open
 safe-read
 unistd
+xalloc
 
 configure.ac:
 gl_COPY_FILE
-- 
1.6.2.5