Re: [Libguestfs] [libnbd PATCH 4/5] 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. Then, instead of using Py_{INC,DEC}REF across the paired aio_p{read,write} and corresponding completion function, we now rely on PyObject_GetBuffer and ByBuffer_Release. The use of memoryview protects us from the python user changing the buffer out from under our feet: $ ./run nbdsh nbd> h.connect_command(['nbdkit','-s','memory','10']) nbd> b = bytearray(10) nbd> b1 = nbd.Buffer.from_bytearray(b) nbd> h.aio_pread(b1, 0) 1 nbd> b.pop() Traceback (most recent call last): File "/usr/lib64/python3.10/code.py", line 90, in runcode exec(code, self.locals) File "<console>", line 1, in <module> BufferError: Existing exports of data: object cannot be re-sized nbd> h.poll(-1) 1 nbd> h.aio_command_completed(1) True nbd> b1 = None nbd> b.pop() 0 This ALSO means that we're doing less copying! now that nbd.Buffer.from_bytearray() is reusing an existing python object instead of copying to a C malloc'd buffer, we have noticeable effects on performance. For example, on my machine: $ export script=' m=1024*1024 size=h.get_size() zero=bytearray(m) for i in range(size // m): c = h.aio_pwrite(nbd.Buffer.from_bytearray(zero), m*i) while not h.aio_command_completed(c): h.poll(-1) ' $ time nbdkit -U - null 10G --run 'nbdsh -u $uri -c "$script"' takes 2.9s pre-patch, and 2.1s post-patch. I noticed that nbd.Buffer(-1) changes from: RuntimeError: length < 0 to SystemError: Negative size passed to PyByteArray_FromStringAndSize It would not be hard to revert that part, if needed.

Pure Python doesn't have any quick isinstance() test for whether an object is buffer-like (only C has that, in PyObject_CheckBuffer) [1]. That makes it interesting to preserve our pre-existing semantics (from the recent commit d477f7c7) where nbd.Buffer(int) gives uninitialized memory, but nbd.Buffer.from_bytearray(int) relies on the bytearray(int) constructor to give us initialized memory. For the constructor, I had to use C, but for .from_bytearray (which is now slightly misnamed, oh well), I chose to stick to a python try/except instead of deferring to a C helper function. [1] https://stackoverflow.com/questions/30017991/check-if-an-object-supports-... The generated code changes as follows: | --- python/methods.h.bak 2022-06-03 17:10:24.533842689 -0500 | +++ python/methods.h 2022-06-03 17:10:33.833858093 -0500 | @@ -28,16 +28,11 @@ | | #include <assert.h> | | -struct py_aio_buffer { | - Py_ssize_t len; | - void *data; | -}; | - | extern char **nbd_internal_py_get_string_list (PyObject *); | extern void nbd_internal_py_free_string_list (char **); | extern int nbd_internal_py_get_sockaddr (PyObject *, | struct sockaddr_storage *, socklen_t *); | -extern struct py_aio_buffer *nbd_internal_py_get_aio_buffer (PyObject *); | +extern PyObject *nbd_internal_py_get_aio_buffer (PyObject *); | extern PyObject *nbd_internal_py_get_nbd_buffer_type (void); | | static inline struct nbd_handle * | @@ -66,9 +61,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/methods.c.bak 2022-06-03 17:10:25.741844689 -0500 | +++ python/methods.c 2022-06-03 17:13:09.689116273 -0500 | @@ -37,7 +37,7 @@ | */ | struct user_data { | PyObject *fn; /* Optional pointer to Python function. */ | - PyObject *buf; /* Optional pointer to persistent buffer. */ | + Py_buffer view; /* persistent buffer, if view->obj set. */ | }; | | static struct user_data * | @@ -58,7 +58,7 @@ free_user_data (void *user_data) | | if (data) { | Py_XDECREF (data->fn); | - Py_XDECREF (data->buf); | + PyBuffer_Release(&data->view); | free (data); | } | } | @@ -3163,7 +3163,7 @@ nbd_internal_py_aio_pread (PyObject *sel | int64_t ret; | PyObject *py_ret = NULL; | PyObject *buf; /* instance of nbd.Buffer */ | - struct py_aio_buffer *buf_buf; /* Contents of nbd.Buffer */ | + PyObject *buf_buf; /* Contents of nbd.Buffer */ | uint64_t offset_u64; | unsigned long long offset; /* really uint64_t */ | struct user_data *completion_user_data = NULL; | @@ -3196,12 +3196,11 @@ nbd_internal_py_aio_pread (PyObject *sel | flags_u32 = flags; | buf_buf = nbd_internal_py_get_aio_buffer (buf); | if (!buf_buf) goto out; | - /* Increment refcount since buffer may be saved by libnbd. */ | - Py_INCREF (buf); | - completion_user_data->buf = buf; | + if (PyObject_GetBuffer(buf_buf, &completion_user_data->view, | + PyBUF_CONTIG) < 0) goto out; | offset_u64 = offset; | | - ret = nbd_aio_pread (h, buf_buf->data, buf_buf->len, offset_u64, completion, flags_u32); | + ret = nbd_aio_pread (h, completion_user_data->view.buf, completion_user_data->view.len, offset_u64, completion, flags_u32); | completion_user_data = NULL; | if (ret == -1) { | raise_exception (); | @@ -3210,6 +3209,7 @@ nbd_internal_py_aio_pread (PyObject *sel | py_ret = PyLong_FromLongLong (ret); | | out: | + Py_XDECREF (buf_buf); | free_user_data (completion_user_data); | return py_ret; | } | @@ -3222,7 +3222,7 @@ nbd_internal_py_aio_pread_structured (Py | int64_t ret; | PyObject *py_ret = NULL; | PyObject *buf; /* instance of nbd.Buffer */ | - struct py_aio_buffer *buf_buf; /* Contents of nbd.Buffer */ | + PyObject *buf_buf; /* Contents of nbd.Buffer */ | uint64_t offset_u64; | unsigned long long offset; /* really uint64_t */ | struct user_data *chunk_user_data = NULL; | @@ -3259,9 +3259,8 @@ nbd_internal_py_aio_pread_structured (Py | flags_u32 = flags; | buf_buf = nbd_internal_py_get_aio_buffer (buf); | if (!buf_buf) goto out; | - /* Increment refcount since buffer may be saved by libnbd. */ | - Py_INCREF (buf); | - completion_user_data->buf = buf; | + if (PyObject_GetBuffer(buf_buf, &completion_user_data->view, | + PyBUF_CONTIG) < 0) goto out; | offset_u64 = offset; | chunk.user_data = chunk_user_data = alloc_user_data (); | if (chunk_user_data == NULL) goto out; | @@ -3274,7 +3273,7 @@ nbd_internal_py_aio_pread_structured (Py | Py_INCREF (py_chunk_fn); | chunk_user_data->fn = py_chunk_fn; | | - ret = nbd_aio_pread_structured (h, buf_buf->data, buf_buf->len, offset_u64, chunk, completion, flags_u32); | + ret = nbd_aio_pread_structured (h, completion_user_data->view.buf, completion_user_data->view.len, offset_u64, chunk, completion, flags_u32);

| chunk_user_data = NULL; | completion_user_data = NULL; | if (ret == -1) { | @@ -3284,6 +3283,7 @@ nbd_internal_py_aio_pread_structured (Py | py_ret = PyLong_FromLongLong (ret); | | out: | + Py_XDECREF (buf_buf); | free_user_data (chunk_user_data); | free_user_data (completion_user_data); | return py_ret; | @@ -3297,7 +3297,7 @@ nbd_internal_py_aio_pwrite (PyObject *se | int64_t ret; | PyObject *py_ret = NULL; | PyObject *buf; /* instance of nbd.Buffer */ | - struct py_aio_buffer *buf_buf; /* Contents of nbd.Buffer */ | + PyObject *buf_buf; /* Contents of nbd.Buffer */ | uint64_t offset_u64; | unsigned long long offset; /* really uint64_t */ | struct user_data *completion_user_data = NULL; | @@ -3330,12 +3330,11 @@ nbd_internal_py_aio_pwrite (PyObject *se | flags_u32 = flags; | buf_buf = nbd_internal_py_get_aio_buffer (buf); | if (!buf_buf) goto out; | - /* Increment refcount since buffer may be saved by libnbd. */ | - Py_INCREF (buf); | - completion_user_data->buf = buf; | + if (PyObject_GetBuffer(buf_buf, &completion_user_data->view, | + PyBUF_CONTIG_RO) < 0) goto out; | offset_u64 = offset; | | - ret = nbd_aio_pwrite (h, buf_buf->data, buf_buf->len, offset_u64, completion, flags_u32); | + ret = nbd_aio_pwrite (h, completion_user_data->view.buf, completion_user_data->view.len, offset_u64, completion, flags_u32); | completion_user_data = NULL; | if (ret == -1) { | raise_exception (); | @@ -3344,6 +3343,7 @@ nbd_internal_py_aio_pwrite (PyObject *se | py_ret = PyLong_FromLongLong (ret); | | out: | + Py_XDECREF (buf_buf); | free_user_data (completion_user_data); | return py_ret; | } | --- python/nbd.py.bak 2022-06-03 17:10:23.350840729 -0500 | +++ python/nbd.py 2022-06-03 17:10:33.852858124 -0500 | @@ -128,19 +128,21 @@ class Buffer(object): | | @classmethod | def from_bytearray(cls, ba): | - '''create an AIO buffer from a bytearray''' | - o = libnbdmod.aio_buffer_from_bytearray(ba) | + '''create an AIO buffer from a bytearray or other buffer''' | self = cls(0) | - self._o = o | + try: | + self._o = memoryview(ba).cast('B') | + except TypeError: | + self._o = bytearray(ba) | return self | | def to_bytearray(self): | '''copy an AIO buffer into a bytearray''' | - return libnbdmod.aio_buffer_to_bytearray(self) | + 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): | ''' | @@ -154,7 +156,7 @@ 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) | | | class NBD(object): --- generator/Python.ml | 52 ++++++------ python/handle.c | 188 +++++++------------------------------------- 2 files changed, 52 insertions(+), 188 deletions(-)

diff --git a/generator/Python.ml b/generator/Python.ml index b862b44..270858f 100644 --- a/generator/Python.ml +++ b/generator/Python.ml @@ -34,16 +34,11 @@ let pr "#include <assert.h>\n"; pr "\n"; pr "\ -struct py_aio_buffer { - Py_ssize_t len; - void *data; -}; - extern char **nbd_internal_py_get_string_list (PyObject *); extern void nbd_internal_py_free_string_list (char **); extern int nbd_internal_py_get_sockaddr (PyObject *, struct sockaddr_storage *, socklen_t *); -extern struct py_aio_buffer *nbd_internal_py_get_aio_buffer (PyObject *); +extern PyObject *nbd_internal_py_get_aio_buffer (PyObject *); extern PyObject *nbd_internal_py_get_nbd_buffer_type (void); static inline struct nbd_handle * @@ -77,9 +72,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"; @@ -109,9 +101,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"; @@ -301,7 +290,7 @@ let | BytesPersistIn (n, _) | BytesPersistOut (n, _) -> pr " PyObject *%s; /* instance of nbd.Buffer */\n" n; - pr " struct py_aio_buffer *%s_buf; /* Contents of nbd.Buffer */\n" n + pr " PyObject *%s_buf; /* Contents of nbd.Buffer */\n" n | Closure { cbname } -> pr " struct user_data *%s_user_data = NULL;\n" cbname; pr " PyObject *py_%s_fn;\n" cbname; @@ -436,12 +425,16 @@ let | BytesOut (n, count) -> pr " %s = PyByteArray_FromStringAndSize (NULL, %s);\n" n count; pr " if (%s == NULL) goto out;\n" n - | BytesPersistIn (n, _) | BytesPersistOut (n, _) -> + | BytesPersistIn (n, _) -> pr " %s_buf = nbd_internal_py_get_aio_buffer (%s);\n" n n; pr " if (!%s_buf) goto out;\n" n; - pr " /* Increment refcount since buffer may be saved by libnbd. */\n"; - pr " Py_INCREF (%s);\n" n; - pr " completion_user_data->buf = %s;\n" n + pr " if (PyObject_GetBuffer(%s_buf, &completion_user_data->view,\n" n; + pr " PyBUF_CONTIG_RO) < 0) goto out;\n" + | BytesPersistOut (n, _) -> + pr " %s_buf = nbd_internal_py_get_aio_buffer (%s);\n" n n; + pr " if (!%s_buf) goto out;\n" n; + pr " if (PyObject_GetBuffer(%s_buf, &completion_user_data->view,\n" n; + pr " PyBUF_CONTIG) < 0) goto out;\n" | Closure { cbname } -> pr " %s.user_data = %s_user_data = alloc_user_data ();\n" cbname cbname; pr " if (%s_user_data == NULL) goto out;\n" cbname; @@ -482,8 +475,8 @@ let | Bool n -> pr ", %s" n | BytesIn (n, _) -> pr ", %s.buf, %s.len" n n | BytesOut (n, count) -> pr ", PyByteArray_AS_STRING (%s), %s" n count - | BytesPersistIn (n, _) - | BytesPersistOut (n, _) -> pr ", %s_buf->data, %s_buf->len" n n + | BytesPersistIn _ | BytesPersistOut _ -> + pr ", completion_user_data->view.buf, completion_user_data->view.len" | Closure { cbname } -> pr ", %s" cbname | Enum (n, _) -> pr ", %s" n | Flags (n, _) -> pr ", %s_u32" n @@ -576,7 +569,8 @@ let pr " if (%s.obj)\n" n; pr " PyBuffer_Release (&%s);\n" n | BytesOut (n, _) -> pr " Py_XDECREF (%s);\n" n - | BytesPersistIn _ | BytesPersistOut _ -> () + | BytesPersistIn (n, _) | BytesPersistOut (n, _) -> + pr " Py_XDECREF (%s_buf);\n" n | Closure { cbname } -> pr " free_user_data (%s_user_data);\n" cbname | Enum _ -> () @@ -625,7 +619,7 @@ let pr " */\n"; pr "struct user_data {\n"; pr " PyObject *fn; /* Optional pointer to Python function. */\n"; - pr " PyObject *buf; /* Optional pointer to persistent buffer. */\n"; + pr " Py_buffer view; /* persistent buffer, if view->obj set. */\n"; pr "};\n"; pr "\n"; pr "static struct user_data *\n"; @@ -646,7 +640,7 @@ let pr "\n"; pr " if (data) {\n"; pr " Py_XDECREF (data->fn);\n"; - pr " Py_XDECREF (data->buf);\n"; + pr " PyBuffer_Release(&data->view);\n"; pr " free (data);\n"; pr " }\n"; pr "}\n"; @@ -768,19 +762,21 @@ let @classmethod def from_bytearray(cls, ba): - '''create an AIO buffer from a bytearray''' - o = libnbdmod.aio_buffer_from_bytearray(ba) + '''create an AIO buffer from a bytearray or other buffer''' self = cls(0) - self._o = o + try: + self._o = memoryview(ba).cast('B') + except TypeError: + self._o = bytearray(ba) return self def to_bytearray(self): '''copy an AIO buffer into a bytearray''' - return libnbdmod.aio_buffer_to_bytearray(self) + 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): ''' @@ -794,7 +790,7 @@ 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) class NBD(object): diff --git a/python/handle.c b/python/handle.c index f84c6e0..7f67159 100644 --- a/python/handle.c +++ b/python/handle.c @@ -98,205 +98,73 @@ nbd_internal_py_display_version (PyObject *self, PyObject *args) static const char aio_buffer_name[] = "nbd.Buffer"; -struct py_aio_buffer * +PyObject * nbd_internal_py_get_aio_buffer (PyObject *buffer) { - if (PyObject_IsInstance (buffer, nbd_internal_py_get_nbd_buffer_type ())) { - PyObject *capsule = PyObject_GetAttrString(buffer, "_o"); - return PyCapsule_GetPointer (capsule, aio_buffer_name); - } + if (PyObject_IsInstance (buffer, nbd_internal_py_get_nbd_buffer_type ())) + return PyObject_GetAttrString(buffer, "_o"); PyErr_SetString (PyExc_TypeError, "aio_buffer: expecting nbd.Buffer instance"); return NULL; } -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. */ PyObject * nbd_internal_py_alloc_aio_buffer (PyObject *self, PyObject *args) { - struct py_aio_buffer *buf; - PyObject *ret; - - buf = malloc (sizeof *buf); - if (buf == NULL) { - PyErr_NoMemory (); - return NULL; - } - - if (!PyArg_ParseTuple (args, (char *) "n:nbd_internal_py_alloc_aio_buffer", - &buf->len)) { - free (buf); - 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, - (char *) "O:nbd_internal_py_aio_buffer_from_bytearray", - &obj)) + if (!PyArg_ParseTuple (args, (char *) "n:nbd_internal_py_alloc_aio_buffer", + &len)) 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); - - 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; - struct py_aio_buffer *buf; - - if (!PyArg_ParseTuple (args, - (char *) "O:nbd_internal_py_aio_buffer_to_bytearray", - &obj)) - return NULL; - - buf = nbd_internal_py_get_aio_buffer (obj); - if (buf == NULL) - return NULL; - - return PyByteArray_FromStringAndSize (buf->data, buf->len); -} - -PyObject * -nbd_internal_py_aio_buffer_size (PyObject *self, PyObject *args) -{ - PyObject *obj; - struct py_aio_buffer *buf; - - if (!PyArg_ParseTuple (args, - (char *) "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; + PyObject *ret = NULL; if (!PyArg_ParseTuple (args, - (char *) "Onn:nbd_internal_py_aio_buffer_is_zero", - &obj, &offset, &size)) + (char *) "y*nn:nbd_internal_py_aio_buffer_is_zero", + &buf, &offset, &size)) 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; + Py_INCREF (ret); + 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 (is_zero (buf->data + offset, size)) - Py_RETURN_TRUE; - else - Py_RETURN_FALSE; + ret = PyBool_FromLong (is_zero (buf.buf + offset, size)); + out: + PyBuffer_Release(&buf); + return ret; } -- 2.36.1 _______________________________________________ Libguestfs mailing list Libguestfs(a)redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs