Libguestfs - Nov 2019 - [PATCH nbdkit] python: Pass memoryview to pwrite()

Passing a memoryview we avoid unneeded copy of the original buffer. On
the python side memoryview object can be used for slicing, writing to
file, or sending to socket.

This may break plugins assuming that the they get a bytearray, but
good python code should not care about the type of the buffer, only
about the behaviour.

Testing with a plugin writing to /dev/null shows 2.7x speedup. Real
plugin will probably show much smaller improvement.

Without patch:

$ time qemu-img convert -p -f raw -O raw -n /var/tmp/disk.img nbd://localhost/
    (100.00/100%)

real	0m1.284s
user	0m0.091s
sys	0m0.576s

With patch:

$ time qemu-img convert -p -f raw -O raw -n /var/tmp/disk.img nbd://localhost/
    (100.00/100%)

real	0m0.477s
user	0m0.078s
sys	0m0.653s
---

More info on how I tested this:

# Creating test image
$ dd if=/dev/zero bs=1M count=1024 | tr "\0" "U" >
/var/tmp/disk.img

$ cat zero.py
import builtins

def open(readonly):
    return builtins.open("/dev/zero", "r+b")

def get_size(h):
    return 1024**3

def pwrite(h, buf, offset):
    h.write(buf)

def pread(h, count, offset):
    raise NotImplementedError

def close(h):
    h.close()

# Running nbdkit
$ ./nbdkit -f -v python zero.py

 plugins/python/python.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/plugins/python/python.c b/plugins/python/python.c
index 214fffb..2b4361a 100644
--- a/plugins/python/python.c
+++ b/plugins/python/python.c
@@ -496,8 +496,8 @@ py_pwrite (void *handle, const void *buf,
     PyErr_Clear ();
 
     r = PyObject_CallFunction (fn, "ONL", obj,
-                               PyByteArray_FromStringAndSize (buf, count),
-                               offset, NULL);
+            PyMemoryView_FromMemory ((char *)buf, count, PyBUF_READ),
+            offset, NULL);
     Py_DECREF (fn);
     if (check_python_failure ("pwrite") == -1)
       return -1;
-- 
2.21.0

On Sat, Nov 23, 2019 at 5:49 PM Nir Soffer <nirsof@gmail.com>
wrote:
>
> Passing a memoryview we avoid unneeded copy of the original buffer. On
> the python side memoryview object can be used for slicing, writing to
> file, or sending to socket.
>
> This may break plugins assuming that the they get a bytearray, but
> good python code should not care about the type of the buffer, only
> about the behaviour.
>
> Testing with a plugin writing to /dev/null shows 2.7x speedup. Real
> plugin will probably show much smaller improvement.
I tested this with v2v, importing fedora 30 vm from local file to
ovirt, and it gives
about a 10% improvement in transfer time.
> Without patch:
>
> $ time qemu-img convert -p -f raw -O raw -n /var/tmp/disk.img
nbd://localhost/
>     (100.00/100%)
>
> real    0m1.284s
> user    0m0.091s
> sys     0m0.576s
>
> With patch:
>
> $ time qemu-img convert -p -f raw -O raw -n /var/tmp/disk.img
nbd://localhost/
>     (100.00/100%)
>
> real    0m0.477s
> user    0m0.078s
> sys     0m0.653s
> ---
>
> More info on how I tested this:
>
> # Creating test image
> $ dd if=/dev/zero bs=1M count=1024 | tr "\0" "U" >
/var/tmp/disk.img
>
> $ cat zero.py
> import builtins
>
> def open(readonly):
>     return builtins.open("/dev/zero", "r+b")
>
> def get_size(h):
>     return 1024**3
>
> def pwrite(h, buf, offset):
>     h.write(buf)
>
> def pread(h, count, offset):
>     raise NotImplementedError
>
> def close(h):
>     h.close()
>
> # Running nbdkit
> $ ./nbdkit -f -v python zero.py
>
>  plugins/python/python.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/plugins/python/python.c b/plugins/python/python.c
> index 214fffb..2b4361a 100644
> --- a/plugins/python/python.c
> +++ b/plugins/python/python.c
> @@ -496,8 +496,8 @@ py_pwrite (void *handle, const void *buf,
>      PyErr_Clear ();
>
>      r = PyObject_CallFunction (fn, "ONL", obj,
> -                               PyByteArray_FromStringAndSize (buf, count),
> -                               offset, NULL);
> +            PyMemoryView_FromMemory ((char *)buf, count, PyBUF_READ),
> +            offset, NULL);
>      Py_DECREF (fn);
>      if (check_python_failure ("pwrite") == -1)
>        return -1;
> --
> 2.21.0
>

On Sat, Nov 23, 2019 at 05:49:05PM +0200, Nir Soffer
wrote:
> Passing a memoryview we avoid unneeded copy of the original buffer. On
> the python side memoryview object can be used for slicing, writing to
> file, or sending to socket.
> 
> This may break plugins assuming that the they get a bytearray, but
> good python code should not care about the type of the buffer, only
> about the behaviour.
We can consider breaking changes for the v2 API, so logically it would
be best if this patch was applied on top of my new API series.

Rich.
> Testing with a plugin writing to /dev/null shows 2.7x speedup. Real
> plugin will probably show much smaller improvement.
> 
> Without patch:
> 
> $ time qemu-img convert -p -f raw -O raw -n /var/tmp/disk.img
nbd://localhost/
>     (100.00/100%)
> 
> real	0m1.284s
> user	0m0.091s
> sys	0m0.576s
> 
> With patch:
> 
> $ time qemu-img convert -p -f raw -O raw -n /var/tmp/disk.img
nbd://localhost/
>     (100.00/100%)
> 
> real	0m0.477s
> user	0m0.078s
> sys	0m0.653s
> ---
> 
> More info on how I tested this:
> 
> # Creating test image
> $ dd if=/dev/zero bs=1M count=1024 | tr "\0" "U" >
/var/tmp/disk.img
> 
> $ cat zero.py
> import builtins
> 
> def open(readonly):
>     return builtins.open("/dev/zero", "r+b")
> 
> def get_size(h):
>     return 1024**3
> 
> def pwrite(h, buf, offset):
>     h.write(buf)
> 
> def pread(h, count, offset):
>     raise NotImplementedError
> 
> def close(h):
>     h.close()
> 
> # Running nbdkit
> $ ./nbdkit -f -v python zero.py
> 
>  plugins/python/python.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/plugins/python/python.c b/plugins/python/python.c
> index 214fffb..2b4361a 100644
> --- a/plugins/python/python.c
> +++ b/plugins/python/python.c
> @@ -496,8 +496,8 @@ py_pwrite (void *handle, const void *buf,
>      PyErr_Clear ();
>  
>      r = PyObject_CallFunction (fn, "ONL", obj,
> -                               PyByteArray_FromStringAndSize (buf, count),
> -                               offset, NULL);
> +            PyMemoryView_FromMemory ((char *)buf, count, PyBUF_READ),
> +            offset, NULL);
>      Py_DECREF (fn);
>      if (check_python_failure ("pwrite") == -1)
>        return -1;
> -- 
> 2.21.0
-- 
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

On Sat, Nov 23, 2019 at 05:49:05PM +0200, Nir Soffer
wrote:
> Passing a memoryview we avoid unneeded copy of the original buffer. On
> the python side memoryview object can be used for slicing, writing to
> file, or sending to socket.
> 
> This may break plugins assuming that the they get a bytearray, but
> good python code should not care about the type of the buffer, only
> about the behaviour.
> 
> Testing with a plugin writing to /dev/null shows 2.7x speedup. Real
> plugin will probably show much smaller improvement.
> 
> Without patch:
> 
> $ time qemu-img convert -p -f raw -O raw -n /var/tmp/disk.img
nbd://localhost/
>     (100.00/100%)
> 
> real	0m1.284s
> user	0m0.091s
> sys	0m0.576s
> 
> With patch:
> 
> $ time qemu-img convert -p -f raw -O raw -n /var/tmp/disk.img
nbd://localhost/
>     (100.00/100%)
> 
> real	0m0.477s
> user	0m0.078s
> sys	0m0.653s
> ---
> 
> More info on how I tested this:
> 
> # Creating test image
> $ dd if=/dev/zero bs=1M count=1024 | tr "\0" "U" >
/var/tmp/disk.img
> 
> $ cat zero.py
> import builtins
> 
> def open(readonly):
>     return builtins.open("/dev/zero", "r+b")
> 
> def get_size(h):
>     return 1024**3
> 
> def pwrite(h, buf, offset):
>     h.write(buf)
> 
> def pread(h, count, offset):
>     raise NotImplementedError
> 
> def close(h):
>     h.close()
> 
> # Running nbdkit
> $ ./nbdkit -f -v python zero.py
> 
>  plugins/python/python.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/plugins/python/python.c b/plugins/python/python.c
> index 214fffb..2b4361a 100644
> --- a/plugins/python/python.c
> +++ b/plugins/python/python.c
> @@ -496,8 +496,8 @@ py_pwrite (void *handle, const void *buf,
>      PyErr_Clear ();
>  
>      r = PyObject_CallFunction (fn, "ONL", obj,
> -                               PyByteArray_FromStringAndSize (buf, count),
> -                               offset, NULL);
> +            PyMemoryView_FromMemory ((char *)buf, count, PyBUF_READ),
> +            offset, NULL);
>      Py_DECREF (fn);
>      if (check_python_failure ("pwrite") == -1)
>        return -1;
I had a second look at this patch this morning, and at how Python 3
memoryview works, and I think we should accept this.  Callers (even v1
callers) should be prepared for any buffer type object.  It also seems
to work with v2v.

Therefore ACK.

Thanks,

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