8:34 a.m.
Less copying is always better. But I was quite surprised by some of
my test cases in trying to prove that I had speedups; there's a huge
difference between:
for i in range(size // m):
buf = h.pread_structured(m, m*i, f)
and
for i in range(size // m):
buf = h.pread_structured(m, m*i, f)
buf = None
The former takes around 4.5s with libnbd 1.12.3, the latter around
15s, even though I was expecting the latter to be faster. But with
more thought, I think what is happening is that Python's compilation
and garbage collection implementation looks for opportunities where it
can prove that a pool of allocated memory can be easily reused
(assigning buf to hold a new bytearray that occupies the same size as
the previous bytearray that is now down to zero references) vs. where
it allocates new memory (the intermediate state of buf going to None
throwing away the intermediate knowledge about relations between its
other values). But with either python script, this patch makes a
noticeable difference by exposing the subbuf in [aio_]pread_structured
as a slice of the original python object, rather than yet another
memory copy.
Eric Blake (5):
python: Simplify passing of mutable *error to callbacks
python: Alter lock for persistent buffer
python: Accept all buffer-like objects in aio_p{read,write}
python: Support len(nbd.Buffer(n))
python: Slice structured read callback buffer from original
generator/Python.ml | 54 ++++++++---------
python/handle.c | 19 ++++--
python/t/405-pread-structured.py | 95 +++++++++---------------------
python/t/500-aio-pread.py | 21 +++++++
python/t/505-aio-pread-callback.py | 93 ++++++++---------------------
python/t/510-aio-pwrite.py | 17 ++++++
python/t/580-aio-is-zero.py | 2 +
python/utils.c | 53 +++++++++++++++++
8 files changed, 186 insertions(+), 168 deletions(-)
--
2.36.1
8:34 a.m.
New subject: [libnbd PATCH v3 1/5] python: Simplify passing of mutable *error to callbacks
Instead of open-coding our code to create a PyObject wrapper of the
mutable *error to be passed to each callback, extract this into a
helper function. We can then slightly optimize things to hang on to a
single pointer to the ctypes module, rather than looking it up on
every callback. The generated code shrinks by more than the added
code in utils.c:
| --- python/methods.h.bak 2022-06-08 14:35:00.454786844 -0500
| +++ python/methods.h 2022-06-08 14:43:24.681596853 -0500
| @@ -35,6 +35,7 @@
| extern PyObject *nbd_internal_py_get_aio_view (PyObject *, bool);
| extern int nbd_internal_py_init_aio_buffer (PyObject *);
| extern PyObject *nbd_internal_py_get_nbd_buffer_type (void);
| +extern PyObject *nbd_internal_py_wrap_errptr (int);
|
| static inline struct nbd_handle *
| get_handle (PyObject *obj)
| --- python/methods.c.bak 2022-06-08 14:35:00.450786838 -0500
| +++ python/methods.c 2022-06-08 14:43:24.696596877 -0500
| @@ -75,13 +75,7 @@ chunk_wrapper (void *user_data, const vo
| PyObject *py_args, *py_ret;
| PyObject *py_error = NULL;
|
| - PyObject *py_error_modname = PyUnicode_FromString ("ctypes");
| - if (!py_error_modname) { PyErr_PrintEx (0); goto out; }
| - PyObject *py_error_mod = PyImport_Import (py_error_modname);
| - Py_DECREF (py_error_modname);
| - if (!py_error_mod) { PyErr_PrintEx (0); goto out; }
| - py_error = PyObject_CallMethod (py_error_mod, "c_int", "i",
*error);
| - Py_DECREF (py_error_mod);
| + py_error = nbd_internal_py_wrap_errptr (*error);
| if (!py_error) { PyErr_PrintEx (0); goto out; }
|
| py_args = Py_BuildValue ("(y#KIO)", subbuf, (int) count, offset, status,
| @@ -132,13 +126,7 @@ completion_wrapper (void *user_data, int
| PyObject *py_args, *py_ret;
| PyObject *py_error = NULL;
|
| - PyObject *py_error_modname = PyUnicode_FromString ("ctypes");
| - if (!py_error_modname) { PyErr_PrintEx (0); goto out; }
| - PyObject *py_error_mod = PyImport_Import (py_error_modname);
| - Py_DECREF (py_error_modname);
| - if (!py_error_mod) { PyErr_PrintEx (0); goto out; }
| - py_error = PyObject_CallMethod (py_error_mod, "c_int", "i",
*error);
| - Py_DECREF (py_error_mod);
| + py_error = nbd_internal_py_wrap_errptr (*error);
| if (!py_error) { PyErr_PrintEx (0); goto out; }
|
| py_args = Py_BuildValue ("(O)", py_error);
| @@ -239,13 +227,7 @@ extent_wrapper (void *user_data, const c
| if (!py_e_entries) { PyErr_PrintEx (0); goto out; }
| PyList_SET_ITEM (py_entries, i_entries, py_e_entries);
| }
| - PyObject *py_error_modname = PyUnicode_FromString ("ctypes");
| - if (!py_error_modname) { PyErr_PrintEx (0); goto out; }
| - PyObject *py_error_mod = PyImport_Import (py_error_modname);
| - Py_DECREF (py_error_modname);
| - if (!py_error_mod) { PyErr_PrintEx (0); goto out; }
| - py_error = PyObject_CallMethod (py_error_mod, "c_int", "i",
*error);
| - Py_DECREF (py_error_mod);
| + py_error = nbd_internal_py_wrap_errptr (*error);
| if (!py_error) { PyErr_PrintEx (0); goto out; }
|
| py_args = Py_BuildValue ("(sKOO)", metacontext, offset, py_entries,
---
generator/Python.ml | 9 ++-------
python/utils.c | 19 +++++++++++++++++++
2 files changed, 21 insertions(+), 7 deletions(-)
diff --git a/generator/Python.ml b/generator/Python.ml
index e94f37b..6980034 100644
--- a/generator/Python.ml
+++ b/generator/Python.ml
@@ -41,6 +41,7 @@ let
extern PyObject *nbd_internal_py_get_aio_view (PyObject *, bool);
extern int nbd_internal_py_init_aio_buffer (PyObject *);
extern PyObject *nbd_internal_py_get_nbd_buffer_type (void);
+extern PyObject *nbd_internal_py_wrap_errptr (int);
static inline struct nbd_handle *
get_handle (PyObject *obj)
@@ -184,13 +185,7 @@ let
| CBInt _
| CBInt64 _ -> ()
| CBMutable (Int n) ->
- pr " PyObject *py_%s_modname = PyUnicode_FromString
(\"ctypes\");\n" n;
- pr " if (!py_%s_modname) { PyErr_PrintEx (0); goto out; }\n" n;
- pr " PyObject *py_%s_mod = PyImport_Import (py_%s_modname);\n" n n;
- pr " Py_DECREF (py_%s_modname);\n" n;
- pr " if (!py_%s_mod) { PyErr_PrintEx (0); goto out; }\n" n;
- pr " py_%s = PyObject_CallMethod (py_%s_mod, \"c_int\",
\"i\", *%s);\n" n n n;
- pr " Py_DECREF (py_%s_mod);\n" n;
+ pr " py_%s = nbd_internal_py_wrap_errptr (*%s);\n" n n;
pr " if (!py_%s) { PyErr_PrintEx (0); goto out; }\n" n;
| CBString _
| CBUInt _
diff --git a/python/utils.c b/python/utils.c
index e0df181..cd44b90 100644
--- a/python/utils.c
+++ b/python/utils.c
@@ -173,3 +173,22 @@ nbd_internal_py_get_nbd_buffer_type (void)
}
return type;
}
+
+/* Helper to package callback *error into modifiable PyObject */
+PyObject *
+nbd_internal_py_wrap_errptr (int err)
+{
+ static PyObject *py_ctypes_mod;
+
+ if (!py_ctypes_mod) {
+ PyObject *py_modname = PyUnicode_FromString ("ctypes");
+ if (!py_modname)
+ return NULL;
+ py_ctypes_mod = PyImport_Import (py_modname);
+ Py_DECREF (py_modname);
+ if (!py_ctypes_mod)
+ return NULL;
+ }
+
+ return PyObject_CallMethod (py_ctypes_mod, "c_int", "i", err);
+}
--
2.36.1
9:22 a.m.
New subject: [libnbd PATCH v3 1/5] python: Simplify passing of mutable *error to callbacks
On Thu, Jun 09, 2022 at 08:34:43AM -0500, Eric Blake wrote:
Instead of open-coding our code to create a PyObject wrapper of the
mutable *error to be passed to each callback, extract this into a
helper function. We can then slightly optimize things to hang on to a
single pointer to the ctypes module, rather than looking it up on
every callback. The generated code shrinks by more than the added
code in utils.c:
| --- python/methods.h.bak 2022-06-08 14:35:00.454786844 -0500
| +++ python/methods.h 2022-06-08 14:43:24.681596853 -0500
| @@ -35,6 +35,7 @@
| extern PyObject *nbd_internal_py_get_aio_view (PyObject *, bool);
| extern int nbd_internal_py_init_aio_buffer (PyObject *);
| extern PyObject *nbd_internal_py_get_nbd_buffer_type (void);
| +extern PyObject *nbd_internal_py_wrap_errptr (int);
|
| static inline struct nbd_handle *
| get_handle (PyObject *obj)
| --- python/methods.c.bak 2022-06-08 14:35:00.450786838 -0500
| +++ python/methods.c 2022-06-08 14:43:24.696596877 -0500
| @@ -75,13 +75,7 @@ chunk_wrapper (void *user_data, const vo
| PyObject *py_args, *py_ret;
| PyObject *py_error = NULL;
|
| - PyObject *py_error_modname = PyUnicode_FromString ("ctypes");
| - if (!py_error_modname) { PyErr_PrintEx (0); goto out; }
| - PyObject *py_error_mod = PyImport_Import (py_error_modname);
| - Py_DECREF (py_error_modname);
| - if (!py_error_mod) { PyErr_PrintEx (0); goto out; }
| - py_error = PyObject_CallMethod (py_error_mod, "c_int", "i",
*error);
| - Py_DECREF (py_error_mod);
| + py_error = nbd_internal_py_wrap_errptr (*error);
| if (!py_error) { PyErr_PrintEx (0); goto out; }
|
| py_args = Py_BuildValue ("(y#KIO)", subbuf, (int) count, offset, status,
| @@ -132,13 +126,7 @@ completion_wrapper (void *user_data, int
| PyObject *py_args, *py_ret;
| PyObject *py_error = NULL;
|
| - PyObject *py_error_modname = PyUnicode_FromString ("ctypes");
| - if (!py_error_modname) { PyErr_PrintEx (0); goto out; }
| - PyObject *py_error_mod = PyImport_Import (py_error_modname);
| - Py_DECREF (py_error_modname);
| - if (!py_error_mod) { PyErr_PrintEx (0); goto out; }
| - py_error = PyObject_CallMethod (py_error_mod, "c_int", "i",
*error);
| - Py_DECREF (py_error_mod);
| + py_error = nbd_internal_py_wrap_errptr (*error);
| if (!py_error) { PyErr_PrintEx (0); goto out; }
|
| py_args = Py_BuildValue ("(O)", py_error);
| @@ -239,13 +227,7 @@ extent_wrapper (void *user_data, const c
| if (!py_e_entries) { PyErr_PrintEx (0); goto out; }
| PyList_SET_ITEM (py_entries, i_entries, py_e_entries);
| }
| - PyObject *py_error_modname = PyUnicode_FromString ("ctypes");
| - if (!py_error_modname) { PyErr_PrintEx (0); goto out; }
| - PyObject *py_error_mod = PyImport_Import (py_error_modname);
| - Py_DECREF (py_error_modname);
| - if (!py_error_mod) { PyErr_PrintEx (0); goto out; }
| - py_error = PyObject_CallMethod (py_error_mod, "c_int", "i",
*error);
| - Py_DECREF (py_error_mod);
| + py_error = nbd_internal_py_wrap_errptr (*error);
| if (!py_error) { PyErr_PrintEx (0); goto out; }
|
| py_args = Py_BuildValue ("(sKOO)", metacontext, offset, py_entries,
---
generator/Python.ml | 9 ++-------
python/utils.c | 19 +++++++++++++++++++
2 files changed, 21 insertions(+), 7 deletions(-)
diff --git a/generator/Python.ml b/generator/Python.ml
index e94f37b..6980034 100644
--- a/generator/Python.ml
+++ b/generator/Python.ml
@@ -41,6 +41,7 @@ let
extern PyObject *nbd_internal_py_get_aio_view (PyObject *, bool);
extern int nbd_internal_py_init_aio_buffer (PyObject *);
extern PyObject *nbd_internal_py_get_nbd_buffer_type (void);
+extern PyObject *nbd_internal_py_wrap_errptr (int);
static inline struct nbd_handle *
get_handle (PyObject *obj)
@@ -184,13 +185,7 @@ let
| CBInt _
| CBInt64 _ -> ()
| CBMutable (Int n) ->
- pr " PyObject *py_%s_modname = PyUnicode_FromString
(\"ctypes\");\n" n;
- pr " if (!py_%s_modname) { PyErr_PrintEx (0); goto out; }\n" n;
- pr " PyObject *py_%s_mod = PyImport_Import (py_%s_modname);\n" n n;
- pr " Py_DECREF (py_%s_modname);\n" n;
- pr " if (!py_%s_mod) { PyErr_PrintEx (0); goto out; }\n" n;
- pr " py_%s = PyObject_CallMethod (py_%s_mod, \"c_int\",
\"i\", *%s);\n" n n n;
- pr " Py_DECREF (py_%s_mod);\n" n;
+ pr " py_%s = nbd_internal_py_wrap_errptr (*%s);\n" n n;
pr " if (!py_%s) { PyErr_PrintEx (0); goto out; }\n" n;
| CBString _
| CBUInt _
diff --git a/python/utils.c b/python/utils.c
index e0df181..cd44b90 100644
--- a/python/utils.c
+++ b/python/utils.c
@@ -173,3 +173,22 @@ nbd_internal_py_get_nbd_buffer_type (void)
}
return type;
}
+
+/* Helper to package callback *error into modifiable PyObject */
+PyObject *
+nbd_internal_py_wrap_errptr (int err)
+{
+ static PyObject *py_ctypes_mod;
+
+ if (!py_ctypes_mod) {
+ PyObject *py_modname = PyUnicode_FromString ("ctypes");
+ if (!py_modname)
+ return NULL;
+ py_ctypes_mod = PyImport_Import (py_modname);
+ Py_DECREF (py_modname);
+ if (!py_ctypes_mod)
+ return NULL;
+ }
+
+ return PyObject_CallMethod (py_ctypes_mod, "c_int", "i", err);
+}
I guess because of the Python GIL we don't need locking here?
We don't run the Python tests with valgrinding so there should be no
issue there.
Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com>
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines. Supports shell scripting,
bindings from many languages. http://libguestfs.org
11:03 a.m.
New subject: [libnbd PATCH v3 1/5] python: Simplify passing of mutable *error to callbacks
On Thu, Jun 09, 2022 at 03:22:42PM +0100, Richard W.M. Jones wrote:
On Thu, Jun 09, 2022 at 08:34:43AM -0500, Eric Blake wrote:
> Instead of open-coding our code to create a PyObject wrapper of the
> mutable *error to be passed to each callback, extract this into a
> helper function. We can then slightly optimize things to hang on to a
> single pointer to the ctypes module, rather than looking it up on
> every callback. The generated code shrinks by more than the added
> code in utils.c:
>
> +++ b/generator/Python.ml
> @@ -41,6 +41,7 @@ let
> extern PyObject *nbd_internal_py_get_aio_view (PyObject *, bool);
> extern int nbd_internal_py_init_aio_buffer (PyObject *);
> extern PyObject *nbd_internal_py_get_nbd_buffer_type (void);
> +extern PyObject *nbd_internal_py_wrap_errptr (int);
>
> static inline struct nbd_handle *
> get_handle (PyObject *obj)
> @@ -184,13 +185,7 @@ let
> | CBInt _
> | CBInt64 _ -> ()
> | CBMutable (Int n) ->
> - pr " PyObject *py_%s_modname = PyUnicode_FromString
(\"ctypes\");\n" n;
> - pr " if (!py_%s_modname) { PyErr_PrintEx (0); goto out; }\n" n;
> - pr " PyObject *py_%s_mod = PyImport_Import (py_%s_modname);\n" n
n;
> - pr " Py_DECREF (py_%s_modname);\n" n;
> - pr " if (!py_%s_mod) { PyErr_PrintEx (0); goto out; }\n" n;
> - pr " py_%s = PyObject_CallMethod (py_%s_mod, \"c_int\",
\"i\", *%s);\n" n n n;
> - pr " Py_DECREF (py_%s_mod);\n" n;
The old code was using PyObject_CallMethod outside the GIL lock...
> + pr " py_%s = nbd_internal_py_wrap_errptr
(*%s);\n" n n;
> pr " if (!py_%s) { PyErr_PrintEx (0); goto out; }\n" n;
> | CBString _
> | CBUInt _
> diff --git a/python/utils.c b/python/utils.c
> index e0df181..cd44b90 100644
> --- a/python/utils.c
> +++ b/python/utils.c
> @@ -173,3 +173,22 @@ nbd_internal_py_get_nbd_buffer_type (void)
> }
> return type;
> }
> +
> +/* Helper to package callback *error into modifiable PyObject */
> +PyObject *
> +nbd_internal_py_wrap_errptr (int err)
> +{
> + static PyObject *py_ctypes_mod;
> +
> + if (!py_ctypes_mod) {
> + PyObject *py_modname = PyUnicode_FromString ("ctypes");
> + if (!py_modname)
> + return NULL;
> + py_ctypes_mod = PyImport_Import (py_modname);
> + Py_DECREF (py_modname);
> + if (!py_ctypes_mod)
> + return NULL;
...the new code as well.
> + }
> +
> + return PyObject_CallMethod (py_ctypes_mod, "c_int", "i",
err);
> +}
I guess because of the Python GIL we don't need locking here?
But you're right that any time we call into the python interpreter for
something that may entail quite some level of complexity, the GIL lock
needs to be worried about. I'll have to audit this and other recent
patches to see if we need to add more use of
PyGILState_{Ensure/Release} around various code blocks (after first
researching what the lock is supposed to do...).
We don't run the Python tests with valgrinding so there should be no
issue there.
It's a reference we never release (same thing done in recent commit
9c5b0eab) - if valgrind can see that static storage is still
reachable, rather than classing it as a leak, then even if we do run
under valgrind we'd be okay. If not, it's provable this is not a
long-term leak, so we could add in a suppression rule.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
3:58 p.m.
New subject: [libnbd PATCH v3 1/5] python: Simplify passing of mutable *error to callbacks
On Thu, Jun 09, 2022 at 11:03:02AM -0500, Eric Blake wrote:
On Thu, Jun 09, 2022 at 03:22:42PM +0100, Richard W.M. Jones wrote:
> On Thu, Jun 09, 2022 at 08:34:43AM -0500, Eric Blake wrote:
> > Instead of open-coding our code to create a PyObject wrapper of the
> > mutable *error to be passed to each callback, extract this into a
> > helper function. We can then slightly optimize things to hang on to a
> > single pointer to the ctypes module, rather than looking it up on
> > every callback. The generated code shrinks by more than the added
> > code in utils.c:
> >
> > +++ b/generator/Python.ml
> > @@ -41,6 +41,7 @@ let
> > extern PyObject *nbd_internal_py_get_aio_view (PyObject *, bool);
> > extern int nbd_internal_py_init_aio_buffer (PyObject *);
> > extern PyObject *nbd_internal_py_get_nbd_buffer_type (void);
> > +extern PyObject *nbd_internal_py_wrap_errptr (int);
> >
> > static inline struct nbd_handle *
> > get_handle (PyObject *obj)
> > @@ -184,13 +185,7 @@ let
> > | CBInt _
> > | CBInt64 _ -> ()
> > | CBMutable (Int n) ->
> > - pr " PyObject *py_%s_modname = PyUnicode_FromString
(\"ctypes\");\n" n;
> > - pr " if (!py_%s_modname) { PyErr_PrintEx (0); goto out; }\n"
n;
> > - pr " PyObject *py_%s_mod = PyImport_Import
(py_%s_modname);\n" n n;
> > - pr " Py_DECREF (py_%s_modname);\n" n;
> > - pr " if (!py_%s_mod) { PyErr_PrintEx (0); goto out; }\n" n;
> > - pr " py_%s = PyObject_CallMethod (py_%s_mod, \"c_int\",
\"i\", *%s);\n" n n n;
> > - pr " Py_DECREF (py_%s_mod);\n" n;
The old code was using PyObject_CallMethod outside the GIL lock...
> > + pr " py_%s = nbd_internal_py_wrap_errptr (*%s);\n" n n;
> > pr " if (!py_%s) { PyErr_PrintEx (0); goto out; }\n" n;
> > | CBString _
> > | CBUInt _
> > diff --git a/python/utils.c b/python/utils.c
> > index e0df181..cd44b90 100644
> > --- a/python/utils.c
> > +++ b/python/utils.c
> > @@ -173,3 +173,22 @@ nbd_internal_py_get_nbd_buffer_type (void)
> > }
> > return type;
> > }
> > +
> > +/* Helper to package callback *error into modifiable PyObject */
> > +PyObject *
> > +nbd_internal_py_wrap_errptr (int err)
> > +{
> > + static PyObject *py_ctypes_mod;
> > +
> > + if (!py_ctypes_mod) {
> > + PyObject *py_modname = PyUnicode_FromString ("ctypes");
> > + if (!py_modname)
> > + return NULL;
> > + py_ctypes_mod = PyImport_Import (py_modname);
> > + Py_DECREF (py_modname);
> > + if (!py_ctypes_mod)
> > + return NULL;
...the new code as well.
> > + }
> > +
> > + return PyObject_CallMethod (py_ctypes_mod, "c_int", "i",
err);
> > +}
>
> I guess because of the Python GIL we don't need locking here?
But you're right that any time we call into the python interpreter for
something that may entail quite some level of complexity, the GIL lock
needs to be worried about. I'll have to audit this and other recent
patches to see if we need to add more use of
PyGILState_{Ensure/Release} around various code blocks (after first
researching what the lock is supposed to do...).
Okay, after reading more about the GIL, it looks like we are (likely)
safe only because we have never really been multi-threaded: none of
our libnbd code uses Py_BEGIN_ALLOW_THREADS to release the GIL around
our calls into libnbd C code, and therefore even though our placement
of PyGILState_Ensure() was too late, all of our callbacks have been
reached from places where we already had the GIL (well, hopefully
that's true). At any rate, I'm working on a patch to explicitly
release the GIL around calls into libnbd C code, which means we DO
need to fix our callback wrappers to grab it at the right point. But
the code in utils.c is indeed safe because of the GIL.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
4:04 p.m.
New subject: [libnbd PATCH v3 1/5] python: Simplify passing of mutable *error to callbacks
On Thu, Jun 09, 2022 at 03:58:45PM -0500, Eric Blake wrote:
On Thu, Jun 09, 2022 at 11:03:02AM -0500, Eric Blake wrote:
> On Thu, Jun 09, 2022 at 03:22:42PM +0100, Richard W.M. Jones wrote:
> > On Thu, Jun 09, 2022 at 08:34:43AM -0500, Eric Blake wrote:
> > > Instead of open-coding our code to create a PyObject wrapper of the
> > > mutable *error to be passed to each callback, extract this into a
> > > helper function. We can then slightly optimize things to hang on to a
> > > single pointer to the ctypes module, rather than looking it up on
> > > every callback. The generated code shrinks by more than the added
> > > code in utils.c:
> > >
>
> > > +++ b/generator/Python.ml
> > > @@ -41,6 +41,7 @@ let
> > > extern PyObject *nbd_internal_py_get_aio_view (PyObject *, bool);
> > > extern int nbd_internal_py_init_aio_buffer (PyObject *);
> > > extern PyObject *nbd_internal_py_get_nbd_buffer_type (void);
> > > +extern PyObject *nbd_internal_py_wrap_errptr (int);
> > >
> > > static inline struct nbd_handle *
> > > get_handle (PyObject *obj)
> > > @@ -184,13 +185,7 @@ let
> > > | CBInt _
> > > | CBInt64 _ -> ()
> > > | CBMutable (Int n) ->
> > > - pr " PyObject *py_%s_modname = PyUnicode_FromString
(\"ctypes\");\n" n;
> > > - pr " if (!py_%s_modname) { PyErr_PrintEx (0); goto out;
}\n" n;
> > > - pr " PyObject *py_%s_mod = PyImport_Import
(py_%s_modname);\n" n n;
> > > - pr " Py_DECREF (py_%s_modname);\n" n;
> > > - pr " if (!py_%s_mod) { PyErr_PrintEx (0); goto out;
}\n" n;
> > > - pr " py_%s = PyObject_CallMethod (py_%s_mod,
\"c_int\", \"i\", *%s);\n" n n n;
> > > - pr " Py_DECREF (py_%s_mod);\n" n;
>
> The old code was using PyObject_CallMethod outside the GIL lock...
>
> > > + pr " py_%s = nbd_internal_py_wrap_errptr (*%s);\n" n
n;
> > > pr " if (!py_%s) { PyErr_PrintEx (0); goto out; }\n"
n;
> > > | CBString _
> > > | CBUInt _
> > > diff --git a/python/utils.c b/python/utils.c
> > > index e0df181..cd44b90 100644
> > > --- a/python/utils.c
> > > +++ b/python/utils.c
> > > @@ -173,3 +173,22 @@ nbd_internal_py_get_nbd_buffer_type (void)
> > > }
> > > return type;
> > > }
> > > +
> > > +/* Helper to package callback *error into modifiable PyObject */
> > > +PyObject *
> > > +nbd_internal_py_wrap_errptr (int err)
> > > +{
> > > + static PyObject *py_ctypes_mod;
> > > +
> > > + if (!py_ctypes_mod) {
> > > + PyObject *py_modname = PyUnicode_FromString ("ctypes");
> > > + if (!py_modname)
> > > + return NULL;
> > > + py_ctypes_mod = PyImport_Import (py_modname);
> > > + Py_DECREF (py_modname);
> > > + if (!py_ctypes_mod)
> > > + return NULL;
>
> ...the new code as well.
>
> > > + }
> > > +
> > > + return PyObject_CallMethod (py_ctypes_mod, "c_int",
"i", err);
> > > +}
> >
> > I guess because of the Python GIL we don't need locking here?
>
> But you're right that any time we call into the python interpreter for
> something that may entail quite some level of complexity, the GIL lock
> needs to be worried about. I'll have to audit this and other recent
> patches to see if we need to add more use of
> PyGILState_{Ensure/Release} around various code blocks (after first
> researching what the lock is supposed to do...).
Okay, after reading more about the GIL, it looks like we are (likely)
safe only because we have never really been multi-threaded: none of
our libnbd code uses Py_BEGIN_ALLOW_THREADS to release the GIL around
our calls into libnbd C code, and therefore even though our placement
of PyGILState_Ensure() was too late, all of our callbacks have been
reached from places where we already had the GIL (well, hopefully
that's true). At any rate, I'm working on a patch to explicitly
release the GIL around calls into libnbd C code, which means we DO
need to fix our callback wrappers to grab it at the right point. But
the code in utils.c is indeed safe because of the GIL.
FWIW the libguestfs python bindings allow threads and have callbacks
(back to C) which release the GIL, so may be a good example to follow.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines. Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top
8:34 a.m.
New subject: [libnbd PATCH v3 2/5] python: Alter lock for persistent buffer
As long as we were wrapping a C buffer and not leaking any Python
objects, we had to lock the nbd.Buffer object, to ensure that the
PyCapsule was not released prematurely, as there was nothing else to
hold on to. But now that the previous commit (4f15d0e9) swapped to
wrapping a Python bytearray object, we are better off locking the
PyMemoryView created from that object, for two reasons. First, if we
are going to allow aio_{pread,pwrite} to take an arbitrary buffer-like
object instead of requiring an nbd.Buffer object, then that's all we
have available to lock. Second, hanging on to a PyMemoryView provides
some nice guarantees that the underlying python buffer-like object
won't be reallocated behind our backs. The following snippets from
nbdsh use bad style (you should never access a ._* member outside of
its class), but demonstrate the same issue we would be faced with once
nbd.Buffer starts sharing rather than copying in the public
.{to,from}_bytearray:
Pre-patch:
nbd> b = nbd.Buffer(10)
nbd> c = h.aio_pread(b, 0)
nbd> b._o.pop()
0
nbd> b.size()
9
nbd> h.poll(-1)
Oops - we changed the size of the underlying bytearray prior to
receiving the server's reply. If that caused python to reallocate the
buffer underlying the bytearray, we've just clobbered random memory.
Post-patch:
nbd> b = nbd.Buffer(10)
nbd> c = h.aio_pread(b, 0)
nbd> b._o.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)
nbd> h.aio_command_completed(c)
True
nbd> b._o.pop()
0
nbd> b.size()
9
There - now we are sure that as long as our read is pending, the
underlying bytearray cannot be reallocated; but as soon as it
completes, we have regained full use of the bytearray.
---
generator/Python.ml | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/generator/Python.ml b/generator/Python.ml
index 6980034..20d7e78 100644
--- a/generator/Python.ml
+++ b/generator/Python.ml
@@ -424,16 +424,12 @@ let
pr " %s_view = nbd_internal_py_get_aio_view (%s, true);\n" n n;
pr " if (!%s_view) goto out;\n" n;
pr " py_%s = PyMemoryView_GET_BUFFER (%s_view);\n" 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 " completion_user_data->view = %s_view;\n" n
| BytesPersistOut (n, _) ->
pr " %s_view = nbd_internal_py_get_aio_view (%s, false);\n" n n;
pr " if (!%s_view) goto out;\n" n;
pr " py_%s = PyMemoryView_GET_BUFFER (%s_view);\n" 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 " completion_user_data->view = %s_view;\n" 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;
@@ -538,8 +534,7 @@ let
pr " if (%s.obj)\n" n;
pr " PyBuffer_Release (&%s);\n" n
| BytesOut (n, _) -> pr " Py_XDECREF (%s);\n" n
- | BytesPersistIn (n, _) | BytesPersistOut (n, _) ->
- pr " Py_XDECREF (%s_view);\n" n
+ | BytesPersistIn (n, _) | BytesPersistOut (n, _) -> ()
| Closure { cbname } ->
pr " free_user_data (%s_user_data);\n" cbname
| Enum _ -> ()
@@ -588,7 +583,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 " PyObject *view; /* Optional PyMemoryView of persistent buffer. */\n";
pr "};\n";
pr "\n";
pr "static struct user_data *\n";
@@ -609,7 +604,7 @@ let
pr "\n";
pr " if (data) {\n";
pr " Py_XDECREF (data->fn);\n";
- pr " Py_XDECREF (data->buf);\n";
+ pr " Py_XDECREF (data->view);\n";
pr " free (data);\n";
pr " }\n";
pr "}\n";
--
2.36.1
9:27 a.m.
New subject: [libnbd PATCH v3 2/5] python: Alter lock for persistent buffer
On Thu, Jun 09, 2022 at 08:34:44AM -0500, Eric Blake wrote:
As long as we were wrapping a C buffer and not leaking any Python
objects, we had to lock the nbd.Buffer object, to ensure that the
PyCapsule was not released prematurely, as there was nothing else to
hold on to. But now that the previous commit (4f15d0e9) swapped to
wrapping a Python bytearray object, we are better off locking the
PyMemoryView created from that object, for two reasons. First, if we
are going to allow aio_{pread,pwrite} to take an arbitrary buffer-like
object instead of requiring an nbd.Buffer object, then that's all we
have available to lock. Second, hanging on to a PyMemoryView provides
some nice guarantees that the underlying python buffer-like object
won't be reallocated behind our backs. The following snippets from
nbdsh use bad style (you should never access a ._* member outside of
its class), but demonstrate the same issue we would be faced with once
nbd.Buffer starts sharing rather than copying in the public
.{to,from}_bytearray:
Pre-patch:
nbd> b = nbd.Buffer(10)
nbd> c = h.aio_pread(b, 0)
nbd> b._o.pop()
0
nbd> b.size()
9
nbd> h.poll(-1)
Oops - we changed the size of the underlying bytearray prior to
receiving the server's reply. If that caused python to reallocate the
buffer underlying the bytearray, we've just clobbered random memory.
Post-patch:
nbd> b = nbd.Buffer(10)
nbd> c = h.aio_pread(b, 0)
nbd> b._o.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
Wow what a horrible error message! However it comes from Python, not
us, so there's not a lot we can do about it.
nbd> h.poll(-1)
nbd> h.aio_command_completed(c)
True
nbd> b._o.pop()
0
nbd> b.size()
9
There - now we are sure that as long as our read is pending, the
underlying bytearray cannot be reallocated; but as soon as it
completes, we have regained full use of the bytearray.
---
generator/Python.ml | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/generator/Python.ml b/generator/Python.ml
index 6980034..20d7e78 100644
--- a/generator/Python.ml
+++ b/generator/Python.ml
@@ -424,16 +424,12 @@ let
pr " %s_view = nbd_internal_py_get_aio_view (%s, true);\n" n n;
pr " if (!%s_view) goto out;\n" n;
pr " py_%s = PyMemoryView_GET_BUFFER (%s_view);\n" 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 " completion_user_data->view = %s_view;\n" n
| BytesPersistOut (n, _) ->
pr " %s_view = nbd_internal_py_get_aio_view (%s, false);\n" n n;
pr " if (!%s_view) goto out;\n" n;
pr " py_%s = PyMemoryView_GET_BUFFER (%s_view);\n" 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 " completion_user_data->view = %s_view;\n" 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;
@@ -538,8 +534,7 @@ let
pr " if (%s.obj)\n" n;
pr " PyBuffer_Release (&%s);\n" n
| BytesOut (n, _) -> pr " Py_XDECREF (%s);\n" n
- | BytesPersistIn (n, _) | BytesPersistOut (n, _) ->
- pr " Py_XDECREF (%s_view);\n" n
+ | BytesPersistIn (n, _) | BytesPersistOut (n, _) -> ()
| Closure { cbname } ->
pr " free_user_data (%s_user_data);\n" cbname
| Enum _ -> ()
@@ -588,7 +583,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 " PyObject *view; /* Optional PyMemoryView of persistent buffer.
*/\n";
pr "};\n";
pr "\n";
pr "static struct user_data *\n";
@@ -609,7 +604,7 @@ let
pr "\n";
pr " if (data) {\n";
pr " Py_XDECREF (data->fn);\n";
- pr " Py_XDECREF (data->buf);\n";
+ pr " Py_XDECREF (data->view);\n";
pr " free (data);\n";
pr " }\n";
pr "}\n";
I gather it works, but I don't understand why. What increments the
ref count on data->view?
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
11:24 a.m.
New subject: [libnbd PATCH v3 2/5] python: Alter lock for persistent buffer
On Thu, Jun 09, 2022 at 03:27:36PM +0100, Richard W.M. Jones wrote:
> Post-patch:
> nbd> b = nbd.Buffer(10)
> nbd> c = h.aio_pread(b, 0)
> nbd> b._o.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
Wow what a horrible error message! However it comes from Python, not
us, so there's not a lot we can do about it.
Yeah, it doesn't tell you how to find which object is still hanging on
to the buffer export. And during development, I had several times
where I didn't call enough Py_DECREF(), which was interesting to track
down where the stale references were being kept when all I had was
this message to go on.
> nbd> h.poll(-1)
> nbd> h.aio_command_completed(c)
> True
> nbd> b._o.pop()
> 0
> nbd> b.size()
> 9
But I'm pretty happy about the results - Python's ability to magically
lock a bytearray from being resized while a memoryview is active, so
that we can then peer into its underlying C memory without copying,
then restore full functionality when we're done with the underlying
memory, is pretty cool.
As to your question about ref-counting:
> +++ b/generator/Python.ml
> @@ -424,16 +424,12 @@ let
> pr " %s_view = nbd_internal_py_get_aio_view (%s, true);\n" n n;
> pr " if (!%s_view) goto out;\n" n;
> pr " py_%s = PyMemoryView_GET_BUFFER (%s_view);\n" 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 " completion_user_data->view = %s_view;\n" n
Pre-patch and post-patch: buf_view provides a new object (reference
count incremented to at least 1), as a result of calling
nbd_internal_py_get_aio_view. Pre-patch did not want that reference
count left around, so it cleans up that reference in the same function
at [1]; rather, pre-patch wanted to save a different object (buf) and
had to both Py_INCREF() it as well as assigning buf into
completion_user_data for later cleanup [2]. Post-patch WANTS to use
the reference count on view as our long-term lock, so it no longer
needs in-function cleanup at [1], and no longer has to mess with the
reference counts on buf.
> | BytesPersistOut (n, _) ->
> pr " %s_view = nbd_internal_py_get_aio_view (%s, false);\n" n n;
> pr " if (!%s_view) goto out;\n" n;
> pr " py_%s = PyMemoryView_GET_BUFFER (%s_view);\n" 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 " completion_user_data->view = %s_view;\n" 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;
> @@ -538,8 +534,7 @@ let
> pr " if (%s.obj)\n" n;
> pr " PyBuffer_Release (&%s);\n" n
> | BytesOut (n, _) -> pr " Py_XDECREF (%s);\n" n
> - | BytesPersistIn (n, _) | BytesPersistOut (n, _) ->
> - pr " Py_XDECREF (%s_view);\n" n
> + | BytesPersistIn (n, _) | BytesPersistOut (n, _) -> ()
[1] mentioned above, where we cleaned up buf_view pre-patch but not
post-patch.
> | Closure { cbname } ->
> pr " free_user_data (%s_user_data);\n" cbname
> | Enum _ -> ()
> @@ -588,7 +583,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 " PyObject *view; /* Optional PyMemoryView of persistent buffer.
*/\n";
> pr "};\n";
> pr "\n";
> pr "static struct user_data *\n";
> @@ -609,7 +604,7 @@ let
> pr "\n";
> pr " if (data) {\n";
> pr " Py_XDECREF (data->fn);\n";
> - pr " Py_XDECREF (data->buf);\n";
> + pr " Py_XDECREF (data->view);\n";
[2], where we cleaned up buf pre-patch, and buf_view post-patch.
> pr " free (data);\n";
> pr " }\n";
> pr "}\n";
I gather it works, but I don't understand why. What increments the
ref count on data->view?
I hope that answered your question.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
11:35 a.m.
New subject: [libnbd PATCH v3 2/5] python: Alter lock for persistent buffer
On Thu, Jun 09, 2022 at 11:24:48AM -0500, Eric Blake wrote:
On Thu, Jun 09, 2022 at 03:27:36PM +0100, Richard W.M. Jones wrote:
> > Post-patch:
> > nbd> b = nbd.Buffer(10)
> > nbd> c = h.aio_pread(b, 0)
> > nbd> b._o.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
>
> Wow what a horrible error message! However it comes from Python, not
> us, so there's not a lot we can do about it.
Yeah, it doesn't tell you how to find which object is still hanging on
to the buffer export. And during development, I had several times
where I didn't call enough Py_DECREF(), which was interesting to track
down where the stale references were being kept when all I had was
this message to go on.
>
> > nbd> h.poll(-1)
> > nbd> h.aio_command_completed(c)
> > True
> > nbd> b._o.pop()
> > 0
> > nbd> b.size()
> > 9
But I'm pretty happy about the results - Python's ability to magically
lock a bytearray from being resized while a memoryview is active, so
that we can then peer into its underlying C memory without copying,
then restore full functionality when we're done with the underlying
memory, is pretty cool.
As to your question about ref-counting:
> > +++ b/generator/Python.ml
> > @@ -424,16 +424,12 @@ let
> > pr " %s_view = nbd_internal_py_get_aio_view (%s, true);\n" n
n;
> > pr " if (!%s_view) goto out;\n" n;
> > pr " py_%s = PyMemoryView_GET_BUFFER (%s_view);\n" 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 " completion_user_data->view = %s_view;\n" n
Pre-patch and post-patch: buf_view provides a new object (reference
count incremented to at least 1), as a result of calling
nbd_internal_py_get_aio_view. Pre-patch did not want that reference
count left around, so it cleans up that reference in the same function
at [1]; rather, pre-patch wanted to save a different object (buf) and
had to both Py_INCREF() it as well as assigning buf into
completion_user_data for later cleanup [2]. Post-patch WANTS to use
the reference count on view as our long-term lock, so it no longer
needs in-function cleanup at [1], and no longer has to mess with the
reference counts on buf.
> > | BytesPersistOut (n, _) ->
> > pr " %s_view = nbd_internal_py_get_aio_view (%s, false);\n"
n n;
> > pr " if (!%s_view) goto out;\n" n;
> > pr " py_%s = PyMemoryView_GET_BUFFER (%s_view);\n" 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 " completion_user_data->view = %s_view;\n" 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;
> > @@ -538,8 +534,7 @@ let
> > pr " if (%s.obj)\n" n;
> > pr " PyBuffer_Release (&%s);\n" n
> > | BytesOut (n, _) -> pr " Py_XDECREF (%s);\n" n
> > - | BytesPersistIn (n, _) | BytesPersistOut (n, _) ->
> > - pr " Py_XDECREF (%s_view);\n" n
> > + | BytesPersistIn (n, _) | BytesPersistOut (n, _) -> ()
[1] mentioned above, where we cleaned up buf_view pre-patch but not
post-patch.
> > | Closure { cbname } ->
> > pr " free_user_data (%s_user_data);\n" cbname
> > | Enum _ -> ()
> > @@ -588,7 +583,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 " PyObject *view; /* Optional PyMemoryView of persistent buffer.
*/\n";
> > pr "};\n";
> > pr "\n";
> > pr "static struct user_data *\n";
> > @@ -609,7 +604,7 @@ let
> > pr "\n";
> > pr " if (data) {\n";
> > pr " Py_XDECREF (data->fn);\n";
> > - pr " Py_XDECREF (data->buf);\n";
> > + pr " Py_XDECREF (data->view);\n";
[2], where we cleaned up buf pre-patch, and buf_view post-patch.
> > pr " free (data);\n";
> > pr " }\n";
> > pr "}\n";
>
> I gather it works, but I don't understand why. What increments the
> ref count on data->view?
I hope that answered your question.
Sure thing, thanks.
Reviewed-by: Richard W.M. Jones <rjones(a)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-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
8:34 a.m.
New subject: [libnbd PATCH v3 3/5] python: Accept all buffer-like objects in aio_p{read, write}
After the work in the previous patches, it is now a trivial feature
addition to support any buffer-like object as the argument to
h.aio_{pread,pwrite}, and matching how h.pwrite already did that. For
example, you can do h.aio_pwrite(b'123', 0), instead of having to copy
into nbd.Buffer first. More importantly, whereas
nbd.Buffer.to_bytearray() currently copies out and
nbd.Buffer.from_bytearray(buffer) copies in, using native types means
you can reduce that copy overhead.
For a demonstration of the speedups possible, compare two scripts that
utilize a new buffer for every I/O (important if you are going to do
parallel operations - even though this demo is serialized), one using
nbd.Buffer, the other using native python types, with timing on my
machine:
$ export script1='
m=1024*1024
size=h.get_size()
for i in range(size // m):
b1 = nbd.Buffer(m)
c = h.aio_pread(b1, m*i)
while not h.aio_command_completed(c):
h.poll(-1)
b2 = nbd.Buffer.from_bytearray(b1.to_bytearray())
c = h.aio_pwrite(b2, m*i)
while not h.aio_command_completed(c):
h.poll(-1)
'
$ export script2='
m=1024*1024
size=h.get_size()
for i in range(size // m):
b1 = bytearray(m)
c = h.aio_pread(b1, m*i)
while not h.aio_command_completed(c):
h.poll(-1)
b2 = bytes(b1)
c = h.aio_pwrite(b2, m*i)
while not h.aio_command_completed(c):
h.poll(-1)
'
$ nbdkit -U - --filter=checkwrite pattern 10G \
--run 'nbdsh -u "$uri" -c "h.set_pread_initialize(True)" -c
"script1"'
takes ~31.5s (this setting for pread_initialize is default)
$ nbdkit -U - --filter=checkwrite pattern 10G \
--run 'nbdsh -u "$uri" -c "h.set_pread_initialize(False)" -c
"script1"'
takes ~30.8s (not re-zeroing the buffer during aio_pread helped)
$ nbdkit -U - --filter=checkwrite pattern 10G \
--run 'nbdsh -u "$uri" -c "h.set_pread_initialize(True)" -c
"script2"'
takes ~23.0s (less copying when going from b1 to b2 helped even more)
$ nbdkit -U - --filter=checkwrite pattern 10G \
--run 'nbdsh -u "$uri" -c "h.set_pread_initialize(False)" -c
"script2"'
takes ~22.8s (again, aio_pread is faster when not re-zeroing)
Of course, you can get better speed still by reusing the same buffer
for every operation, at which point the difference is in the noise;
but this approach is no longer parallelizable as written (you don't
want the same buffer in use by more than one aio operation at a time):
$ export script3='
m=1024*1024
size=h.get_size()
h.set_pread_initialize(False)
buf=nbd.Buffer(m) # or bytearray(m)
for i in range(size // m):
c = h.aio_pread(buf, m*i)
while not h.aio_command_completed(c):
h.poll(-1)
c = h.aio_pwrite(buf, m*i)
while not h.aio_command_completed(c):
h.poll(-1)
'
$ nbdkit -U - --filter=checkwrite pattern 10G \
--run 'nbdsh -u "$uri" -c "script3"'
takes about ~15.2s, regardless of whether I used nbd.Buffer(m) or
bytearray(m) when setting up buf.
---
generator/Python.ml | 8 ++++----
python/handle.c | 19 +++++++++++++------
python/t/500-aio-pread.py | 21 +++++++++++++++++++++
python/t/510-aio-pwrite.py | 17 +++++++++++++++++
4 files changed, 55 insertions(+), 10 deletions(-)
diff --git a/generator/Python.ml b/generator/Python.ml
index 20d7e78..cd7f0e3 100644
--- a/generator/Python.ml
+++ b/generator/Python.ml
@@ -38,7 +38,7 @@ let
extern void nbd_internal_py_free_string_list (char **);
extern int nbd_internal_py_get_sockaddr (PyObject *,
struct sockaddr_storage *, socklen_t *);
-extern PyObject *nbd_internal_py_get_aio_view (PyObject *, bool);
+extern PyObject *nbd_internal_py_get_aio_view (PyObject *, int);
extern int nbd_internal_py_init_aio_buffer (PyObject *);
extern PyObject *nbd_internal_py_get_nbd_buffer_type (void);
extern PyObject *nbd_internal_py_wrap_errptr (int);
@@ -288,7 +288,7 @@ let
pr " Py_ssize_t %s;\n" count
| BytesPersistIn (n, _)
| BytesPersistOut (n, _) ->
- pr " PyObject *%s; /* Instance of nbd.Buffer */\n" n;
+ pr " PyObject *%s; /* Buffer-like object or nbd.Buffer */\n" n;
pr " PyObject *%s_view = NULL; /* PyMemoryView of %s */\n" n n;
pr " Py_buffer *py_%s; /* buffer of %s_view */\n" n n
| Closure { cbname } ->
@@ -421,12 +421,12 @@ let
pr " %s = PyByteArray_FromStringAndSize (NULL, %s);\n" n count;
pr " if (%s == NULL) goto out;\n" n
| BytesPersistIn (n, _) ->
- pr " %s_view = nbd_internal_py_get_aio_view (%s, true);\n" n n;
+ pr " %s_view = nbd_internal_py_get_aio_view (%s, PyBUF_READ);\n" n n;
pr " if (!%s_view) goto out;\n" n;
pr " py_%s = PyMemoryView_GET_BUFFER (%s_view);\n" n n;
pr " completion_user_data->view = %s_view;\n" n
| BytesPersistOut (n, _) ->
- pr " %s_view = nbd_internal_py_get_aio_view (%s, false);\n" n n;
+ pr " %s_view = nbd_internal_py_get_aio_view (%s, PyBUF_WRITE);\n" n n;
pr " if (!%s_view) goto out;\n" n;
pr " py_%s = PyMemoryView_GET_BUFFER (%s_view);\n" n n;
pr " completion_user_data->view = %s_view;\n" n
diff --git a/python/handle.c b/python/handle.c
index ca6c8e9..f72d200 100644
--- a/python/handle.c
+++ b/python/handle.c
@@ -96,16 +96,23 @@ nbd_internal_py_display_version (PyObject *self, PyObject *args)
return Py_None;
}
-/* Return new reference to MemoryView wrapping aio_buffer contents */
+/* Return new reference to MemoryView wrapping buffer or aio_buffer contents.
+ * buffertype is PyBUF_READ or PyBUF_WRITE, for how the buffer will be used
+ * (remember, aio_pwrite READS the buffer, aio_pread WRITES into the buffer).
+ */
PyObject *
-nbd_internal_py_get_aio_view (PyObject *object, bool require_init)
+nbd_internal_py_get_aio_view (PyObject *object, int buffertype)
{
PyObject *buffer = NULL;
- if (PyObject_IsInstance (object, nbd_internal_py_get_nbd_buffer_type ())) {
+ if (PyObject_CheckBuffer (object))
+ buffer = object;
+ else if (PyObject_IsInstance (object,
+ nbd_internal_py_get_nbd_buffer_type ())) {
buffer = PyObject_GetAttrString (object, "_o");
- if (require_init && ! PyObject_HasAttrString (object, "_init")) {
+ if (buffertype == PyBUF_READ &&
+ ! PyObject_HasAttrString (object, "_init")) {
assert (PyByteArray_Check (buffer));
memset (PyByteArray_AS_STRING (buffer), 0,
PyByteArray_GET_SIZE (buffer));
@@ -115,10 +122,10 @@ nbd_internal_py_get_aio_view (PyObject *object, bool require_init)
}
if (buffer)
- return PyMemoryView_FromObject (buffer);
+ return PyMemoryView_GetContiguous (buffer, buffertype, 'A');
PyErr_SetString (PyExc_TypeError,
- "aio_buffer: expecting nbd.Buffer instance");
+ "aio_buffer: expecting buffer or nbd.Buffer instance");
return NULL;
}
diff --git a/python/t/500-aio-pread.py b/python/t/500-aio-pread.py
index 781b709..6851928 100644
--- a/python/t/500-aio-pread.py
+++ b/python/t/500-aio-pread.py
@@ -45,3 +45,24 @@ if sys.byteorder == 'little':
arr.byteswap()
assert buf1 == memoryview(arr).cast('B')[:512]
assert buf2 == memoryview(arr).cast('B')[512:]
+
+# It is also possible to read directly into any writeable buffer-like object.
+# However, aio.Buffer(n) with h.set_pread_initialize(False) may be faster,
+# because it skips python's pre-initialization of bytearray(n).
+try:
+ h.aio_pread(bytes(512), 0)
+ assert False
+except BufferError:
+ pass
+buf3 = bytearray(512)
+cookie = h.aio_pread(buf3, 0)
+# While the read is pending, the buffer cannot be resized
+try:
+ buf3.pop()
+ assert False
+except BufferError:
+ pass
+while not h.aio_command_completed(cookie):
+ h.poll(-1)
+buf3.append(buf3.pop())
+assert buf3 == buf1
diff --git a/python/t/510-aio-pwrite.py b/python/t/510-aio-pwrite.py
index d09e249..4b4aac8 100644
--- a/python/t/510-aio-pwrite.py
+++ b/python/t/510-aio-pwrite.py
@@ -67,4 +67,21 @@ with open(datafile, "rb") as f:
assert nbd.Buffer.from_bytearray(content).is_zero()
+# It is also possible to write any buffer-like object.
+# While the write is pending, the buffer cannot be resized
+cookie = h.aio_pwrite(buf, 0, flags=nbd.CMD_FLAG_FUA)
+try:
+ buf.pop()
+ assert False
+except BufferError:
+ pass
+while not h.aio_command_completed(cookie):
+ h.poll(-1)
+buf.append(buf.pop())
+
+with open(datafile, "rb") as f:
+ content = f.read()
+
+assert buf == content
+
os.unlink(datafile)
--
2.36.1
9:31 a.m.
New subject: [libnbd PATCH v3 3/5] python: Accept all buffer-like objects in aio_p{read, write}
On Thu, Jun 09, 2022 at 08:34:45AM -0500, Eric Blake wrote:
After the work in the previous patches, it is now a trivial feature
addition to support any buffer-like object as the argument to
h.aio_{pread,pwrite}, and matching how h.pwrite already did that. For
example, you can do h.aio_pwrite(b'123', 0), instead of having to copy
into nbd.Buffer first. More importantly, whereas
nbd.Buffer.to_bytearray() currently copies out and
nbd.Buffer.from_bytearray(buffer) copies in, using native types means
you can reduce that copy overhead.
For a demonstration of the speedups possible, compare two scripts that
utilize a new buffer for every I/O (important if you are going to do
parallel operations - even though this demo is serialized), one using
nbd.Buffer, the other using native python types, with timing on my
machine:
$ export script1='
m=1024*1024
size=h.get_size()
for i in range(size // m):
b1 = nbd.Buffer(m)
c = h.aio_pread(b1, m*i)
while not h.aio_command_completed(c):
h.poll(-1)
b2 = nbd.Buffer.from_bytearray(b1.to_bytearray())
c = h.aio_pwrite(b2, m*i)
while not h.aio_command_completed(c):
h.poll(-1)
'
$ export script2='
m=1024*1024
size=h.get_size()
for i in range(size // m):
b1 = bytearray(m)
c = h.aio_pread(b1, m*i)
while not h.aio_command_completed(c):
h.poll(-1)
b2 = bytes(b1)
c = h.aio_pwrite(b2, m*i)
while not h.aio_command_completed(c):
h.poll(-1)
'
$ nbdkit -U - --filter=checkwrite pattern 10G \
--run 'nbdsh -u "$uri" -c "h.set_pread_initialize(True)" -c
"script1"'
Should that be "$script1" ? (and same below)
takes ~31.5s (this setting for pread_initialize is default)
$ nbdkit -U - --filter=checkwrite pattern 10G \
--run 'nbdsh -u "$uri" -c "h.set_pread_initialize(False)" -c
"script1"'
takes ~30.8s (not re-zeroing the buffer during aio_pread helped)
$ nbdkit -U - --filter=checkwrite pattern 10G \
--run 'nbdsh -u "$uri" -c "h.set_pread_initialize(True)" -c
"script2"'
takes ~23.0s (less copying when going from b1 to b2 helped even more)
$ nbdkit -U - --filter=checkwrite pattern 10G \
--run 'nbdsh -u "$uri" -c "h.set_pread_initialize(False)" -c
"script2"'
takes ~22.8s (again, aio_pread is faster when not re-zeroing)
Of course, you can get better speed still by reusing the same buffer
for every operation, at which point the difference is in the noise;
but this approach is no longer parallelizable as written (you don't
want the same buffer in use by more than one aio operation at a time):
$ export script3='
m=1024*1024
size=h.get_size()
h.set_pread_initialize(False)
buf=nbd.Buffer(m) # or bytearray(m)
for i in range(size // m):
c = h.aio_pread(buf, m*i)
while not h.aio_command_completed(c):
h.poll(-1)
c = h.aio_pwrite(buf, m*i)
while not h.aio_command_completed(c):
h.poll(-1)
'
$ nbdkit -U - --filter=checkwrite pattern 10G \
--run 'nbdsh -u "$uri" -c "script3"'
takes about ~15.2s, regardless of whether I used nbd.Buffer(m) or
bytearray(m) when setting up buf.
---
generator/Python.ml | 8 ++++----
python/handle.c | 19 +++++++++++++------
python/t/500-aio-pread.py | 21 +++++++++++++++++++++
python/t/510-aio-pwrite.py | 17 +++++++++++++++++
4 files changed, 55 insertions(+), 10 deletions(-)
diff --git a/generator/Python.ml b/generator/Python.ml
index 20d7e78..cd7f0e3 100644
--- a/generator/Python.ml
+++ b/generator/Python.ml
@@ -38,7 +38,7 @@ let
extern void nbd_internal_py_free_string_list (char **);
extern int nbd_internal_py_get_sockaddr (PyObject *,
struct sockaddr_storage *, socklen_t *);
-extern PyObject *nbd_internal_py_get_aio_view (PyObject *, bool);
+extern PyObject *nbd_internal_py_get_aio_view (PyObject *, int);
extern int nbd_internal_py_init_aio_buffer (PyObject *);
extern PyObject *nbd_internal_py_get_nbd_buffer_type (void);
extern PyObject *nbd_internal_py_wrap_errptr (int);
@@ -288,7 +288,7 @@ let
pr " Py_ssize_t %s;\n" count
| BytesPersistIn (n, _)
| BytesPersistOut (n, _) ->
- pr " PyObject *%s; /* Instance of nbd.Buffer */\n" n;
+ pr " PyObject *%s; /* Buffer-like object or nbd.Buffer */\n" n;
pr " PyObject *%s_view = NULL; /* PyMemoryView of %s */\n" n n;
pr " Py_buffer *py_%s; /* buffer of %s_view */\n" n n
| Closure { cbname } ->
@@ -421,12 +421,12 @@ let
pr " %s = PyByteArray_FromStringAndSize (NULL, %s);\n" n count;
pr " if (%s == NULL) goto out;\n" n
| BytesPersistIn (n, _) ->
- pr " %s_view = nbd_internal_py_get_aio_view (%s, true);\n" n n;
+ pr " %s_view = nbd_internal_py_get_aio_view (%s, PyBUF_READ);\n" n n;
pr " if (!%s_view) goto out;\n" n;
pr " py_%s = PyMemoryView_GET_BUFFER (%s_view);\n" n n;
pr " completion_user_data->view = %s_view;\n" n
| BytesPersistOut (n, _) ->
- pr " %s_view = nbd_internal_py_get_aio_view (%s, false);\n" n n;
+ pr " %s_view = nbd_internal_py_get_aio_view (%s, PyBUF_WRITE);\n" n
n;
pr " if (!%s_view) goto out;\n" n;
pr " py_%s = PyMemoryView_GET_BUFFER (%s_view);\n" n n;
pr " completion_user_data->view = %s_view;\n" n
diff --git a/python/handle.c b/python/handle.c
index ca6c8e9..f72d200 100644
--- a/python/handle.c
+++ b/python/handle.c
@@ -96,16 +96,23 @@ nbd_internal_py_display_version (PyObject *self, PyObject *args)
return Py_None;
}
-/* Return new reference to MemoryView wrapping aio_buffer contents */
+/* Return new reference to MemoryView wrapping buffer or aio_buffer contents.
+ * buffertype is PyBUF_READ or PyBUF_WRITE, for how the buffer will be used
+ * (remember, aio_pwrite READS the buffer, aio_pread WRITES into the buffer).
+ */
PyObject *
-nbd_internal_py_get_aio_view (PyObject *object, bool require_init)
+nbd_internal_py_get_aio_view (PyObject *object, int buffertype)
{
PyObject *buffer = NULL;
- if (PyObject_IsInstance (object, nbd_internal_py_get_nbd_buffer_type ())) {
+ if (PyObject_CheckBuffer (object))
+ buffer = object;
+ else if (PyObject_IsInstance (object,
+ nbd_internal_py_get_nbd_buffer_type ())) {
buffer = PyObject_GetAttrString (object, "_o");
- if (require_init && ! PyObject_HasAttrString (object, "_init")) {
+ if (buffertype == PyBUF_READ &&
+ ! PyObject_HasAttrString (object, "_init")) {
assert (PyByteArray_Check (buffer));
memset (PyByteArray_AS_STRING (buffer), 0,
PyByteArray_GET_SIZE (buffer));
@@ -115,10 +122,10 @@ nbd_internal_py_get_aio_view (PyObject *object, bool require_init)
}
if (buffer)
- return PyMemoryView_FromObject (buffer);
+ return PyMemoryView_GetContiguous (buffer, buffertype, 'A');
PyErr_SetString (PyExc_TypeError,
- "aio_buffer: expecting nbd.Buffer instance");
+ "aio_buffer: expecting buffer or nbd.Buffer instance");
return NULL;
}
diff --git a/python/t/500-aio-pread.py b/python/t/500-aio-pread.py
index 781b709..6851928 100644
--- a/python/t/500-aio-pread.py
+++ b/python/t/500-aio-pread.py
@@ -45,3 +45,24 @@ if sys.byteorder == 'little':
arr.byteswap()
assert buf1 == memoryview(arr).cast('B')[:512]
assert buf2 == memoryview(arr).cast('B')[512:]
+
+# It is also possible to read directly into any writeable buffer-like object.
+# However, aio.Buffer(n) with h.set_pread_initialize(False) may be faster,
+# because it skips python's pre-initialization of bytearray(n).
+try:
+ h.aio_pread(bytes(512), 0)
+ assert False
+except BufferError:
+ pass
+buf3 = bytearray(512)
+cookie = h.aio_pread(buf3, 0)
+# While the read is pending, the buffer cannot be resized
+try:
+ buf3.pop()
+ assert False
+except BufferError:
+ pass
+while not h.aio_command_completed(cookie):
+ h.poll(-1)
+buf3.append(buf3.pop())
+assert buf3 == buf1
diff --git a/python/t/510-aio-pwrite.py b/python/t/510-aio-pwrite.py
index d09e249..4b4aac8 100644
--- a/python/t/510-aio-pwrite.py
+++ b/python/t/510-aio-pwrite.py
@@ -67,4 +67,21 @@ with open(datafile, "rb") as f:
assert nbd.Buffer.from_bytearray(content).is_zero()
+# It is also possible to write any buffer-like object.
+# While the write is pending, the buffer cannot be resized
+cookie = h.aio_pwrite(buf, 0, flags=nbd.CMD_FLAG_FUA)
+try:
+ buf.pop()
+ assert False
+except BufferError:
+ pass
+while not h.aio_command_completed(cookie):
+ h.poll(-1)
+buf.append(buf.pop())
+
+with open(datafile, "rb") as f:
+ content = f.read()
+
+assert buf == content
+
os.unlink(datafile)
Acked-by: Richard W.M. Jones <rjones(a)redhat.com>
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit
11:31 a.m.
New subject: [libnbd PATCH v3 3/5] python: Accept all buffer-like objects in aio_p{read, write}
On Thu, Jun 09, 2022 at 03:31:02PM +0100, Richard W.M. Jones wrote:
On Thu, Jun 09, 2022 at 08:34:45AM -0500, Eric Blake wrote:
> After the work in the previous patches, it is now a trivial feature
> addition to support any buffer-like object as the argument to
> h.aio_{pread,pwrite}, and matching how h.pwrite already did that. For
> example, you can do h.aio_pwrite(b'123', 0), instead of having to copy
> into nbd.Buffer first. More importantly, whereas
> nbd.Buffer.to_bytearray() currently copies out and
> nbd.Buffer.from_bytearray(buffer) copies in, using native types means
> you can reduce that copy overhead.
>
> For a demonstration of the speedups possible, compare two scripts that
> utilize a new buffer for every I/O (important if you are going to do
> parallel operations - even though this demo is serialized), one using
> nbd.Buffer, the other using native python types, with timing on my
> machine:
>
> $ export script1='
>
> $ nbdkit -U - --filter=checkwrite pattern 10G \
> --run 'nbdsh -u "$uri" -c "h.set_pread_initialize(True)"
-c "script1"'
Should that be "$script1" ? (and same below)
Yep. Serves me right for copy-pasting the wrong line.
Acked-by: Richard W.M. Jones <rjones(a)redhat.com>
Thanks for looking at this series; I can see from the difference
between Acked-by and Reviewed-by that it gets pretty technical ;)
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
8:34 a.m.
New subject: [libnbd PATCH v3 4/5] python: Support len(nbd.Buffer(n))
b.size() is atypical, Python programmers generally expect to be able
to do len(b). Keep the old spelling for backwards compatibility.
---
generator/Python.ml | 4 ++++
python/t/580-aio-is-zero.py | 2 ++
2 files changed, 6 insertions(+)
diff --git a/generator/Python.ml b/generator/Python.ml
index cd7f0e3..975cab4 100644
--- a/generator/Python.ml
+++ b/generator/Python.ml
@@ -745,6 +745,10 @@ let
return bytearray(self._o)
def size(self):
+ '''Return the size of an AIO buffer.'''
+ return len(self)
+
+ def __len__(self):
'''Return the size of an AIO buffer.'''
return len(self._o)
diff --git a/python/t/580-aio-is-zero.py b/python/t/580-aio-is-zero.py
index 1a9a7d3..dd57e30 100644
--- a/python/t/580-aio-is-zero.py
+++ b/python/t/580-aio-is-zero.py
@@ -24,6 +24,7 @@ import nbd
ba = bytearray(2**20)
buf = nbd.Buffer.from_bytearray(ba)
assert buf.size() == 2**20
+assert len(buf) == 2**20
assert buf.is_zero()
# The above buffer is 2**20 (= 1MB), slices of it should also be zero.
@@ -74,5 +75,6 @@ assert buf.is_zero(2**21-1, 1)
# used with aio_pread; but it will be zeroed if accessed prematurely
buf = nbd.Buffer(1024)
assert buf.size() == 1024
+assert len(buf) == 1024
assert buf.is_zero()
assert nbd.Buffer.from_bytearray(buf.to_bytearray()).is_zero()
--
2.36.1
9:31 a.m.
New subject: [libnbd PATCH v3 4/5] python: Support len(nbd.Buffer(n))
On Thu, Jun 09, 2022 at 08:34:46AM -0500, Eric Blake wrote:
b.size() is atypical, Python programmers generally expect to be able
to do len(b). Keep the old spelling for backwards compatibility.
---
generator/Python.ml | 4 ++++
python/t/580-aio-is-zero.py | 2 ++
2 files changed, 6 insertions(+)
diff --git a/generator/Python.ml b/generator/Python.ml
index cd7f0e3..975cab4 100644
--- a/generator/Python.ml
+++ b/generator/Python.ml
@@ -745,6 +745,10 @@ let
return bytearray(self._o)
def size(self):
+ '''Return the size of an AIO buffer.'''
+ return len(self)
+
+ def __len__(self):
'''Return the size of an AIO buffer.'''
return len(self._o)
diff --git a/python/t/580-aio-is-zero.py b/python/t/580-aio-is-zero.py
index 1a9a7d3..dd57e30 100644
--- a/python/t/580-aio-is-zero.py
+++ b/python/t/580-aio-is-zero.py
@@ -24,6 +24,7 @@ import nbd
ba = bytearray(2**20)
buf = nbd.Buffer.from_bytearray(ba)
assert buf.size() == 2**20
+assert len(buf) == 2**20
assert buf.is_zero()
# The above buffer is 2**20 (= 1MB), slices of it should also be zero.
@@ -74,5 +75,6 @@ assert buf.is_zero(2**21-1, 1)
# used with aio_pread; but it will be zeroed if accessed prematurely
buf = nbd.Buffer(1024)
assert buf.size() == 1024
+assert len(buf) == 1024
assert buf.is_zero()
assert nbd.Buffer.from_bytearray(buf.to_bytearray()).is_zero()
--
Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com>
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit
8:34 a.m.
New subject: [libnbd PATCH v3 5/5] python: Slice structured read callback buffer from original
The Py_BuildValue "y#" format copies data; this is because Python
assumes our C memory can go out of scope, while the user's python
callback can stash off whatever bytearray object it got. But this
copying is inefficient. Now that we already have a Python buffer-like
object in scope for the duration of all structured reads
(pread_structured since commit f8c76c01, aio_pread_structured since
commit 4f15d0e9), we know that the C memory parameter to our
chunk_callback is already (a portion of) the underlying contiguous
memory of an original Python object. If we pass the Python callback a
memoryview object over the correct slice of that memory, then Python
will correctly handle the intricacies of what happens if the callback
saves our memoryview object for later use, all without any data
copying overhead, making use of the pread_structured form much faster.
We basically want to code up the C equivalent to the python expression
'view = memoryview(buf).toreadonly()[start:end]', where start and end
are determined by comparing subbuf/count against the original buf.
But in C, that involves creating PyObject integers to form a PySlice,
then using PyObject_GetItem to apply the slice to a PyMemoryView (I
don't know of any faster tricks; googling does not find any quick
hits). Extract this work into a new utils.c helper function, to avoid
repetition (and in case we figure out a way to do it in fewer lines by
directly manipulating Py_buffer objects).
The testsuite is updated to cover examples of the data persistence
aspect, demonstrating that our stashed slice is still tied to the
returned Python buffer, even though the callback is long since
completed (prior to this patch, stash was an independent bytes copy,
and while it persisted on after the callback, it is frozen in time to
the contents of the buffer at the time the callback was reached). The
fact that data is now shared is is an API change, but one unlikely to
cause problems - it is very unusual for a callback function to try and
stash sub-buffers for later use (neither libnbd nor nbdkit was
utilizing that in their testsuites before this patch).
To demonstrate the speedup now that there is less copying involved per
callback, I tested:
$ export script='
def f(b,o,s,e):
pass
m=1024*1024
size=h.get_size()
h.set_pread_initialize(False)
for i in range(size // m):
buf = h.pread_structured(m, m*i, f)
buf = None
'
$ time ./run nbdkit -U - memory 10G --run 'nbdsh -u "$uri" -c
"$script"'
On my machine, this took 16.9s with libnbd 1.12.3, 15.5s pre-patch,
and 3.6s post-patch, for more than 4x speedup in the best case. Much
of the differences boil down to how efficient the Python garbage
collector is able to spot liveness of various buffers, to reuse rather
than allocate new buffers on the Python side; removing the 'buf =
None' line speeds up libnbd 1.12.3 to 4.5s (better reuse of the "y#"
memory in the callback when backed by C memory), 9.0s pre-patch
(copying "y#" from Python memory has scanning effects), and 3.0s
post-patch (zero-copying is always best).
A variation with aio_pread_structured reusing the same nbd.Buffer is
not quite as drastic: 3.9s with libnbd 1.12.3, 4.0s pre-patch, and
3.4s with this patch, but still shows that it is beneficial.
The corresponding diff to generated code is:
| --- python/methods.h.bak 2022-06-09 07:23:51.657260039 -0500
| +++ python/methods.h 2022-06-09 07:23:57.629266878 -0500
| @@ -36,6 +36,7 @@
| extern int nbd_internal_py_init_aio_buffer (PyObject *);
| extern PyObject *nbd_internal_py_get_nbd_buffer_type (void);
| extern PyObject *nbd_internal_py_wrap_errptr (int);
| +extern PyObject *nbd_internal_py_get_subview (PyObject *, const char *, size_t);
|
| static inline struct nbd_handle *
| get_handle (PyObject *obj)
| --- python/methods.c.bak 2022-06-09 07:23:51.652260034 -0500
| +++ python/methods.c 2022-06-09 07:23:57.644266895 -0500
| @@ -73,13 +73,15 @@ chunk_wrapper (void *user_data, const vo
|
| PyGILState_STATE py_save = PyGILState_UNLOCKED;
| PyObject *py_args, *py_ret;
| + PyObject *py_subbuf = NULL;
| PyObject *py_error = NULL;
|
| + py_subbuf = nbd_internal_py_get_subview (data->view, subbuf, count);
| + if (!py_subbuf) { PyErr_PrintEx (0); goto out; }
| py_error = nbd_internal_py_wrap_errptr (*error);
| if (!py_error) { PyErr_PrintEx (0); goto out; }
|
| - py_args = Py_BuildValue ("(y#KIO)", subbuf, (int) count, offset, status,
| - py_error);
| + py_args = Py_BuildValue ("(OKIO)", py_subbuf, offset, status, py_error);
| if (!py_args) { PyErr_PrintEx (0); goto out; }
|
| py_save = PyGILState_Ensure ();
| @@ -106,6 +108,7 @@ chunk_wrapper (void *user_data, const vo
| };
|
| out:
| + Py_XDECREF (py_subbuf);
| if (py_error) {
| PyObject *py_error_ret = PyObject_GetAttrString (py_error, "value");
| *error = PyLong_AsLong (py_error_ret);
| @@ -2231,6 +2234,7 @@ nbd_internal_py_pread_structured (PyObje
| /* Increment refcount since pointer may be saved by libnbd. */
| Py_INCREF (py_chunk_fn);
| chunk_user_data->fn = py_chunk_fn;
| + chunk_user_data->view = nbd_internal_py_get_aio_view (buf, PyBUF_WRITE);
|
| ret = nbd_pread_structured (h, PyByteArray_AS_STRING (buf), count,
| offset_u64, chunk, flags_u32);
| @@ -3116,6 +3120,7 @@ nbd_internal_py_aio_pread_structured (Py
| /* Increment refcount since pointer may be saved by libnbd. */
| Py_INCREF (py_chunk_fn);
| chunk_user_data->fn = py_chunk_fn;
| + chunk_user_data->view = nbd_internal_py_get_aio_view (buf, PyBUF_WRITE);
|
| if (nbd_internal_py_init_aio_buffer (buf) < 0) goto out;
| ret = nbd_aio_pread_structured (h, py_buf->buf, py_buf->len, offset_u64,
---
generator/Python.ml | 18 ++++--
python/t/405-pread-structured.py | 95 +++++++++---------------------
python/t/505-aio-pread-callback.py | 93 ++++++++---------------------
python/utils.c | 34 +++++++++++
4 files changed, 99 insertions(+), 141 deletions(-)
diff --git a/generator/Python.ml b/generator/Python.ml
index 975cab4..c424239 100644
--- a/generator/Python.ml
+++ b/generator/Python.ml
@@ -42,6 +42,7 @@ let
extern int nbd_internal_py_init_aio_buffer (PyObject *);
extern PyObject *nbd_internal_py_get_nbd_buffer_type (void);
extern PyObject *nbd_internal_py_wrap_errptr (int);
+extern PyObject *nbd_internal_py_get_subview (PyObject *, const char *, size_t);
static inline struct nbd_handle *
get_handle (PyObject *obj)
@@ -163,8 +164,8 @@ let
pr " PyObject *py_args, *py_ret;\n";
List.iter (
function
- | CBArrayAndLen (UInt32 n, _) ->
- pr " PyObject *py_%s = NULL;\n" n
+ | CBArrayAndLen (UInt32 n, _)
+ | CBBytesIn (n, _)
| CBMutable (Int n) ->
pr " PyObject *py_%s = NULL;\n" n
| _ -> ()
@@ -181,7 +182,9 @@ let
pr " if (!py_e_%s) { PyErr_PrintEx (0); goto out; }\n" n;
pr " PyList_SET_ITEM (py_%s, i_%s, py_e_%s);\n" n n n;
pr " }\n"
- | CBBytesIn _
+ | CBBytesIn (n, len) ->
+ pr " py_%s = nbd_internal_py_get_subview (data->view, %s, %s);\n" n
n len;
+ pr " if (!py_%s) { PyErr_PrintEx (0); goto out; }\n" n
| CBInt _
| CBInt64 _ -> ()
| CBMutable (Int n) ->
@@ -198,7 +201,7 @@ let
List.map (
function
| CBArrayAndLen (UInt32 n, _) -> "O", sprintf "py_%s" n
- | CBBytesIn (n, len) -> "y#", sprintf "%s, (int) %s" n len
+ | CBBytesIn (n, _) -> "O", sprintf "py_%s" n
| CBInt n -> "i", n
| CBInt64 n -> "L", n
| CBMutable (Int n) -> "O", sprintf "py_%s" n
@@ -250,6 +253,8 @@ let
function
| CBArrayAndLen (UInt32 n, _) ->
pr " Py_XDECREF (py_%s);\n" n
+ | CBBytesIn (n, _) ->
+ pr " Py_XDECREF (py_%s);\n" n
| CBMutable (Int n) ->
pr " if (py_%s) {\n" n;
pr " PyObject *py_%s_ret = PyObject_GetAttrString (py_%s,
\"value\");\n" n n;
@@ -257,7 +262,6 @@ let
pr " Py_DECREF (py_%s_ret);\n" n;
pr " Py_DECREF (py_%s);\n" n;
pr " }\n"
- | CBBytesIn _
| CBInt _ | CBInt64 _
| CBString _
| CBUInt _ | CBUInt64 _ -> ()
@@ -440,7 +444,9 @@ let
pr " }\n";
pr " /* Increment refcount since pointer may be saved by libnbd.
*/\n";
pr " Py_INCREF (py_%s_fn);\n" cbname;
- pr " %s_user_data->fn = py_%s_fn;\n" cbname cbname
+ pr " %s_user_data->fn = py_%s_fn;\n" cbname cbname;
+ if cbname = "chunk" then
+ pr " chunk_user_data->view = nbd_internal_py_get_aio_view (buf,
PyBUF_WRITE);\n"
| Enum _ -> ()
| Flags (n, _) -> pr " %s_u32 = %s;\n" n n
| Fd _ | Int _ -> ()
diff --git a/python/t/405-pread-structured.py b/python/t/405-pread-structured.py
index afd1e7e..9d3df68 100644
--- a/python/t/405-pread-structured.py
+++ b/python/t/405-pread-structured.py
@@ -1,5 +1,5 @@
# libnbd Python bindings
-# Copyright (C) 2010-2019 Red Hat Inc.
+# Copyright (C) 2010-2022 Red Hat Inc.
#
# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
@@ -17,76 +17,19 @@
import nbd
import errno
+import sys
+from array import array
h = nbd.NBD()
h.connect_command(["nbdkit", "-s", "--exit-with-parent",
"-v",
"pattern", "size=512"])
-expected = (b'\x00\x00\x00\x00\x00\x00\x00\x00'
- + b'\x00\x00\x00\x00\x00\x00\x00\x08'
- + b'\x00\x00\x00\x00\x00\x00\x00\x10'
- + b'\x00\x00\x00\x00\x00\x00\x00\x18'
- + b'\x00\x00\x00\x00\x00\x00\x00 '
- + b'\x00\x00\x00\x00\x00\x00\x00('
- + b'\x00\x00\x00\x00\x00\x00\x000'
- + b'\x00\x00\x00\x00\x00\x00\x008'
- + b'\x00\x00\x00\x00\x00\x00\x00@'
- + b'\x00\x00\x00\x00\x00\x00\x00H'
- + b'\x00\x00\x00\x00\x00\x00\x00P'
- + b'\x00\x00\x00\x00\x00\x00\x00X'
- + b'\x00\x00\x00\x00\x00\x00\x00`'
- + b'\x00\x00\x00\x00\x00\x00\x00h'
- + b'\x00\x00\x00\x00\x00\x00\x00p'
- + b'\x00\x00\x00\x00\x00\x00\x00x'
- + b'\x00\x00\x00\x00\x00\x00\x00\x80'
- + b'\x00\x00\x00\x00\x00\x00\x00\x88'
- + b'\x00\x00\x00\x00\x00\x00\x00\x90'
- + b'\x00\x00\x00\x00\x00\x00\x00\x98'
- + b'\x00\x00\x00\x00\x00\x00\x00\xa0'
- + b'\x00\x00\x00\x00\x00\x00\x00\xa8'
- + b'\x00\x00\x00\x00\x00\x00\x00\xb0'
- + b'\x00\x00\x00\x00\x00\x00\x00\xb8'
- + b'\x00\x00\x00\x00\x00\x00\x00\xc0'
- + b'\x00\x00\x00\x00\x00\x00\x00\xc8'
- + b'\x00\x00\x00\x00\x00\x00\x00\xd0'
- + b'\x00\x00\x00\x00\x00\x00\x00\xd8'
- + b'\x00\x00\x00\x00\x00\x00\x00\xe0'
- + b'\x00\x00\x00\x00\x00\x00\x00\xe8'
- + b'\x00\x00\x00\x00\x00\x00\x00\xf0'
- + b'\x00\x00\x00\x00\x00\x00\x00\xf8'
- + b'\x00\x00\x00\x00\x00\x00\x01\x00'
- + b'\x00\x00\x00\x00\x00\x00\x01\x08'
- + b'\x00\x00\x00\x00\x00\x00\x01\x10'
- + b'\x00\x00\x00\x00\x00\x00\x01\x18'
- + b'\x00\x00\x00\x00\x00\x00\x01 '
- + b'\x00\x00\x00\x00\x00\x00\x01('
- + b'\x00\x00\x00\x00\x00\x00\x010'
- + b'\x00\x00\x00\x00\x00\x00\x018'
- + b'\x00\x00\x00\x00\x00\x00\x01@'
- + b'\x00\x00\x00\x00\x00\x00\x01H'
- + b'\x00\x00\x00\x00\x00\x00\x01P'
- + b'\x00\x00\x00\x00\x00\x00\x01X'
- + b'\x00\x00\x00\x00\x00\x00\x01`'
- + b'\x00\x00\x00\x00\x00\x00\x01h'
- + b'\x00\x00\x00\x00\x00\x00\x01p'
- + b'\x00\x00\x00\x00\x00\x00\x01x'
- + b'\x00\x00\x00\x00\x00\x00\x01\x80'
- + b'\x00\x00\x00\x00\x00\x00\x01\x88'
- + b'\x00\x00\x00\x00\x00\x00\x01\x90'
- + b'\x00\x00\x00\x00\x00\x00\x01\x98'
- + b'\x00\x00\x00\x00\x00\x00\x01\xa0'
- + b'\x00\x00\x00\x00\x00\x00\x01\xa8'
- + b'\x00\x00\x00\x00\x00\x00\x01\xb0'
- + b'\x00\x00\x00\x00\x00\x00\x01\xb8'
- + b'\x00\x00\x00\x00\x00\x00\x01\xc0'
- + b'\x00\x00\x00\x00\x00\x00\x01\xc8'
- + b'\x00\x00\x00\x00\x00\x00\x01\xd0'
- + b'\x00\x00\x00\x00\x00\x00\x01\xd8'
- + b'\x00\x00\x00\x00\x00\x00\x01\xe0'
- + b'\x00\x00\x00\x00\x00\x00\x01\xe8'
- + b'\x00\x00\x00\x00\x00\x00\x01\xf0'
- + b'\x00\x00\x00\x00\x00\x00\x01\xf8')
-
+# The nbdkit pattern plugin exposes 64-bit numbers in bigendian order
+arr = array('q', range(0, 512, 8))
+if sys.byteorder == 'little':
+ arr.byteswap()
+expected = memoryview(arr).cast('B')
+stash = None
def f(user_data, buf2, offset, s, err):
assert err.value == 0
@@ -94,8 +37,15 @@ def f(user_data, buf2, offset, s, err):
if user_data != 42:
raise ValueError('unexpected user_data')
assert buf2 == expected
+ try:
+ buf2[0] = 1
+ assert False
+ except TypeError:
+ pass
assert offset == 0
assert s == nbd.READ_DATA
+ global stash
+ stash = buf2
buf = h.pread_structured(512, 0, lambda *args: f(42, *args))
@@ -104,6 +54,19 @@ print("%r" % buf)
assert buf == expected
+# The callback can stash its slice; as long as that is live, we can't
+# resize buf but can view changes in buf through the slice
+try:
+ buf.pop()
+ assert False
+except BufferError:
+ pass
+buf[0] ^= 1
+assert buf == stash
+stash = None
+buf.pop()
+
+# Tests of error handling
buf = h.pread_structured(512, 0, lambda *args: f(42, *args),
nbd.CMD_FLAG_DF)
diff --git a/python/t/505-aio-pread-callback.py b/python/t/505-aio-pread-callback.py
index 1773aee..5c7b250 100644
--- a/python/t/505-aio-pread-callback.py
+++ b/python/t/505-aio-pread-callback.py
@@ -1,5 +1,5 @@
# libnbd Python bindings
-# Copyright (C) 2010-2019 Red Hat Inc.
+# Copyright (C) 2010-2022 Red Hat Inc.
#
# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
@@ -17,76 +17,19 @@
import nbd
import errno
+import sys
+from array import array
h = nbd.NBD()
h.connect_command(["nbdkit", "-s", "--exit-with-parent",
"-v",
"pattern", "size=512"])
-expected = (b'\x00\x00\x00\x00\x00\x00\x00\x00'
- + b'\x00\x00\x00\x00\x00\x00\x00\x08'
- + b'\x00\x00\x00\x00\x00\x00\x00\x10'
- + b'\x00\x00\x00\x00\x00\x00\x00\x18'
- + b'\x00\x00\x00\x00\x00\x00\x00 '
- + b'\x00\x00\x00\x00\x00\x00\x00('
- + b'\x00\x00\x00\x00\x00\x00\x000'
- + b'\x00\x00\x00\x00\x00\x00\x008'
- + b'\x00\x00\x00\x00\x00\x00\x00@'
- + b'\x00\x00\x00\x00\x00\x00\x00H'
- + b'\x00\x00\x00\x00\x00\x00\x00P'
- + b'\x00\x00\x00\x00\x00\x00\x00X'
- + b'\x00\x00\x00\x00\x00\x00\x00`'
- + b'\x00\x00\x00\x00\x00\x00\x00h'
- + b'\x00\x00\x00\x00\x00\x00\x00p'
- + b'\x00\x00\x00\x00\x00\x00\x00x'
- + b'\x00\x00\x00\x00\x00\x00\x00\x80'
- + b'\x00\x00\x00\x00\x00\x00\x00\x88'
- + b'\x00\x00\x00\x00\x00\x00\x00\x90'
- + b'\x00\x00\x00\x00\x00\x00\x00\x98'
- + b'\x00\x00\x00\x00\x00\x00\x00\xa0'
- + b'\x00\x00\x00\x00\x00\x00\x00\xa8'
- + b'\x00\x00\x00\x00\x00\x00\x00\xb0'
- + b'\x00\x00\x00\x00\x00\x00\x00\xb8'
- + b'\x00\x00\x00\x00\x00\x00\x00\xc0'
- + b'\x00\x00\x00\x00\x00\x00\x00\xc8'
- + b'\x00\x00\x00\x00\x00\x00\x00\xd0'
- + b'\x00\x00\x00\x00\x00\x00\x00\xd8'
- + b'\x00\x00\x00\x00\x00\x00\x00\xe0'
- + b'\x00\x00\x00\x00\x00\x00\x00\xe8'
- + b'\x00\x00\x00\x00\x00\x00\x00\xf0'
- + b'\x00\x00\x00\x00\x00\x00\x00\xf8'
- + b'\x00\x00\x00\x00\x00\x00\x01\x00'
- + b'\x00\x00\x00\x00\x00\x00\x01\x08'
- + b'\x00\x00\x00\x00\x00\x00\x01\x10'
- + b'\x00\x00\x00\x00\x00\x00\x01\x18'
- + b'\x00\x00\x00\x00\x00\x00\x01 '
- + b'\x00\x00\x00\x00\x00\x00\x01('
- + b'\x00\x00\x00\x00\x00\x00\x010'
- + b'\x00\x00\x00\x00\x00\x00\x018'
- + b'\x00\x00\x00\x00\x00\x00\x01@'
- + b'\x00\x00\x00\x00\x00\x00\x01H'
- + b'\x00\x00\x00\x00\x00\x00\x01P'
- + b'\x00\x00\x00\x00\x00\x00\x01X'
- + b'\x00\x00\x00\x00\x00\x00\x01`'
- + b'\x00\x00\x00\x00\x00\x00\x01h'
- + b'\x00\x00\x00\x00\x00\x00\x01p'
- + b'\x00\x00\x00\x00\x00\x00\x01x'
- + b'\x00\x00\x00\x00\x00\x00\x01\x80'
- + b'\x00\x00\x00\x00\x00\x00\x01\x88'
- + b'\x00\x00\x00\x00\x00\x00\x01\x90'
- + b'\x00\x00\x00\x00\x00\x00\x01\x98'
- + b'\x00\x00\x00\x00\x00\x00\x01\xa0'
- + b'\x00\x00\x00\x00\x00\x00\x01\xa8'
- + b'\x00\x00\x00\x00\x00\x00\x01\xb0'
- + b'\x00\x00\x00\x00\x00\x00\x01\xb8'
- + b'\x00\x00\x00\x00\x00\x00\x01\xc0'
- + b'\x00\x00\x00\x00\x00\x00\x01\xc8'
- + b'\x00\x00\x00\x00\x00\x00\x01\xd0'
- + b'\x00\x00\x00\x00\x00\x00\x01\xd8'
- + b'\x00\x00\x00\x00\x00\x00\x01\xe0'
- + b'\x00\x00\x00\x00\x00\x00\x01\xe8'
- + b'\x00\x00\x00\x00\x00\x00\x01\xf0'
- + b'\x00\x00\x00\x00\x00\x00\x01\xf8')
-
+# The nbdkit pattern plugin exposes 64-bit numbers in bigendian order
+arr = array('q', range(0, 512, 8))
+if sys.byteorder == 'little':
+ arr.byteswap()
+expected = memoryview(arr).cast('B')
+stash = None
def chunk(user_data, buf2, offset, s, err):
print("in chunk, user_data %d" % user_data)
@@ -97,6 +40,8 @@ def chunk(user_data, buf2, offset, s, err):
assert buf2 == expected
assert offset == 0
assert s == nbd.READ_DATA
+ global stash
+ stash = buf2
def callback(user_data, err):
@@ -111,19 +56,29 @@ def callback(user_data, err):
# First try: succeed in both callbacks
-buf = nbd.Buffer(512)
+buf = bytearray(512)
cookie = h.aio_pread_structured(buf, 0,
lambda *args: chunk(42, *args),
lambda *args: callback((42, 42), *args))
while not h.aio_command_completed(cookie):
h.poll(-1)
-buf = buf.to_bytearray()
-
print("%r" % buf)
assert buf == expected
+# The callback can stash its slice; as long as that is live, we can't
+# resize buf but can view changes in buf through the slice
+try:
+ buf.pop()
+ assert False
+except BufferError:
+ pass
+buf[0] ^= 1
+assert buf == stash
+stash = None
+buf.pop()
+
# Second try: fail only during callback
buf = nbd.Buffer(512)
cookie = h.aio_pread_structured(buf, 0,
diff --git a/python/utils.c b/python/utils.c
index cd44b90..d5851f7 100644
--- a/python/utils.c
+++ b/python/utils.c
@@ -192,3 +192,37 @@ nbd_internal_py_wrap_errptr (int err)
return PyObject_CallMethod (py_ctypes_mod, "c_int", "i", err);
}
+
+/* Helper to compute view.toreadonly()[start:end] in chunk callback */
+PyObject *
+nbd_internal_py_get_subview (PyObject *view, const char *subbuf, size_t count)
+{
+ Py_buffer *orig;
+ const char *base;
+ PyObject *start, *end, *slice;
+ PyObject *ret;
+
+ assert (PyMemoryView_Check (view));
+ orig = PyMemoryView_GET_BUFFER (view);
+ assert (PyBuffer_IsContiguous (orig, 'A'));
+ base = orig->buf;
+ assert (subbuf >= base && count <= orig->len &&
+ subbuf + count <= base + orig->len);
+ start = PyLong_FromLong (subbuf - base);
+ if (!start) return NULL;
+ end = PyLong_FromLong (subbuf - base + count);
+ if (!end) { Py_DECREF (start); return NULL; }
+ slice = PySlice_New (start, end, NULL);
+ Py_DECREF (start);
+ Py_DECREF (end);
+ if (!slice) return NULL;
+ ret = PyObject_GetItem (view, slice);
+ Py_DECREF (slice);
+ /* memoryview.toreadonly() was only added in Python 3.8.
+ * PyMemoryView_GetContiguous (ret, PyBuf_READ, 'A') doesn't force
readonly.
+ * So we mess around directly with the Py_buffer.
+ */
+ if (ret)
+ PyMemoryView_GET_BUFFER (ret)->readonly = 1;
+ return ret;
+}
--
2.36.1
9:34 a.m.
New subject: [libnbd PATCH v3 5/5] python: Slice structured read callback buffer from original
On Thu, Jun 09, 2022 at 08:34:47AM -0500, Eric Blake wrote:
The Py_BuildValue "y#" format copies data; this is because
Python
assumes our C memory can go out of scope, while the user's python
callback can stash off whatever bytearray object it got. But this
copying is inefficient. Now that we already have a Python buffer-like
object in scope for the duration of all structured reads
(pread_structured since commit f8c76c01, aio_pread_structured since
commit 4f15d0e9), we know that the C memory parameter to our
chunk_callback is already (a portion of) the underlying contiguous
memory of an original Python object. If we pass the Python callback a
memoryview object over the correct slice of that memory, then Python
will correctly handle the intricacies of what happens if the callback
saves our memoryview object for later use, all without any data
copying overhead, making use of the pread_structured form much faster.
We basically want to code up the C equivalent to the python expression
'view = memoryview(buf).toreadonly()[start:end]', where start and end
are determined by comparing subbuf/count against the original buf.
But in C, that involves creating PyObject integers to form a PySlice,
then using PyObject_GetItem to apply the slice to a PyMemoryView (I
don't know of any faster tricks; googling does not find any quick
hits). Extract this work into a new utils.c helper function, to avoid
repetition (and in case we figure out a way to do it in fewer lines by
directly manipulating Py_buffer objects).
The testsuite is updated to cover examples of the data persistence
aspect, demonstrating that our stashed slice is still tied to the
returned Python buffer, even though the callback is long since
completed (prior to this patch, stash was an independent bytes copy,
and while it persisted on after the callback, it is frozen in time to
the contents of the buffer at the time the callback was reached). The
fact that data is now shared is is an API change, but one unlikely to
cause problems - it is very unusual for a callback function to try and
stash sub-buffers for later use (neither libnbd nor nbdkit was
utilizing that in their testsuites before this patch).
To demonstrate the speedup now that there is less copying involved per
callback, I tested:
$ export script='
def f(b,o,s,e):
pass
m=1024*1024
size=h.get_size()
h.set_pread_initialize(False)
for i in range(size // m):
buf = h.pread_structured(m, m*i, f)
buf = None
'
$ time ./run nbdkit -U - memory 10G --run 'nbdsh -u "$uri" -c
"$script"'
On my machine, this took 16.9s with libnbd 1.12.3, 15.5s pre-patch,
and 3.6s post-patch, for more than 4x speedup in the best case. Much
of the differences boil down to how efficient the Python garbage
collector is able to spot liveness of various buffers, to reuse rather
than allocate new buffers on the Python side; removing the 'buf =
None' line speeds up libnbd 1.12.3 to 4.5s (better reuse of the "y#"
memory in the callback when backed by C memory), 9.0s pre-patch
(copying "y#" from Python memory has scanning effects), and 3.0s
post-patch (zero-copying is always best).
A variation with aio_pread_structured reusing the same nbd.Buffer is
not quite as drastic: 3.9s with libnbd 1.12.3, 4.0s pre-patch, and
3.4s with this patch, but still shows that it is beneficial.
The corresponding diff to generated code is:
| --- python/methods.h.bak 2022-06-09 07:23:51.657260039 -0500
| +++ python/methods.h 2022-06-09 07:23:57.629266878 -0500
| @@ -36,6 +36,7 @@
| extern int nbd_internal_py_init_aio_buffer (PyObject *);
| extern PyObject *nbd_internal_py_get_nbd_buffer_type (void);
| extern PyObject *nbd_internal_py_wrap_errptr (int);
| +extern PyObject *nbd_internal_py_get_subview (PyObject *, const char *, size_t);
|
| static inline struct nbd_handle *
| get_handle (PyObject *obj)
| --- python/methods.c.bak 2022-06-09 07:23:51.652260034 -0500
| +++ python/methods.c 2022-06-09 07:23:57.644266895 -0500
| @@ -73,13 +73,15 @@ chunk_wrapper (void *user_data, const vo
|
| PyGILState_STATE py_save = PyGILState_UNLOCKED;
| PyObject *py_args, *py_ret;
| + PyObject *py_subbuf = NULL;
| PyObject *py_error = NULL;
|
| + py_subbuf = nbd_internal_py_get_subview (data->view, subbuf, count);
| + if (!py_subbuf) { PyErr_PrintEx (0); goto out; }
| py_error = nbd_internal_py_wrap_errptr (*error);
| if (!py_error) { PyErr_PrintEx (0); goto out; }
|
| - py_args = Py_BuildValue ("(y#KIO)", subbuf, (int) count, offset, status,
| - py_error);
| + py_args = Py_BuildValue ("(OKIO)", py_subbuf, offset, status, py_error);
| if (!py_args) { PyErr_PrintEx (0); goto out; }
|
| py_save = PyGILState_Ensure ();
| @@ -106,6 +108,7 @@ chunk_wrapper (void *user_data, const vo
| };
|
| out:
| + Py_XDECREF (py_subbuf);
| if (py_error) {
| PyObject *py_error_ret = PyObject_GetAttrString (py_error, "value");
| *error = PyLong_AsLong (py_error_ret);
| @@ -2231,6 +2234,7 @@ nbd_internal_py_pread_structured (PyObje
| /* Increment refcount since pointer may be saved by libnbd. */
| Py_INCREF (py_chunk_fn);
| chunk_user_data->fn = py_chunk_fn;
| + chunk_user_data->view = nbd_internal_py_get_aio_view (buf, PyBUF_WRITE);
|
| ret = nbd_pread_structured (h, PyByteArray_AS_STRING (buf), count,
| offset_u64, chunk, flags_u32);
| @@ -3116,6 +3120,7 @@ nbd_internal_py_aio_pread_structured (Py
| /* Increment refcount since pointer may be saved by libnbd. */
| Py_INCREF (py_chunk_fn);
| chunk_user_data->fn = py_chunk_fn;
| + chunk_user_data->view = nbd_internal_py_get_aio_view (buf, PyBUF_WRITE);
|
| if (nbd_internal_py_init_aio_buffer (buf) < 0) goto out;
| ret = nbd_aio_pread_structured (h, py_buf->buf, py_buf->len, offset_u64,
---
generator/Python.ml | 18 ++++--
python/t/405-pread-structured.py | 95 +++++++++---------------------
python/t/505-aio-pread-callback.py | 93 ++++++++---------------------
python/utils.c | 34 +++++++++++
4 files changed, 99 insertions(+), 141 deletions(-)
diff --git a/generator/Python.ml b/generator/Python.ml
index 975cab4..c424239 100644
--- a/generator/Python.ml
+++ b/generator/Python.ml
@@ -42,6 +42,7 @@ let
extern int nbd_internal_py_init_aio_buffer (PyObject *);
extern PyObject *nbd_internal_py_get_nbd_buffer_type (void);
extern PyObject *nbd_internal_py_wrap_errptr (int);
+extern PyObject *nbd_internal_py_get_subview (PyObject *, const char *, size_t);
static inline struct nbd_handle *
get_handle (PyObject *obj)
@@ -163,8 +164,8 @@ let
pr " PyObject *py_args, *py_ret;\n";
List.iter (
function
- | CBArrayAndLen (UInt32 n, _) ->
- pr " PyObject *py_%s = NULL;\n" n
+ | CBArrayAndLen (UInt32 n, _)
+ | CBBytesIn (n, _)
| CBMutable (Int n) ->
pr " PyObject *py_%s = NULL;\n" n
| _ -> ()
@@ -181,7 +182,9 @@ let
pr " if (!py_e_%s) { PyErr_PrintEx (0); goto out; }\n" n;
pr " PyList_SET_ITEM (py_%s, i_%s, py_e_%s);\n" n n n;
pr " }\n"
- | CBBytesIn _
+ | CBBytesIn (n, len) ->
+ pr " py_%s = nbd_internal_py_get_subview (data->view, %s, %s);\n" n
n len;
+ pr " if (!py_%s) { PyErr_PrintEx (0); goto out; }\n" n
| CBInt _
| CBInt64 _ -> ()
| CBMutable (Int n) ->
@@ -198,7 +201,7 @@ let
List.map (
function
| CBArrayAndLen (UInt32 n, _) -> "O", sprintf "py_%s" n
- | CBBytesIn (n, len) -> "y#", sprintf "%s, (int) %s" n len
+ | CBBytesIn (n, _) -> "O", sprintf "py_%s" n
| CBInt n -> "i", n
| CBInt64 n -> "L", n
| CBMutable (Int n) -> "O", sprintf "py_%s" n
@@ -250,6 +253,8 @@ let
function
| CBArrayAndLen (UInt32 n, _) ->
pr " Py_XDECREF (py_%s);\n" n
+ | CBBytesIn (n, _) ->
+ pr " Py_XDECREF (py_%s);\n" n
| CBMutable (Int n) ->
pr " if (py_%s) {\n" n;
pr " PyObject *py_%s_ret = PyObject_GetAttrString (py_%s,
\"value\");\n" n n;
@@ -257,7 +262,6 @@ let
pr " Py_DECREF (py_%s_ret);\n" n;
pr " Py_DECREF (py_%s);\n" n;
pr " }\n"
- | CBBytesIn _
| CBInt _ | CBInt64 _
| CBString _
| CBUInt _ | CBUInt64 _ -> ()
@@ -440,7 +444,9 @@ let
pr " }\n";
pr " /* Increment refcount since pointer may be saved by libnbd.
*/\n";
pr " Py_INCREF (py_%s_fn);\n" cbname;
- pr " %s_user_data->fn = py_%s_fn;\n" cbname cbname
+ pr " %s_user_data->fn = py_%s_fn;\n" cbname cbname;
+ if cbname = "chunk" then
+ pr " chunk_user_data->view = nbd_internal_py_get_aio_view (buf,
PyBUF_WRITE);\n"
| Enum _ -> ()
| Flags (n, _) -> pr " %s_u32 = %s;\n" n n
| Fd _ | Int _ -> ()
diff --git a/python/t/405-pread-structured.py b/python/t/405-pread-structured.py
index afd1e7e..9d3df68 100644
--- a/python/t/405-pread-structured.py
+++ b/python/t/405-pread-structured.py
@@ -1,5 +1,5 @@
# libnbd Python bindings
-# Copyright (C) 2010-2019 Red Hat Inc.
+# Copyright (C) 2010-2022 Red Hat Inc.
#
# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
@@ -17,76 +17,19 @@
import nbd
import errno
+import sys
+from array import array
h = nbd.NBD()
h.connect_command(["nbdkit", "-s", "--exit-with-parent",
"-v",
"pattern", "size=512"])
-expected = (b'\x00\x00\x00\x00\x00\x00\x00\x00'
- + b'\x00\x00\x00\x00\x00\x00\x00\x08'
- + b'\x00\x00\x00\x00\x00\x00\x00\x10'
- + b'\x00\x00\x00\x00\x00\x00\x00\x18'
- + b'\x00\x00\x00\x00\x00\x00\x00 '
- + b'\x00\x00\x00\x00\x00\x00\x00('
- + b'\x00\x00\x00\x00\x00\x00\x000'
- + b'\x00\x00\x00\x00\x00\x00\x008'
- + b'\x00\x00\x00\x00\x00\x00\x00@'
- + b'\x00\x00\x00\x00\x00\x00\x00H'
- + b'\x00\x00\x00\x00\x00\x00\x00P'
- + b'\x00\x00\x00\x00\x00\x00\x00X'
- + b'\x00\x00\x00\x00\x00\x00\x00`'
- + b'\x00\x00\x00\x00\x00\x00\x00h'
- + b'\x00\x00\x00\x00\x00\x00\x00p'
- + b'\x00\x00\x00\x00\x00\x00\x00x'
- + b'\x00\x00\x00\x00\x00\x00\x00\x80'
- + b'\x00\x00\x00\x00\x00\x00\x00\x88'
- + b'\x00\x00\x00\x00\x00\x00\x00\x90'
- + b'\x00\x00\x00\x00\x00\x00\x00\x98'
- + b'\x00\x00\x00\x00\x00\x00\x00\xa0'
- + b'\x00\x00\x00\x00\x00\x00\x00\xa8'
- + b'\x00\x00\x00\x00\x00\x00\x00\xb0'
- + b'\x00\x00\x00\x00\x00\x00\x00\xb8'
- + b'\x00\x00\x00\x00\x00\x00\x00\xc0'
- + b'\x00\x00\x00\x00\x00\x00\x00\xc8'
- + b'\x00\x00\x00\x00\x00\x00\x00\xd0'
- + b'\x00\x00\x00\x00\x00\x00\x00\xd8'
- + b'\x00\x00\x00\x00\x00\x00\x00\xe0'
- + b'\x00\x00\x00\x00\x00\x00\x00\xe8'
- + b'\x00\x00\x00\x00\x00\x00\x00\xf0'
- + b'\x00\x00\x00\x00\x00\x00\x00\xf8'
- + b'\x00\x00\x00\x00\x00\x00\x01\x00'
- + b'\x00\x00\x00\x00\x00\x00\x01\x08'
- + b'\x00\x00\x00\x00\x00\x00\x01\x10'
- + b'\x00\x00\x00\x00\x00\x00\x01\x18'
- + b'\x00\x00\x00\x00\x00\x00\x01 '
- + b'\x00\x00\x00\x00\x00\x00\x01('
- + b'\x00\x00\x00\x00\x00\x00\x010'
- + b'\x00\x00\x00\x00\x00\x00\x018'
- + b'\x00\x00\x00\x00\x00\x00\x01@'
- + b'\x00\x00\x00\x00\x00\x00\x01H'
- + b'\x00\x00\x00\x00\x00\x00\x01P'
- + b'\x00\x00\x00\x00\x00\x00\x01X'
- + b'\x00\x00\x00\x00\x00\x00\x01`'
- + b'\x00\x00\x00\x00\x00\x00\x01h'
- + b'\x00\x00\x00\x00\x00\x00\x01p'
- + b'\x00\x00\x00\x00\x00\x00\x01x'
- + b'\x00\x00\x00\x00\x00\x00\x01\x80'
- + b'\x00\x00\x00\x00\x00\x00\x01\x88'
- + b'\x00\x00\x00\x00\x00\x00\x01\x90'
- + b'\x00\x00\x00\x00\x00\x00\x01\x98'
- + b'\x00\x00\x00\x00\x00\x00\x01\xa0'
- + b'\x00\x00\x00\x00\x00\x00\x01\xa8'
- + b'\x00\x00\x00\x00\x00\x00\x01\xb0'
- + b'\x00\x00\x00\x00\x00\x00\x01\xb8'
- + b'\x00\x00\x00\x00\x00\x00\x01\xc0'
- + b'\x00\x00\x00\x00\x00\x00\x01\xc8'
- + b'\x00\x00\x00\x00\x00\x00\x01\xd0'
- + b'\x00\x00\x00\x00\x00\x00\x01\xd8'
- + b'\x00\x00\x00\x00\x00\x00\x01\xe0'
- + b'\x00\x00\x00\x00\x00\x00\x01\xe8'
- + b'\x00\x00\x00\x00\x00\x00\x01\xf0'
- + b'\x00\x00\x00\x00\x00\x00\x01\xf8')
-
+# The nbdkit pattern plugin exposes 64-bit numbers in bigendian order
+arr = array('q', range(0, 512, 8))
+if sys.byteorder == 'little':
+ arr.byteswap()
+expected = memoryview(arr).cast('B')
+stash = None
def f(user_data, buf2, offset, s, err):
assert err.value == 0
@@ -94,8 +37,15 @@ def f(user_data, buf2, offset, s, err):
if user_data != 42:
raise ValueError('unexpected user_data')
assert buf2 == expected
+ try:
+ buf2[0] = 1
+ assert False
+ except TypeError:
+ pass
assert offset == 0
assert s == nbd.READ_DATA
+ global stash
+ stash = buf2
buf = h.pread_structured(512, 0, lambda *args: f(42, *args))
@@ -104,6 +54,19 @@ print("%r" % buf)
assert buf == expected
+# The callback can stash its slice; as long as that is live, we can't
+# resize buf but can view changes in buf through the slice
+try:
+ buf.pop()
+ assert False
+except BufferError:
+ pass
+buf[0] ^= 1
+assert buf == stash
+stash = None
+buf.pop()
+
+# Tests of error handling
buf = h.pread_structured(512, 0, lambda *args: f(42, *args),
nbd.CMD_FLAG_DF)
diff --git a/python/t/505-aio-pread-callback.py b/python/t/505-aio-pread-callback.py
index 1773aee..5c7b250 100644
--- a/python/t/505-aio-pread-callback.py
+++ b/python/t/505-aio-pread-callback.py
@@ -1,5 +1,5 @@
# libnbd Python bindings
-# Copyright (C) 2010-2019 Red Hat Inc.
+# Copyright (C) 2010-2022 Red Hat Inc.
#
# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
@@ -17,76 +17,19 @@
import nbd
import errno
+import sys
+from array import array
h = nbd.NBD()
h.connect_command(["nbdkit", "-s", "--exit-with-parent",
"-v",
"pattern", "size=512"])
-expected = (b'\x00\x00\x00\x00\x00\x00\x00\x00'
- + b'\x00\x00\x00\x00\x00\x00\x00\x08'
- + b'\x00\x00\x00\x00\x00\x00\x00\x10'
- + b'\x00\x00\x00\x00\x00\x00\x00\x18'
- + b'\x00\x00\x00\x00\x00\x00\x00 '
- + b'\x00\x00\x00\x00\x00\x00\x00('
- + b'\x00\x00\x00\x00\x00\x00\x000'
- + b'\x00\x00\x00\x00\x00\x00\x008'
- + b'\x00\x00\x00\x00\x00\x00\x00@'
- + b'\x00\x00\x00\x00\x00\x00\x00H'
- + b'\x00\x00\x00\x00\x00\x00\x00P'
- + b'\x00\x00\x00\x00\x00\x00\x00X'
- + b'\x00\x00\x00\x00\x00\x00\x00`'
- + b'\x00\x00\x00\x00\x00\x00\x00h'
- + b'\x00\x00\x00\x00\x00\x00\x00p'
- + b'\x00\x00\x00\x00\x00\x00\x00x'
- + b'\x00\x00\x00\x00\x00\x00\x00\x80'
- + b'\x00\x00\x00\x00\x00\x00\x00\x88'
- + b'\x00\x00\x00\x00\x00\x00\x00\x90'
- + b'\x00\x00\x00\x00\x00\x00\x00\x98'
- + b'\x00\x00\x00\x00\x00\x00\x00\xa0'
- + b'\x00\x00\x00\x00\x00\x00\x00\xa8'
- + b'\x00\x00\x00\x00\x00\x00\x00\xb0'
- + b'\x00\x00\x00\x00\x00\x00\x00\xb8'
- + b'\x00\x00\x00\x00\x00\x00\x00\xc0'
- + b'\x00\x00\x00\x00\x00\x00\x00\xc8'
- + b'\x00\x00\x00\x00\x00\x00\x00\xd0'
- + b'\x00\x00\x00\x00\x00\x00\x00\xd8'
- + b'\x00\x00\x00\x00\x00\x00\x00\xe0'
- + b'\x00\x00\x00\x00\x00\x00\x00\xe8'
- + b'\x00\x00\x00\x00\x00\x00\x00\xf0'
- + b'\x00\x00\x00\x00\x00\x00\x00\xf8'
- + b'\x00\x00\x00\x00\x00\x00\x01\x00'
- + b'\x00\x00\x00\x00\x00\x00\x01\x08'
- + b'\x00\x00\x00\x00\x00\x00\x01\x10'
- + b'\x00\x00\x00\x00\x00\x00\x01\x18'
- + b'\x00\x00\x00\x00\x00\x00\x01 '
- + b'\x00\x00\x00\x00\x00\x00\x01('
- + b'\x00\x00\x00\x00\x00\x00\x010'
- + b'\x00\x00\x00\x00\x00\x00\x018'
- + b'\x00\x00\x00\x00\x00\x00\x01@'
- + b'\x00\x00\x00\x00\x00\x00\x01H'
- + b'\x00\x00\x00\x00\x00\x00\x01P'
- + b'\x00\x00\x00\x00\x00\x00\x01X'
- + b'\x00\x00\x00\x00\x00\x00\x01`'
- + b'\x00\x00\x00\x00\x00\x00\x01h'
- + b'\x00\x00\x00\x00\x00\x00\x01p'
- + b'\x00\x00\x00\x00\x00\x00\x01x'
- + b'\x00\x00\x00\x00\x00\x00\x01\x80'
- + b'\x00\x00\x00\x00\x00\x00\x01\x88'
- + b'\x00\x00\x00\x00\x00\x00\x01\x90'
- + b'\x00\x00\x00\x00\x00\x00\x01\x98'
- + b'\x00\x00\x00\x00\x00\x00\x01\xa0'
- + b'\x00\x00\x00\x00\x00\x00\x01\xa8'
- + b'\x00\x00\x00\x00\x00\x00\x01\xb0'
- + b'\x00\x00\x00\x00\x00\x00\x01\xb8'
- + b'\x00\x00\x00\x00\x00\x00\x01\xc0'
- + b'\x00\x00\x00\x00\x00\x00\x01\xc8'
- + b'\x00\x00\x00\x00\x00\x00\x01\xd0'
- + b'\x00\x00\x00\x00\x00\x00\x01\xd8'
- + b'\x00\x00\x00\x00\x00\x00\x01\xe0'
- + b'\x00\x00\x00\x00\x00\x00\x01\xe8'
- + b'\x00\x00\x00\x00\x00\x00\x01\xf0'
- + b'\x00\x00\x00\x00\x00\x00\x01\xf8')
-
+# The nbdkit pattern plugin exposes 64-bit numbers in bigendian order
+arr = array('q', range(0, 512, 8))
+if sys.byteorder == 'little':
+ arr.byteswap()
+expected = memoryview(arr).cast('B')
+stash = None
def chunk(user_data, buf2, offset, s, err):
print("in chunk, user_data %d" % user_data)
@@ -97,6 +40,8 @@ def chunk(user_data, buf2, offset, s, err):
assert buf2 == expected
assert offset == 0
assert s == nbd.READ_DATA
+ global stash
+ stash = buf2
def callback(user_data, err):
@@ -111,19 +56,29 @@ def callback(user_data, err):
# First try: succeed in both callbacks
-buf = nbd.Buffer(512)
+buf = bytearray(512)
cookie = h.aio_pread_structured(buf, 0,
lambda *args: chunk(42, *args),
lambda *args: callback((42, 42), *args))
while not h.aio_command_completed(cookie):
h.poll(-1)
-buf = buf.to_bytearray()
-
print("%r" % buf)
assert buf == expected
+# The callback can stash its slice; as long as that is live, we can't
+# resize buf but can view changes in buf through the slice
+try:
+ buf.pop()
+ assert False
+except BufferError:
+ pass
+buf[0] ^= 1
+assert buf == stash
+stash = None
+buf.pop()
+
# Second try: fail only during callback
buf = nbd.Buffer(512)
cookie = h.aio_pread_structured(buf, 0,
diff --git a/python/utils.c b/python/utils.c
index cd44b90..d5851f7 100644
--- a/python/utils.c
+++ b/python/utils.c
@@ -192,3 +192,37 @@ nbd_internal_py_wrap_errptr (int err)
return PyObject_CallMethod (py_ctypes_mod, "c_int", "i", err);
}
+
+/* Helper to compute view.toreadonly()[start:end] in chunk callback */
+PyObject *
+nbd_internal_py_get_subview (PyObject *view, const char *subbuf, size_t count)
+{
+ Py_buffer *orig;
+ const char *base;
+ PyObject *start, *end, *slice;
+ PyObject *ret;
+
+ assert (PyMemoryView_Check (view));
+ orig = PyMemoryView_GET_BUFFER (view);
+ assert (PyBuffer_IsContiguous (orig, 'A'));
+ base = orig->buf;
+ assert (subbuf >= base && count <= orig->len &&
+ subbuf + count <= base + orig->len);
+ start = PyLong_FromLong (subbuf - base);
+ if (!start) return NULL;
+ end = PyLong_FromLong (subbuf - base + count);
+ if (!end) { Py_DECREF (start); return NULL; }
+ slice = PySlice_New (start, end, NULL);
+ Py_DECREF (start);
+ Py_DECREF (end);
+ if (!slice) return NULL;
+ ret = PyObject_GetItem (view, slice);
+ Py_DECREF (slice);
+ /* memoryview.toreadonly() was only added in Python 3.8.
+ * PyMemoryView_GetContiguous (ret, PyBuf_READ, 'A') doesn't force
readonly.
+ * So we mess around directly with the Py_buffer.
+ */
+ if (ret)
+ PyMemoryView_GET_BUFFER (ret)->readonly = 1;
+ return ret;
+}
Acked-by: Richard W.M. Jones <rjones(a)redhat.com>
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
2:09 p.m.
New subject: [libnbd PATCH v3 5/5] python: Slice structured read callback buffer from original
On Thu, Jun 09, 2022 at 03:34:19PM +0100, Richard W.M. Jones wrote:
On Thu, Jun 09, 2022 at 08:34:47AM -0500, Eric Blake wrote:
> The Py_BuildValue "y#" format copies data; this is because Python
> assumes our C memory can go out of scope, while the user's python
> callback can stash off whatever bytearray object it got. But this
> copying is inefficient. Now that we already have a Python buffer-like
> object in scope for the duration of all structured reads
> (pread_structured since commit f8c76c01, aio_pread_structured since
> commit 4f15d0e9), we know that the C memory parameter to our
> chunk_callback is already (a portion of) the underlying contiguous
> memory of an original Python object. If we pass the Python callback a
> memoryview object over the correct slice of that memory, then Python
> will correctly handle the intricacies of what happens if the callback
> saves our memoryview object for later use, all without any data
> copying overhead, making use of the pread_structured form much faster.
>
Acked-by: Richard W.M. Jones <rjones(a)redhat.com>
Series is now pushed as 67dd054..8bb89d6
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
898
days inactive
899
days old
17 comments
2 participants
participants (2)
-
Eric Blake -
Richard W.M. Jones