Libguestfs - Apr 2017 - Re: [PATCH] daemon: Move the useful 'is_zero' function into common code.

This is largely a simple refactoring, but it combines another
definition of this function from virt-builder which had a slightly
different prototype.
---
 builder/pxzcat-c.c         | 20 +-------------------
 daemon/daemon.h            | 18 ------------------
 lib/guestfs-internal-all.h | 18 ++++++++++++++++++
 3 files changed, 19 insertions(+), 37 deletions(-)

diff --git a/builder/pxzcat-c.c b/builder/pxzcat-c.c
index 33923b532..ef37849ed 100644
--- a/builder/pxzcat-c.c
+++ b/builder/pxzcat-c.c
@@ -409,24 +409,6 @@ parse_indexes (value filenamev, int fd)
   return combined_index;
 }
 
-/* Return true iff the buffer is all zero bytes.
- *
- * Note that gcc is smart enough to optimize this properly:
- *
http://stackoverflow.com/questions/1493936/faster-means-of-checking-for-an-empty-buffer-in-c/1493989#1493989
- */
-static inline int
-is_zero (const unsigned char *buffer, size_t size)
-{
-  size_t i;
-
-  for (i = 0; i < size; ++i) {
-    if (buffer[i] != 0)
-      return 0;
-  }
-
-  return 1;
-}
-
 struct global_state {
   /* Current iterator.  Threads update this, but it is protected by a
    * mutex, and each thread takes a copy of it when working on it.
@@ -681,7 +663,7 @@ worker_thread (void *vp)
         /* Don't write if the block is all zero, to preserve output file
          * sparseness.  However we have to update oposition.
          */
-        if (!is_zero (outbuf, wsz)) {
+        if (!is_zero ((char *) outbuf, wsz)) {
           if (xpwrite (global->ofd, outbuf, wsz, oposition) == -1) {
             perror (global->outputfile);
             return &state->status;
diff --git a/daemon/daemon.h b/daemon/daemon.h
index 332e2966a..a317139a5 100644
--- a/daemon/daemon.h
+++ b/daemon/daemon.h
@@ -327,24 +327,6 @@ extern void pulse_mode_cancel (void);
  */
 extern void notify_progress_no_ratelimit (uint64_t position, uint64_t total,
const struct timeval *now);
 
-/* Return true iff the buffer is all zero bytes.
- *
- * Note that gcc is smart enough to optimize this properly:
- *
http://stackoverflow.com/questions/1493936/faster-means-of-checking-for-an-empty-buffer-in-c/1493989#1493989
- */
-static inline int
-is_zero (const char *buffer, size_t size)
-{
-  size_t i;
-
-  for (i = 0; i < size; ++i) {
-    if (buffer[i] != 0)
-      return 0;
-  }
-
-  return 1;
-}
-
 /* Helper for functions that need a root filesystem mounted.
  * NB. Cannot be used for FileIn functions.
  */
diff --git a/lib/guestfs-internal-all.h b/lib/guestfs-internal-all.h
index 0c841ff67..4f7433332 100644
--- a/lib/guestfs-internal-all.h
+++ b/lib/guestfs-internal-all.h
@@ -89,6 +89,24 @@
 #define xdr_uint32_t xdr_u_int32_t
 #endif
 
+/* Return true iff the buffer is all zero bytes.
+ *
+ * Note that gcc is smart enough to optimize this properly:
+ *
http://stackoverflow.com/questions/1493936/faster-means-of-checking-for-an-empty-buffer-in-c/1493989#1493989
+ */
+static inline int
+is_zero (const char *buffer, size_t size)
+{
+  size_t i;
+
+  for (i = 0; i < size; ++i) {
+    if (buffer[i] != 0)
+      return 0;
+  }
+
+  return 1;
+}
+
 /* Macro which compiles the regexp once when the program/library is
  * loaded, and frees it when the library is unloaded.
  */
-- 
2.12.0

On 04/19/2017 09:17 AM, Richard W.M. Jones wrote:
> This is largely a simple refactoring, but it combines another
> definition of this function from virt-builder which had a slightly
> different prototype.
> ---
> +/* Return true iff the buffer is all zero bytes.
> + *
> + * Note that gcc is smart enough to optimize this properly:
> + *
http://stackoverflow.com/questions/1493936/faster-means-of-checking-for-an-empty-buffer-in-c/1493989#1493989
> + */
> +static inline int
> +is_zero (const char *buffer, size_t size)
> +{
> +  size_t i;
> +
> +  for (i = 0; i < size; ++i) {
> +    if (buffer[i] != 0)
> +      return 0;
> +  }
> +
> +  return 1;
> +}
This is still byte-at-a-time.  Often, you can get even faster results by
exploiting libc's optimizations in memcmp (particularly when it is
comparing pointers with nice alignments), by manually checking that the
first 16 bytes are zero, then letting memcmp do the rest:

size_t i;
size_t limit = MIN(size, 16);
for (i = 0; i < limit; ++i)
  if (buffer[i])
    return 0;
if (size != limit)
  return !memcmp(buffer, buffer + 16, size - 16);
return 1;

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On Wed, Apr 19, 2017 at 09:35:16AM -0500, Eric Blake
wrote:
> On 04/19/2017 09:17 AM, Richard W.M. Jones wrote:
> > This is largely a simple refactoring, but it combines another
> > definition of this function from virt-builder which had a slightly
> > different prototype.
> > ---
> 
> > +/* Return true iff the buffer is all zero bytes.
> > + *
> > + * Note that gcc is smart enough to optimize this properly:
> > + *
http://stackoverflow.com/questions/1493936/faster-means-of-checking-for-an-empty-buffer-in-c/1493989#1493989
> > + */
> > +static inline int
> > +is_zero (const char *buffer, size_t size)
> > +{
> > +  size_t i;
> > +
> > +  for (i = 0; i < size; ++i) {
> > +    if (buffer[i] != 0)
> > +      return 0;
> > +  }
> > +
> > +  return 1;
> > +}
> 
> This is still byte-at-a-time.  Often, you can get even faster results by
> exploiting libc's optimizations in memcmp (particularly when it is
> comparing pointers with nice alignments), by manually checking that the
> first 16 bytes are zero, then letting memcmp do the rest:
> 
> size_t i;
> size_t limit = MIN(size, 16);
> for (i = 0; i < limit; ++i)
>   if (buffer[i])
>     return 0;
> if (size != limit)
>   return !memcmp(buffer, buffer + 16, size - 16);
> return 1;
If performance is critical, QEMU has a permissively licensed impl that
uses sse/avx instructions to super-optimize it on x86 (util/bufferiszero.c).
It is a large amount of code though, so only worth it if this is a notable
bottleneck for libguestfs.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|