Eric Blake
2022-Jun-14 12:58 UTC
[Libguestfs] [libnbd PATCH] python: Allow control over copy/share of nbd.Buffer
Add new methods nbd.Buffer.{to,from}_buffer that avoid the copying present in the existing nbd.Buffer.{to,from}_bytearray. The reduction in copies possible by this approach is no longer quite as necessary, now that aio_p{read,write} have been converted to take buffers directly. However, there is still one (marginal) benefit: if you use h.set_pread_initialize(False), and create a new buffer for every I/O (rather than reusing a buffer pool), nbd.Buffer(n) handed to h.aio_pread then buf.to_buffer() is slightly faster than handing a bytearray(n) directly to h.aio_pread, because we are able to skip the step of pre-zeroing the buffer. --- Instead of adding a copy=True/False parameter to the existing API as I had previously suggested, I decided that a new API made more sense. generator/Python.ml | 39 ++++++++++++++++++++++---- python/t/585-aio-buffer-share.py | 47 ++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 6 deletions(-) create mode 100644 python/t/585-aio-buffer-share.py diff --git a/generator/Python.ml b/generator/Python.ml index 03a7e6b..cb89ccd 100644 --- a/generator/Python.ml +++ b/generator/Python.ml @@ -735,6 +735,21 @@ let '''Allocate an uninitialized AIO buffer used for nbd.aio_pread.''' self._o = libnbdmod.alloc_aio_buffer(len) + @classmethod + def from_buffer(cls, buf): + '''Create an AIO buffer that shares an existing buffer-like object. + + Because the buffer is shared, changes to the original are visible + to nbd.aio_pwrite, and changes in nbd.aio_pread are visible to the + original. + ''' + self = cls(0) + # Ensure that buf is already buffer-like + with memoryview(buf): + self._o = buf + self._init = True + return self + @classmethod def from_bytearray(cls, ba): '''Create an AIO buffer from a bytearray or other buffer-like object. @@ -743,16 +758,28 @@ let bytearray constructor. Otherwise, ba is copied. Either way, the resulting AIO buffer is independent from the original. ''' - self = cls(0) - self._o = bytearray(ba) - self._init = True - return self + return cls.from_buffer(bytearray(ba)) - def to_bytearray(self): - '''Copy an AIO buffer into a bytearray.''' + def to_buffer(self): + '''Return a shared view of the AIO buffer contents. + + This exposes the underlying buffer; changes to the buffer are + visible to nbd.aio_pwrite, and changes from nbd.aio_pread are + visible in the buffer. + ''' if not hasattr(self, '_init'): self._o = bytearray(len(self._o)) self._init = True + return self._o + + def to_bytearray(self): + '''Copy an AIO buffer into a bytearray. + + This copies the contents of an AIO buffer to a new bytearray, which + remains independent from the original. + ''' + if not hasattr(self, '_init'): + return bytearray(len(self._o)) return bytearray(self._o) def size(self): diff --git a/python/t/585-aio-buffer-share.py b/python/t/585-aio-buffer-share.py new file mode 100644 index 0000000..21752b7 --- /dev/null +++ b/python/t/585-aio-buffer-share.py @@ -0,0 +1,47 @@ +# libnbd Python bindings +# 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 +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + +# This tests the nbd.Buffer is_zero method. We can do this entirely +# without using the NBD protocol. + +import nbd + +# Use of to/from_bytearray always creates copies +ba = bytearray(512) +buf = nbd.Buffer.from_bytearray(ba) +ba.append(1) +assert len(ba) == 513 +assert len(buf) == 512 +assert buf.is_zero() +assert buf.to_bytearray() is not ba + +# Use of to/from_buffer shares the same buffer +buf = nbd.Buffer.from_buffer(ba) +assert not buf.is_zero() +assert len(buf) == 513 +ba.pop() +assert buf.is_zero() +assert len(buf) == 512 +assert buf.to_buffer() is ba + +# Even though nbd.Buffer(n) start uninitialized, we sanitize before exporting. +# This test cheats and examines the private member ._init +buf = nbd.Buffer(512) +assert buf.is_zero() +assert not hasattr(buf, '_init') +assert buf.to_buffer() == bytearray(512) +assert hasattr(buf, '_init') -- 2.36.1
Richard W.M. Jones
2022-Jun-14 14:39 UTC
[Libguestfs] [libnbd PATCH] python: Allow control over copy/share of nbd.Buffer
On Tue, Jun 14, 2022 at 07:58:08AM -0500, Eric Blake wrote:> Add new methods nbd.Buffer.{to,from}_buffer that avoid the copying > present in the existing nbd.Buffer.{to,from}_bytearray. The reduction > in copies possible by this approach is no longer quite as necessary, > now that aio_p{read,write} have been converted to take buffers > directly. However, there is still one (marginal) benefit: if you use > h.set_pread_initialize(False), and create a new buffer for every I/O > (rather than reusing a buffer pool), nbd.Buffer(n) handed to > h.aio_pread then buf.to_buffer() is slightly faster than handing a > bytearray(n) directly to h.aio_pread, because we are able to skip the > step of pre-zeroing the buffer. > --- > > Instead of adding a copy=True/False parameter to the existing API as I > had previously suggested, I decided that a new API made more sense. > > generator/Python.ml | 39 ++++++++++++++++++++++---- > python/t/585-aio-buffer-share.py | 47 ++++++++++++++++++++++++++++++++ > 2 files changed, 80 insertions(+), 6 deletions(-) > create mode 100644 python/t/585-aio-buffer-share.py > > diff --git a/generator/Python.ml b/generator/Python.ml > index 03a7e6b..cb89ccd 100644 > --- a/generator/Python.ml > +++ b/generator/Python.ml > @@ -735,6 +735,21 @@ let > '''Allocate an uninitialized AIO buffer used for nbd.aio_pread.''' > self._o = libnbdmod.alloc_aio_buffer(len) > > + @classmethod > + def from_buffer(cls, buf): > + '''Create an AIO buffer that shares an existing buffer-like object. > + > + Because the buffer is shared, changes to the original are visible > + to nbd.aio_pwrite, and changes in nbd.aio_pread are visible to the > + original. > + ''' > + self = cls(0) > + # Ensure that buf is already buffer-like > + with memoryview(buf): > + self._o = buf > + self._init = True > + return self > + > @classmethod > def from_bytearray(cls, ba): > '''Create an AIO buffer from a bytearray or other buffer-like object. > @@ -743,16 +758,28 @@ let > bytearray constructor. Otherwise, ba is copied. Either way, the > resulting AIO buffer is independent from the original. > ''' > - self = cls(0) > - self._o = bytearray(ba) > - self._init = True > - return self > + return cls.from_buffer(bytearray(ba)) > > - def to_bytearray(self): > - '''Copy an AIO buffer into a bytearray.''' > + def to_buffer(self): > + '''Return a shared view of the AIO buffer contents. > + > + This exposes the underlying buffer; changes to the buffer are > + visible to nbd.aio_pwrite, and changes from nbd.aio_pread are > + visible in the buffer. > + ''' > if not hasattr(self, '_init'): > self._o = bytearray(len(self._o)) > self._init = True > + return self._o > + > + def to_bytearray(self): > + '''Copy an AIO buffer into a bytearray. > + > + This copies the contents of an AIO buffer to a new bytearray, which > + remains independent from the original. > + ''' > + if not hasattr(self, '_init'): > + return bytearray(len(self._o)) > return bytearray(self._o) > > def size(self): > diff --git a/python/t/585-aio-buffer-share.py b/python/t/585-aio-buffer-share.py > new file mode 100644 > index 0000000..21752b7 > --- /dev/null > +++ b/python/t/585-aio-buffer-share.py > @@ -0,0 +1,47 @@ > +# libnbd Python bindings > +# 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 > +# the Free Software Foundation; either version 2 of the License, or > +# (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program; if not, write to the Free Software > +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA > + > +# This tests the nbd.Buffer is_zero method. We can do this entirely > +# without using the NBD protocol. > + > +import nbd > + > +# Use of to/from_bytearray always creates copies > +ba = bytearray(512) > +buf = nbd.Buffer.from_bytearray(ba) > +ba.append(1) > +assert len(ba) == 513 > +assert len(buf) == 512 > +assert buf.is_zero() > +assert buf.to_bytearray() is not ba > + > +# Use of to/from_buffer shares the same buffer > +buf = nbd.Buffer.from_buffer(ba) > +assert not buf.is_zero() > +assert len(buf) == 513 > +ba.pop() > +assert buf.is_zero() > +assert len(buf) == 512 > +assert buf.to_buffer() is ba > + > +# Even though nbd.Buffer(n) start uninitialized, we sanitize before exporting. > +# This test cheats and examines the private member ._init > +buf = nbd.Buffer(512) > +assert buf.is_zero() > +assert not hasattr(buf, '_init') > +assert buf.to_buffer() == bytearray(512) > +assert hasattr(buf, '_init') > -- > 2.36.1Acked-by: Richard W.M. Jones <rjones at redhat.com> Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v