Re: [Libguestfs] [libnbd PATCH v2 8/8] python: Make nbd.Buffer lighter-weight

7 Jun 2022

On Mon, Jun 06, 2022 at 09:08:33PM -0500, Eric Blake wrote:
...
Instead of storing a PyCapsule in _o and repeatedly doing lookups to
dereference a stored malloc'd pointer, it is easier to just directly
store a Python buffer-like object as _o.  Track initialization via
._init: absent for nbd.Buffer(int), present for
nbd.Buffer.from_bytearray or after we have forced initialization due
to aio_pread or .to_bytearray.  At this point, we are still copying in
and out of .{from,to}_bytearray, but we are getting closer to reducing
the number of copies.  The sheer size in reduced lines of code is a
testament to the ease of doing things closer to Python, rather than
hidden behind unpacking a PyCapsule layer.
Acceptable side effect: nbd.Buffer(-1) changes from:
RuntimeError: length < 0
to
SystemError: Negative size passed to PyByteArray_FromStringAndSize
The generated code changes as follows:
| --- python/methods.h.bak	2022-06-06 20:36:01.973327739 -0500
| +++ python/methods.h	2022-06-06 20:36:16.146352508 -0500
| @@ -62,9 +62,6 @@
|  extern PyObject *nbd_internal_py_close (PyObject *self, PyObject *args);
|  extern PyObject *nbd_internal_py_display_version (PyObject *self, PyObject *args);
|  extern PyObject *nbd_internal_py_alloc_aio_buffer (PyObject *self, PyObject *args);
| -extern PyObject *nbd_internal_py_aio_buffer_from_bytearray (PyObject *self, PyObject *args);
| -extern PyObject *nbd_internal_py_aio_buffer_to_bytearray (PyObject *self, PyObject *args);
| -extern PyObject *nbd_internal_py_aio_buffer_size (PyObject *self, PyObject *args);
|  extern PyObject *nbd_internal_py_aio_buffer_is_zero (PyObject *self, PyObject *args);
|  extern PyObject *nbd_internal_py_set_debug (PyObject *self, PyObject *args);
|  extern PyObject *nbd_internal_py_get_debug (PyObject *self, PyObject *args);
| --- python/nbd.py.bak	2022-06-06 20:36:01.978327748 -0500
| +++ python/nbd.py	2022-06-06 20:56:41.866729560 -0500
| @@ -134,18 +134,21 @@ class Buffer(object):
|          bytearray constructor.  Otherwise, ba is copied.  Either way, the
|          resulting AIO buffer is independent from the original.
|          '''
| -        o = libnbdmod.aio_buffer_from_bytearray(ba)
|          self = cls(0)
| -        self._o = o
| +        self._o = bytearray(ba)
| +        self._init = True
|          return self
|
|      def to_bytearray(self):
|          '''Copy an AIO buffer into a bytearray.'''
| -        return libnbdmod.aio_buffer_to_bytearray(self)
| +        if not hasattr(self, '_init'):
| +            self._o = bytearray(len(self._o))
| +            self._init = True
| +        return bytearray(self._o)
|
|      def size(self):
|          '''Return the size of an AIO buffer.'''
| -        return libnbdmod.aio_buffer_size(self)
| +        return len(self._o)
|
|      def is_zero(self, offset=0, size=-1):
|          '''Returns true if and only if all bytes in the buffer are zeroes.
| @@ -161,7 +164,8 @@ class Buffer(object):
|          always returns true.  If size > 0, we check the interval
|          [offset..offset+size-1].
|          '''
| -        return libnbdmod.aio_buffer_is_zero(self, offset, size)
| +        return libnbdmod.aio_buffer_is_zero(self._o, offset, size,
| +                                            hasattr(self, '_init'))
|
|
|  class NBD(object):
---
 generator/Python.ml |  20 ++--
 python/handle.c     | 242 +++++++++-----------------------------------
 2 files changed, 56 insertions(+), 206 deletions(-)

diff --git a/generator/Python.ml b/generator/Python.ml
index c49af4f..86781ac 100644
--- a/generator/Python.ml
+++ b/generator/Python.ml
@@ -73,9 +73,6 @@ let
   ) ([ "create"; "close";
        "display_version";
        "alloc_aio_buffer";
-       "aio_buffer_from_bytearray";
-       "aio_buffer_to_bytearray";
-       "aio_buffer_size";
        "aio_buffer_is_zero"] @ List.map fst handle_calls);
pr "\n";
@@ -105,9 +102,6 @@ let
   ) ([ "create"; "close";
        "display_version";
        "alloc_aio_buffer";
-       "aio_buffer_from_bytearray";
-       "aio_buffer_to_bytearray";
-       "aio_buffer_size";
        "aio_buffer_is_zero"] @ List.map fst handle_calls);
   pr "  { NULL, NULL, 0, NULL }\n";
   pr "};\n";
@@ -748,18 +742,21 @@ let
         bytearray constructor.  Otherwise, ba is copied.  Either way, the
         resulting AIO buffer is independent from the original.
         '''
-        o = libnbdmod.aio_buffer_from_bytearray(ba)
         self = cls(0)
-        self._o = o
+        self._o = bytearray(ba)
+        self._init = True
         return self
def to_bytearray(self):
         '''Copy an AIO buffer into a bytearray.'''
-        return libnbdmod.aio_buffer_to_bytearray(self)
+        if not hasattr(self, '_init'):
+            self._o = bytearray(len(self._o))
+            self._init = True
+        return bytearray(self._o)
def size(self):
         '''Return the size of an AIO buffer.'''
-        return libnbdmod.aio_buffer_size(self)
+        return len(self._o)
def is_zero(self, offset=0, size=-1):
         '''Returns true if and only if all bytes in the buffer are zeroes.
@@ -775,7 +772,8 @@ let
         always returns true.  If size > 0, we check the interval
         [offset..offset+size-1].
         '''
-        return libnbdmod.aio_buffer_is_zero(self, offset, size)
+        return libnbdmod.aio_buffer_is_zero(self._o, offset, size,
+                                            hasattr(self, '_init'))
class NBD(object):
diff --git a/python/handle.c b/python/handle.c
index 79b369d..ca6c8e9 100644
--- a/python/handle.c
+++ b/python/handle.c
@@ -40,12 +40,6 @@
#include "methods.h"
-struct py_aio_buffer {
-  Py_ssize_t len;
-  void *data;
-  bool initialized;
-};
-
 static inline PyObject *
 put_handle (struct nbd_handle *h)
 {
@@ -102,242 +96,100 @@ nbd_internal_py_display_version (PyObject *self, PyObject *args)
   return Py_None;
 }
-static const char aio_buffer_name[] = "nbd.Buffer";
-
-/* Return internal buffer pointer of nbd.Buffer */
-static struct py_aio_buffer *
-nbd_internal_py_get_aio_buffer (PyObject *object)
+/* Return new reference to MemoryView wrapping aio_buffer contents */
+PyObject *
+nbd_internal_py_get_aio_view (PyObject *object, bool require_init)
 {
+  PyObject *buffer = NULL;
+
   if (PyObject_IsInstance (object, nbd_internal_py_get_nbd_buffer_type ())) {
-    PyObject *capsule = PyObject_GetAttrString(object, "_o");
+    buffer = PyObject_GetAttrString (object, "_o");
-    return PyCapsule_GetPointer (capsule, aio_buffer_name);
+    if (require_init && ! PyObject_HasAttrString (object, "_init")) {
+      assert (PyByteArray_Check (buffer));
+      memset (PyByteArray_AS_STRING (buffer), 0,
+              PyByteArray_GET_SIZE (buffer));
+      if (PyObject_SetAttrString (object, "_init", Py_True) < 0)
+        return NULL;
+    }
   }
+  if (buffer)
+    return PyMemoryView_FromObject (buffer);
+
   PyErr_SetString (PyExc_TypeError,
                    "aio_buffer: expecting nbd.Buffer instance");
   return NULL;
 }
-/* Return new reference to MemoryView wrapping aio_buffer contents */
-PyObject *
-nbd_internal_py_get_aio_view (PyObject *object, bool require_init)
-{
-  struct py_aio_buffer *buf = nbd_internal_py_get_aio_buffer (object);
-
-  if (!buf)
-    return NULL;
-
-  if (require_init && !buf->initialized) {
-    memset (buf->data, 0, buf->len);
-    buf->initialized = true;
-  }
-
-  return PyMemoryView_FromMemory (buf->data, buf->len,
-                                  require_init ? PyBUF_READ : PyBUF_WRITE);
-}
-
 int
 nbd_internal_py_init_aio_buffer (PyObject *object)
 {
-  struct py_aio_buffer *buf = nbd_internal_py_get_aio_buffer (object);
-
-  assert (buf);
-  buf->initialized = true;
+  if (PyObject_IsInstance (object, nbd_internal_py_get_nbd_buffer_type ()))
+    return PyObject_SetAttrString (object, "_init", Py_True);
   return 0;
 }
-static void
-free_aio_buffer (PyObject *capsule)
-{
-  struct py_aio_buffer *buf = PyCapsule_GetPointer (capsule, aio_buffer_name);
-
-  if (buf)
-    free (buf->data);
-  free (buf);
-}
-
-/* Allocate a persistent buffer used for nbd_aio_pread. */
+/* Allocate an uninitialized persistent buffer used for nbd_aio_pread. */
 PyObject *
 nbd_internal_py_alloc_aio_buffer (PyObject *self, PyObject *args)
 {
-  struct py_aio_buffer *buf;
-  PyObject *ret;
+  Py_ssize_t len;
-  buf = malloc (sizeof *buf);
-  if (buf == NULL) {
-    PyErr_NoMemory ();
-    return NULL;
-  }
-
-  buf->initialized = false;
   if (!PyArg_ParseTuple (args, "n:nbd_internal_py_alloc_aio_buffer",
-                         &buf->len)) {
-    free (buf);
+                         &len))
     return NULL;
-  }
-  if (buf->len < 0) {
-    PyErr_SetString (PyExc_RuntimeError, "length < 0");
-    free (buf);
-    return NULL;
-  }
-  buf->data = malloc (buf->len);
-  if (buf->data == NULL) {
-    PyErr_NoMemory ();
-    free (buf);
-    return NULL;
-  }
-
-  ret = PyCapsule_New (buf, aio_buffer_name, free_aio_buffer);
-  if (ret == NULL) {
-    free (buf->data);
-    free (buf);
-    return NULL;
-  }
-
-  return ret;
-}
-
-PyObject *
-nbd_internal_py_aio_buffer_from_bytearray (PyObject *self, PyObject *args)
-{
-  PyObject *obj;
-  PyObject *arr = NULL;
-  Py_ssize_t len;
-  void *data;
-  struct py_aio_buffer *buf;
-  PyObject *ret;
-
-  if (!PyArg_ParseTuple (args,
-                         "O:nbd_internal_py_aio_buffer_from_bytearray",
-                         &obj))
-    return NULL;
-
-  if (! PyByteArray_Check (obj)) {
-    arr = PyByteArray_FromObject (obj);
-    if (arr == NULL)
-      return NULL;
-    obj = arr;
-  }
-  data = PyByteArray_AsString (obj);
-  if (!data) {
-    PyErr_SetString (PyExc_RuntimeError,
-                     "parameter is not a bytearray or buffer");
-    Py_XDECREF (arr);
-    return NULL;
-  }
-  len = PyByteArray_Size (obj);
-
-  buf = malloc (sizeof *buf);
-  if (buf == NULL) {
-    PyErr_NoMemory ();
-    Py_XDECREF (arr);
-    return NULL;
-  }
-
-  buf->len = len;
-  buf->data = malloc (len);
-  if (buf->data == NULL) {
-    PyErr_NoMemory ();
-    free (buf);
-    Py_XDECREF (arr);
-    return NULL;
-  }
-  memcpy (buf->data, data, len);
-  Py_XDECREF (arr);
-  buf->initialized = true;
-
-  ret = PyCapsule_New (buf, aio_buffer_name, free_aio_buffer);
-  if (ret == NULL) {
-    free (buf->data);
-    free (buf);
-    return NULL;
-  }
-
-  return ret;
-}
-
-PyObject *
-nbd_internal_py_aio_buffer_to_bytearray (PyObject *self, PyObject *args)
-{
-  PyObject *obj;
-  PyObject *view;
-  PyObject *ret;
-
-  if (!PyArg_ParseTuple (args,
-                         "O:nbd_internal_py_aio_buffer_to_bytearray",
-                         &obj))
-    return NULL;
-
-  view = nbd_internal_py_get_aio_view (obj, true);
-  if (view == NULL)
-    return NULL;
-
-  ret = PyByteArray_FromObject (view);
-  Py_DECREF (view);
-  return ret;
-}
-
-PyObject *
-nbd_internal_py_aio_buffer_size (PyObject *self, PyObject *args)
-{
-  PyObject *obj;
-  struct py_aio_buffer *buf;
-
-  if (!PyArg_ParseTuple (args,
-                         "O:nbd_internal_py_aio_buffer_size",
-                         &obj))
-    return NULL;
-
-  buf = nbd_internal_py_get_aio_buffer (obj);
-  if (buf == NULL)
-    return NULL;
-
-  return PyLong_FromSsize_t (buf->len);
+  /* Constructing bytearray(len) in python zeroes the memory; doing it this
+   * way gives uninitialized memory.  This correctly flags negative len.
+   */
+  return PyByteArray_FromStringAndSize (NULL, len);
 }
PyObject *
 nbd_internal_py_aio_buffer_is_zero (PyObject *self, PyObject *args)
 {
-  PyObject *obj;
-  struct py_aio_buffer *buf;
+  Py_buffer buf;
   Py_ssize_t offset, size;
+  int init;
+  PyObject *ret = NULL;
if (!PyArg_ParseTuple (args,
-                         "Onn:nbd_internal_py_aio_buffer_is_zero",
-                         &obj, &offset, &size))
+                         "y*nnp:nbd_internal_py_aio_buffer_is_zero",
+                         &buf, &offset, &size, &init))
     return NULL;
-  if (size == 0)
-    Py_RETURN_TRUE;
-
-  buf = nbd_internal_py_get_aio_buffer (obj);
-  if (buf == NULL)
-    return NULL;
+  if (size == 0) {
+    ret = Py_True;
+    goto out;
+  }
/* Check the bounds of the offset. */
-  if (offset < 0 || offset > buf->len) {
+  if (offset < 0 || offset > buf.len) {
     PyErr_SetString (PyExc_IndexError, "offset out of range");
-    return NULL;
+    goto out;
   }
/* Compute or check the length. */
   if (size == -1)
-    size = buf->len - offset;
+    size = buf.len - offset;
   else if (size < 0) {
     PyErr_SetString (PyExc_IndexError,
                      "size cannot be negative, "
                      "except -1 to mean to the end of the buffer");
-    return NULL;
+    goto out;
   }
-  else if ((size_t) offset + size > buf->len) {
+  else if ((size_t) offset + size > buf.len) {
     PyErr_SetString (PyExc_IndexError, "size out of range");
-    return NULL;
+    goto out;
   }
-  if (!buf->initialized || is_zero (buf->data + offset, size))
-    Py_RETURN_TRUE;
+  if (!init || is_zero (buf.buf + offset, size))
+    ret = Py_True;
   else
-    Py_RETURN_FALSE;
+    ret = Py_False;
+ out:
+  PyBuffer_Release (&buf);
+  Py_XINCREF (ret);
+  return ret;
 }
Looks like a nice simplification!

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

    