Re: [PATCH] (x)memcoll: performance improvement when input is known to be NUL delimited.

[Chen Guo]

Hi Bruno,
    I tried with the checks for the presence of that last NUL byte like you 
suggested, and to my surprise on the remote machine I was testing on it was 
consistently (as in every run for 20 runs) faster, and thus I've included it in 
the patch. This is not what I expected at all, and I'm having a hard time 
coming up with a reason why this is; if someone has an explanation I'd love to 
know. I also tested on my laptop, but even after killing X the variance was too 
high for meaningful results.

    Even though gnulib seems to have it's own stdlib.h, I included <stdlib.h> 
in memcoll.c instead, since that's what xmemcoll.c includes.

>From 19516c730bced56670d180899d099e18be2a9ae3 Mon Sep 17 00:00:00 2001
From: Chen Guo <address@hidden>
Date: Sun, 7 Mar 2010 17:07:49 -0800
Subject: [PATCH] (x)memcoll: performance improvement when input is known to be
 NUL delimited.

This is in suport of a patch to coreutils' sort, where for each
input line xmemcoll is called several times. If each input line is
NUL delimited when read in, memcoll no longer needs to save the
last byte, NUL delimit, then put the last byte back every time the
line is compared. Sorting a 96MB, 1M line file, performance
improvement is roughly 1%.

* lib/memcoll.c: Include stdlib.
(memcoll0) New function.
(strcoll_loop) New function, refactored for use in both memcoll
and memcoll0.
* lib/memcoll.h: Add prototype for memcoll0.
* lib/xmemcoll.c: (xmemcoll0) New function.
(collate_error) New function, refactored for use in both xmemcoll
and xmemcoll0.
* lib/xmemcoll.h: Add prototype for xmemcoll0.
* m4/memcoll.m4: add inline invocation.
---
 lib/memcoll.c  |   88 +++++++++++++++++++++++++++++++++++--------------------
 lib/memcoll.h  |    1 +
 lib/xmemcoll.c |   35 +++++++++++++++++-----
 lib/xmemcoll.h |    1 +
 m4/memcoll.m4  |    1 +
 5 files changed, 86 insertions(+), 40 deletions(-)

diff --git a/lib/memcoll.c b/lib/memcoll.c
index e08ffa5..8e48551 100644
--- a/lib/memcoll.c
+++ b/lib/memcoll.c
@@ -23,20 +23,54 @@
 #include "memcoll.h"
 
 #include <errno.h>
+#include <stdlib.h>
 #include <string.h>
 
+/* Ensure strcoll operates on the entire input strings, in case they contain
+   NUL bytes. */
+
+static inline int
+strcoll_loop (const char *s1, size_t s1len, const char *s2, size_t s2len)
+{
+  int diff;
+  while (! (errno = 0, (diff = strcoll (s1, s2)) || errno))
+    {
+      /* strcoll found no difference, but perhaps it was fooled by NUL
+         characters in the data.  Work around this problem by advancing
+         past the NUL chars.  */
+      size_t size1 = strlen (s1) + 1;
+      size_t size2 = strlen (s2) + 1;
+      s1 += size1;
+      s2 += size2;
+      s1len -= size1;
+      s2len -= size2;
+
+      if (s1len == 0)
+        {
+          if (s2len != 0)
+            diff = -1;
+          break;
+        }
+      else if (s2len == 0)
+        {
+          diff = 1;
+          break;
+        }
+    }
+  return diff;
+}
+
 /* Compare S1 (with length S1LEN) and S2 (with length S2LEN) according
    to the LC_COLLATE locale.  S1 and S2 do not overlap, and are not
    adjacent.  Perhaps temporarily modify the bytes after S1 and S2,
    but restore their original contents before returning.  Set errno to an
    error number if there is an error, and to zero otherwise.  */
+
 int
 memcoll (char *s1, size_t s1len, char *s2, size_t s2len)
 {
   int diff;
 
-#if HAVE_STRCOLL
-
   /* strcoll is slow on many platforms, so check for the common case
      where the arguments are bytewise equal.  Otherwise, walk through
      the buffers using strcoll on each substring.  */
@@ -54,43 +88,33 @@ memcoll (char *s1, size_t s1len, char *s2, size_t s2len)
       s1[s1len++] = '\0';
       s2[s2len++] = '\0';
 
-      while (! (errno = 0, (diff = strcoll (s1, s2)) || errno))
-        {
-          /* strcoll found no difference, but perhaps it was fooled by NUL
-             characters in the data.  Work around this problem by advancing
-             past the NUL chars.  */
-          size_t size1 = strlen (s1) + 1;
-          size_t size2 = strlen (s2) + 1;
-          s1 += size1;
-          s2 += size2;
-          s1len -= size1;
-          s2len -= size2;
-
-          if (s1len == 0)
-            {
-              if (s2len != 0)
-                diff = -1;
-              break;
-            }
-          else if (s2len == 0)
-            {
-              diff = 1;
-              break;
-            }
-        }
+      diff = strcoll_loop (s1, s1len, s2, s2len);
 
       s1[s1len - 1] = n1;
       s2[s2len - 1] = n2;
     }
 
-#else
+  return diff;
+}
 
-  diff = memcmp (s1, s2, s1len < s2len ? s1len : s2len);
-  if (! diff)
-    diff = s1len < s2len ? -1 : s1len != s2len;
-  errno = 0;
+/* Like memcoll, but S1 and S2 are known to be NUL delimited, thus no
+   modification to S1 or S2 are needed. */
+int
+memcoll0 (const char *s1, size_t s1len, const char *s2, size_t s2len)
+{
+  int diff;
+  if (!(s1len > 0 && s1[s1len] == '\0'))
+    abort ();
+  if (!(s2len > 0 && s2[s2len] == '\0'))
+    abort ();
 
-#endif
+  if (s1len == s2len && memcmp (s1, s2, s1len) == 0)
+    {
+      errno = 0;
+      diff = 0;
+    }
+  else
+    diff = strcoll_loop (s1, s1len, s2, s2len);
 
   return diff;
 }
diff --git a/lib/memcoll.h b/lib/memcoll.h
index 8f2e1b1..b43a96e 100644
--- a/lib/memcoll.h
+++ b/lib/memcoll.h
@@ -23,5 +23,6 @@
 # include <stddef.h>
 
 int memcoll (char *, size_t, char *, size_t);
+int memcoll0 (const char *, size_t, const char *, size_t);
 
 #endif /* MEMCOLL_H_ */
diff --git a/lib/xmemcoll.c b/lib/xmemcoll.c
index 84bbd8c..36958f4 100644
--- a/lib/xmemcoll.c
+++ b/lib/xmemcoll.c
@@ -31,6 +31,18 @@
 #include "quotearg.h"
 #include "xmemcoll.h"
 
+static inline void
+collate_error (int collation_errno, const char *s1, size_t s1len,
+               const char *s2, size_t s2len)
+{
+  error (0, collation_errno, _("string comparison failed"));
+  error (0, 0, _("Set LC_ALL='C' to work around the problem."));
+  error (exit_failure, 0,
+         _("The strings compared were %s and %s."),
+         quotearg_n_style_mem (0, locale_quoting_style, s1, s1len),
+         quotearg_n_style_mem (1, locale_quoting_style, s2, s2len));
+}
+
 /* Compare S1 (with length S1LEN) and S2 (with length S2LEN) according
    to the LC_COLLATE locale.  S1 and S2 do not overlap, and are not
    adjacent.  Temporarily modify the bytes after S1 and S2, but
@@ -44,14 +56,21 @@ xmemcoll (char *s1, size_t s1len, char *s2, size_t s2len)
   int collation_errno = errno;
 
   if (collation_errno)
-    {
-      error (0, collation_errno, _("string comparison failed"));
-      error (0, 0, _("Set LC_ALL='C' to work around the problem."));
-      error (exit_failure, 0,
-             _("The strings compared were %s and %s."),
-             quotearg_n_style_mem (0, locale_quoting_style, s1, s1len),
-             quotearg_n_style_mem (1, locale_quoting_style, s2, s2len));
-    }
+    collate_error (collation_errno, s1, s1len, s2, s2len);
+
+  return diff;
+}
 
+/* Like xmemcoll, but S1 and S2 are known to be NUL delimited, thus
+   no modifications to S1 and S2 are needed. */
+
+int
+xmemcoll0 (const char *s1, size_t s1len, const char *s2, size_t s2len)
+{
+  int diff = memcoll0 (s1, s1len, s2, s2len);
+  int collation_errno = errno;
+
+  if (collation_errno)
+    collate_error (collation_errno, s1, s1len, s2, s2len);
   return diff;
 }
diff --git a/lib/xmemcoll.h b/lib/xmemcoll.h
index 2f422e8..b72127d 100644
--- a/lib/xmemcoll.h
+++ b/lib/xmemcoll.h
@@ -1,2 +1,3 @@
 #include <stddef.h>
 int xmemcoll (char *, size_t, char *, size_t);
+int xmemcoll0 (const char *, size_t, const char *, size_t);
diff --git a/m4/memcoll.m4 b/m4/memcoll.m4
index 1c4bd61..0edf30c 100644
--- a/m4/memcoll.m4
+++ b/m4/memcoll.m4
@@ -7,6 +7,7 @@ dnl with or without modifications, as long as this notice is 
preserved.
 
 AC_DEFUN([gl_MEMCOLL],
 [
+  AC_REQUIRE([AC_C_INLINE])
   AC_LIBOBJ([memcoll])
 
   dnl Prerequisites of lib/memcoll.c.
-- 
1.6.3.3