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

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; }