[PATCH libnbd v2 0/9] golang: Safer, easier to use, and faster AioBuffer
Improve AioBuffer to make it safer, easier to use, and faster when integrating with other Go APIs. New Go specific APIs: - MakeAioBufferZero() - creates a new buffer using calloc(), to make it easy and efficient to use a zeroed buffer. - AioBuffer.Slice() - create a slice backed by the underlying buffer without copying the contents of the buffer. Performance improments: - FromBytes() is 3 time faster - Code using Bytes() should use Slice() now. aio_copy example shows up to 260% speedup. Improve testing: - New AioBuffer tests - New AioBuffer benchmarks Documention: - AioBuffer is fully documnted now. Changes since v1: - Rename the new test to libnbd_020_ to match the current test numbering semantics (Eric) - We run the benchmarks in make check using very short timeout to keep them working without overloading the CI. (Eric) - Update copyright year (Eric) - Fix many typos in comments and commit messages (Eric) v1 was here: https://listman.redhat.com/archives/libguestfs/2022-January/msg00218.html Nir Soffer (9): golang: tests: Add test for AioBuffer golang: aio_buffer.go: Make it safer to use golang: aio_buffer.go: Add missing documentation golang: aio_buffer.go: Add MakeAioBufferZero() golang: aio_buffer.go: Add Slice() golang: tests: Use AioBuffer.Slice() golang: aio_buffer.go: Speed up FromBytes() golang: aio_buffer.go: Benchmark copy flows golang: examples: aio_copy: Simplify using AioBuffer golang/Makefile.am | 1 + golang/aio_buffer.go | 39 ++++- golang/examples/aio_copy/aio_copy.go | 29 +--- golang/libnbd_020_aio_buffer_test.go | 236 +++++++++++++++++++++++++++ golang/libnbd_500_aio_pread_test.go | 2 +- golang/libnbd_510_aio_pwrite_test.go | 8 +- golang/run-tests.sh | 5 + 7 files changed, 286 insertions(+), 34 deletions(-) create mode 100644 golang/libnbd_020_aio_buffer_test.go -- 2.34.1
Add unit tests and benchmarks for AioBuffer. The tests are trivial but they server as running documentation, and they point out important details about the type. The benchmarks show the efficiency of allocating a new buffer, zeroing it, and interfacing with Go code. These tests will also ensure that we don't break anything by the next changes. To run the benchmarks use: $ go test -run=xxx -bench=. goos: linux goarch: amd64 pkg: libguestfs.org/libnbd cpu: Intel(R) Core(TM) i7-10850H CPU @ 2.70GHz BenchmarkMakeAioBuffer-12 6871759 157.2 ns/op BenchmarkAioBufferZero-12 17551 69552 ns/op BenchmarkFromBytes-12 9632 139112 ns/op BenchmarkAioBufferBytes-12 69375 16410 ns/op PASS ok libguestfs.org/libnbd 5.843s To make sure the benchmarks will not break, we run them in "make check" with a very short timeout. For actual performance testing run "go test" directly. Signed-off-by: Nir Soffer <nsoffer@redhat.com> --- golang/Makefile.am | 1 + golang/libnbd_020_aio_buffer_test.go | 104 +++++++++++++++++++++++++++ golang/run-tests.sh | 5 ++ 3 files changed, 110 insertions(+) create mode 100644 golang/libnbd_020_aio_buffer_test.go diff --git a/golang/Makefile.am b/golang/Makefile.am index 10fb8934..f170cbc4 100644 --- a/golang/Makefile.am +++ b/golang/Makefile.am @@ -19,20 +19,21 @@ include $(top_srcdir)/subdir-rules.mk source_files = \ aio_buffer.go \ bindings.go \ callbacks.go \ closures.go \ handle.go \ wrappers.go \ wrappers.h \ libnbd_010_load_test.go \ + libnbd_020_aio_buffer_test.go \ libnbd_100_handle_test.go \ libnbd_110_defaults_test.go \ libnbd_120_set_non_defaults_test.go \ libnbd_200_connect_command_test.go \ libnbd_210_opt_abort_test.go \ libnbd_220_opt_list_test.go \ libnbd_230_opt_info_test.go \ libnbd_240_opt_list_meta_test.go \ libnbd_300_get_size_test.go \ libnbd_400_pread_test.go \ diff --git a/golang/libnbd_020_aio_buffer_test.go b/golang/libnbd_020_aio_buffer_test.go new file mode 100644 index 00000000..5898746b --- /dev/null +++ b/golang/libnbd_020_aio_buffer_test.go @@ -0,0 +1,104 @@ +/* libnbd golang tests + * Copyright (C) 2013-2022 Red Hat Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2 of the License, or (at your option) any later version. + * + * This library 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 + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +package libnbd + +import ( + "bytes" + "testing" +) + +func TestAioBuffer(t *testing.T) { + /* Create a buffer with uninitialized backing array. */ + buf := MakeAioBuffer(uint(32)) + defer buf.Free() + + /* Initialize backing array contents. */ + for i := uint(0); i < buf.Size; i++ { + *buf.Get(i) = 0 + } + + /* Create a slice by copying the backing array contents into Go memory. */ + b := buf.Bytes() + + zeroes := make([]byte, 32) + if !bytes.Equal(b, zeroes) { + t.Fatalf("Expected %v, got %v", zeroes, buf.Bytes()) + } + + /* Modifying returned slice does not modify the buffer. */ + for i := 0; i < len(b); i++ { + b[i] = 42 + } + + /* Bytes() still returns zeroes. */ + if !bytes.Equal(buf.Bytes(), zeroes) { + t.Fatalf("Expected %v, got %v", zeroes, buf.Bytes()) + } + + /* Create another buffer from Go slice. */ + buf2 := FromBytes(zeroes) + defer buf2.Free() + + if !bytes.Equal(buf2.Bytes(), zeroes) { + t.Fatalf("Expected %v, got %v", zeroes, buf2.Bytes()) + } +} + +// Typical buffer size. +const bufferSize uint = 256 * 1024 + +// Benchmark creating an uninitialized buffer. +func BenchmarkMakeAioBuffer(b *testing.B) { + for i := 0; i < b.N; i++ { + buf := MakeAioBuffer(bufferSize) + buf.Free() + } +} + +// Benchmark zeroing a buffer. +func BenchmarkAioBufferZero(b *testing.B) { + for i := 0; i < b.N; i++ { + buf := MakeAioBuffer(bufferSize) + for i := uint(0); i < bufferSize; i++ { + *buf.Get(i) = 0 + } + buf.Free() + } +} + +// Benchmark creating a buffer by copying a Go slice. +func BenchmarkFromBytes(b *testing.B) { + for i := 0; i < b.N; i++ { + zeroes := make([]byte, bufferSize) + buf := FromBytes(zeroes) + buf.Free() + } +} + +// Benchmark creating a slice by copying the contents. +func BenchmarkAioBufferBytes(b *testing.B) { + buf := MakeAioBuffer(bufferSize) + defer buf.Free() + var r int + + b.ResetTimer() + for i := 0; i < b.N; i++ { + r += len(buf.Bytes()) + } +} diff --git a/golang/run-tests.sh b/golang/run-tests.sh index f83fa183..3a07b23a 100755 --- a/golang/run-tests.sh +++ b/golang/run-tests.sh @@ -20,10 +20,15 @@ set -e set -x requires nbdkit --version # The -count=1 parameter is the "idiomatic way to bypass test caching". # https://golang.org/doc/go1.10#test # The -v option enables verbose output. $GOLANG test -count=1 -v + +# Run the benchmarks with 10 milliseconds timeout to make sure they do +# not break by mistake, without overloading the CI. For performance +# testing run "go test" directly. +$GOLANG test -run=XXX -bench=. -benchtime=10ms -- 2.34.1
On Fri, Feb 11, 2022 at 03:21:21AM +0200, Nir Soffer wrote:
Add unit tests and benchmarks for AioBuffer. The tests are trivial but they server as running documentation, and they point out important details about the type.
The benchmarks show the efficiency of allocating a new buffer, zeroing it, and interfacing with Go code.
These tests will also ensure that we don't break anything by the next changes.
To run the benchmarks use:
$ go test -run=xxx -bench=. [...] +# Run the benchmarks with 10 milliseconds timeout to make sure they do +# not break by mistake, without overloading the CI. For performance +# testing run "go test" directly. +$GOLANG test -run=XXX -bench=. -benchtime=10ms
-run param is a regexp matching the names of the tests to run. It might be best to use something like this instead: go test -run=<regexp> -bench=. because elsewhere we use "XXX" to mark code that needs to be fixed. Apart from this the whole series seems fine to me, ACK. 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
On Fri, Feb 11, 2022 at 1:22 PM Richard W.M. Jones <rjones@redhat.com> wrote:
On Fri, Feb 11, 2022 at 03:21:21AM +0200, Nir Soffer wrote:
Add unit tests and benchmarks for AioBuffer. The tests are trivial but they server as running documentation, and they point out important details about the type.
The benchmarks show the efficiency of allocating a new buffer, zeroing it, and interfacing with Go code.
These tests will also ensure that we don't break anything by the next changes.
To run the benchmarks use:
$ go test -run=xxx -bench=. [...] +# Run the benchmarks with 10 milliseconds timeout to make sure they do +# not break by mistake, without overloading the CI. For performance +# testing run "go test" directly. +$GOLANG test -run=XXX -bench=. -benchtime=10ms
-run param is a regexp matching the names of the tests to run. It might be best to use something like this instead:
go test -run=<regexp> -bench=.
because elsewhere we use "XXX" to mark code that needs to be fixed.
The intent of this command is to run only the benchmark, using -run=XXX to match no test. I agree this is a poor choice for this project since we use XXX for other purposes.
Apart from this the whole series seems fine to me, ACK.
Thanks, I'll push this with a better regex. Nir
On Fri, Feb 11, 2022 at 9:08 PM Nir Soffer <nsoffer@redhat.com> wrote:
On Fri, Feb 11, 2022 at 1:22 PM Richard W.M. Jones <rjones@redhat.com> wrote:
On Fri, Feb 11, 2022 at 03:21:21AM +0200, Nir Soffer wrote:
Add unit tests and benchmarks for AioBuffer. The tests are trivial but they server as running documentation, and they point out important details about the type.
The benchmarks show the efficiency of allocating a new buffer, zeroing it, and interfacing with Go code.
These tests will also ensure that we don't break anything by the next changes.
To run the benchmarks use:
$ go test -run=xxx -bench=. [...] +# Run the benchmarks with 10 milliseconds timeout to make sure they do +# not break by mistake, without overloading the CI. For performance +# testing run "go test" directly. +$GOLANG test -run=XXX -bench=. -benchtime=10ms
-run param is a regexp matching the names of the tests to run. It might be best to use something like this instead:
go test -run=<regexp> -bench=.
because elsewhere we use "XXX" to mark code that needs to be fixed.
The intent of this command is to run only the benchmark, using -run=XXX to match no test. I agree this is a poor choice for this project since we use XXX for other purposes.
Apart from this the whole series seems fine to me, ACK.
Thanks, I'll push this with a better regex.
Updated using this change: $ git diff -U3 aio-buffer-v2.. diff --git a/golang/run-tests.sh b/golang/run-tests.sh index 3a07b23a..82dcca56 100755 --- a/golang/run-tests.sh +++ b/golang/run-tests.sh @@ -28,7 +28,9 @@ requires nbdkit --version # The -v option enables verbose output. $GOLANG test -count=1 -v -# Run the benchmarks with 10 milliseconds timeout to make sure they do -# not break by mistake, without overloading the CI. For performance +# Run the only benchmarks with 10 milliseconds timeout to make sure they +# do not break by mistake, without overloading the CI. For performance # testing run "go test" directly. -$GOLANG test -run=XXX -bench=. -benchtime=10ms +# The -run=- parameter is the way to match no test, assuming that no test or +# sub test contains "-" in the name. +$GOLANG test -run=- -bench=. -benchtime=10ms I'm waiting a few days in case Eric wants to comment.
On Sat, Feb 12, 2022 at 01:05:29AM +0200, Nir Soffer wrote:
On Fri, Feb 11, 2022 at 9:08 PM Nir Soffer <nsoffer@redhat.com> wrote:
On Fri, Feb 11, 2022 at 1:22 PM Richard W.M. Jones <rjones@redhat.com> wrote:
On Fri, Feb 11, 2022 at 03:21:21AM +0200, Nir Soffer wrote:
Add unit tests and benchmarks for AioBuffer. The tests are trivial but they server as running documentation, and they point out important details about the type.
The benchmarks show the efficiency of allocating a new buffer, zeroing it, and interfacing with Go code.
These tests will also ensure that we don't break anything by the next changes.
To run the benchmarks use:
$ go test -run=xxx -bench=. [...] +# Run the benchmarks with 10 milliseconds timeout to make sure they do +# not break by mistake, without overloading the CI. For performance +# testing run "go test" directly. +$GOLANG test -run=XXX -bench=. -benchtime=10ms
-run param is a regexp matching the names of the tests to run. It might be best to use something like this instead:
go test -run=<regexp> -bench=.
because elsewhere we use "XXX" to mark code that needs to be fixed.
The intent of this command is to run only the benchmark, using -run=XXX to match no test. I agree this is a poor choice for this project since we use XXX for other purposes.
Apart from this the whole series seems fine to me, ACK.
Thanks, I'll push this with a better regex.
Updated using this change:
$ git diff -U3 aio-buffer-v2.. diff --git a/golang/run-tests.sh b/golang/run-tests.sh index 3a07b23a..82dcca56 100755 --- a/golang/run-tests.sh +++ b/golang/run-tests.sh @@ -28,7 +28,9 @@ requires nbdkit --version # The -v option enables verbose output. $GOLANG test -count=1 -v
-# Run the benchmarks with 10 milliseconds timeout to make sure they do -# not break by mistake, without overloading the CI. For performance +# Run the only benchmarks with 10 milliseconds timeout to make sure they +# do not break by mistake, without overloading the CI. For performance # testing run "go test" directly. -$GOLANG test -run=XXX -bench=. -benchtime=10ms +# The -run=- parameter is the way to match no test, assuming that no test or +# sub test contains "-" in the name. +$GOLANG test -run=- -bench=. -benchtime=10ms
I'm waiting a few days in case Eric wants to comment.
I admit that I misread the original patch and thought this was a comment (with "XXX" as a placeholder), rather than a command that runs. I think your updated version is better. Thanks, 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
On Fri, Feb 11, 2022 at 03:21:21AM +0200, Nir Soffer wrote:
Add unit tests and benchmarks for AioBuffer. The tests are trivial but they server as running documentation, and they point out important
serve
details about the type.
The benchmarks show the efficiency of allocating a new buffer, zeroing it, and interfacing with Go code.
These tests will also ensure that we don't break anything by the next changes.
To run the benchmarks use:
$ go test -run=xxx -bench=. goos: linux goarch: amd64 pkg: libguestfs.org/libnbd cpu: Intel(R) Core(TM) i7-10850H CPU @ 2.70GHz BenchmarkMakeAioBuffer-12 6871759 157.2 ns/op BenchmarkAioBufferZero-12 17551 69552 ns/op BenchmarkFromBytes-12 9632 139112 ns/op BenchmarkAioBufferBytes-12 69375 16410 ns/op PASS ok libguestfs.org/libnbd 5.843s
To make sure the benchmarks will not break, we run them in "make check" with a very short timeout. For actual performance testing run "go test" directly.
Sounds good to me. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Mon, Feb 14, 2022 at 3:22 PM Eric Blake <eblake@redhat.com> wrote:
On Fri, Feb 11, 2022 at 03:21:21AM +0200, Nir Soffer wrote:
Add unit tests and benchmarks for AioBuffer. The tests are trivial but they server as running documentation, and they point out important
serve
Fixed
details about the type.
The benchmarks show the efficiency of allocating a new buffer, zeroing it, and interfacing with Go code.
These tests will also ensure that we don't break anything by the next changes.
To run the benchmarks use:
$ go test -run=xxx -bench=. goos: linux goarch: amd64 pkg: libguestfs.org/libnbd cpu: Intel(R) Core(TM) i7-10850H CPU @ 2.70GHz BenchmarkMakeAioBuffer-12 6871759 157.2 ns/op BenchmarkAioBufferZero-12 17551 69552 ns/op BenchmarkFromBytes-12 9632 139112 ns/op BenchmarkAioBufferBytes-12 69375 16410 ns/op PASS ok libguestfs.org/libnbd 5.843s
To make sure the benchmarks will not break, we run them in "make check" with a very short timeout. For actual performance testing run "go test" directly.
Sounds good to me.
Thanks, I'll push tomorrow if you don't have more comments. Nir
On Tue, Feb 15, 2022 at 12:18 AM Nir Soffer <nsoffer@redhat.com> wrote:
On Mon, Feb 14, 2022 at 3:22 PM Eric Blake <eblake@redhat.com> wrote:
On Fri, Feb 11, 2022 at 03:21:21AM +0200, Nir Soffer wrote:
Add unit tests and benchmarks for AioBuffer. The tests are trivial but they server as running documentation, and they point out important
serve
Fixed
details about the type.
The benchmarks show the efficiency of allocating a new buffer, zeroing it, and interfacing with Go code.
These tests will also ensure that we don't break anything by the next changes.
To run the benchmarks use:
$ go test -run=xxx -bench=. goos: linux goarch: amd64 pkg: libguestfs.org/libnbd cpu: Intel(R) Core(TM) i7-10850H CPU @ 2.70GHz BenchmarkMakeAioBuffer-12 6871759 157.2 ns/op BenchmarkAioBufferZero-12 17551 69552 ns/op BenchmarkFromBytes-12 9632 139112 ns/op BenchmarkAioBufferBytes-12 69375 16410 ns/op PASS ok libguestfs.org/libnbd 5.843s
To make sure the benchmarks will not break, we run them in "make check" with a very short timeout. For actual performance testing run "go test" directly.
Sounds good to me.
Thanks, I'll push tomorrow if you don't have more comments.
Pushed as 28137eea9b78ca32fce97f8c68483f447b861bc9..9099657ef541a635eae0d6d79080ad5bb0bc3281
If a Go program tries to use AioBuffer after calling AioBuffer.Free(), the program may silently corrupt data, accessing memory that does not belong to the buffer any more, or segfault if the address is not mapped. In the worst case, it can corrupt memory silently. Calling Free() twice may silently free unrelated memory. Make the buffer safer to use by Freeing only on the first call and setting the pointer to nil. This makes multiple calls to Free() harmless, just like the underlying C.free(). Trying to access Bytes() and Get() after calling Free() will always panic now, revealing the bug in the program. Trying to use AioBuffer with libnbd API will likely segfault and panic. I did not try to test this. Signed-off-by: Nir Soffer <nsoffer@redhat.com> --- golang/aio_buffer.go | 5 +++- golang/libnbd_020_aio_buffer_test.go | 41 ++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/golang/aio_buffer.go b/golang/aio_buffer.go index 2bc69a01..2b77d6ee 100644 --- a/golang/aio_buffer.go +++ b/golang/aio_buffer.go @@ -46,20 +46,23 @@ func MakeAioBuffer(size uint) AioBuffer { func FromBytes(buf []byte) AioBuffer { size := len(buf) ret := MakeAioBuffer(uint(size)) for i := 0; i < len(buf); i++ { *ret.Get(uint(i)) = buf[i] } return ret } func (b *AioBuffer) Free() { - C.free(b.P) + if b.P != nil { + C.free(b.P) + b.P = nil + } } func (b *AioBuffer) Bytes() []byte { return C.GoBytes(b.P, C.int(b.Size)) } func (b *AioBuffer) Get(i uint) *byte { return (*byte)(unsafe.Pointer(uintptr(b.P) + uintptr(i))) } diff --git a/golang/libnbd_020_aio_buffer_test.go b/golang/libnbd_020_aio_buffer_test.go index 5898746b..cec74ddc 100644 --- a/golang/libnbd_020_aio_buffer_test.go +++ b/golang/libnbd_020_aio_buffer_test.go @@ -53,20 +53,61 @@ func TestAioBuffer(t *testing.T) { /* Create another buffer from Go slice. */ buf2 := FromBytes(zeroes) defer buf2.Free() if !bytes.Equal(buf2.Bytes(), zeroes) { t.Fatalf("Expected %v, got %v", zeroes, buf2.Bytes()) } } +func TestAioBufferFree(t *testing.T) { + buf := MakeAioBuffer(uint(32)) + + /* Free the underlying C array. */ + buf.Free() + + /* And clear the pointer. */ + if buf.P != nil { + t.Fatal("Dangling pointer after Free()") + } + + /* Additional Free does nothing. */ + buf.Free() +} + +func TestAioBufferBytesAfterFree(t *testing.T) { + buf := MakeAioBuffer(uint(32)) + buf.Free() + + defer func() { + if r := recover(); r == nil { + t.Fatal("Did not recover from panic calling Bytes() after Free()") + } + }() + + buf.Bytes() +} + +func TestAioBufferGetAfterFree(t *testing.T) { + buf := MakeAioBuffer(uint(32)) + buf.Free() + + defer func() { + if r := recover(); r == nil { + t.Fatal("Did not recover from panic calling Get() after Free()") + } + }() + + *buf.Get(0) = 42 +} + // Typical buffer size. const bufferSize uint = 256 * 1024 // Benchmark creating an uninitialized buffer. func BenchmarkMakeAioBuffer(b *testing.B) { for i := 0; i < b.N; i++ { buf := MakeAioBuffer(bufferSize) buf.Free() } } -- 2.34.1
Add standard function documentation comments. The documentation should be available here: https://pkg.go.dev/libguestfs.org/libnbd#AioBuffer Signed-off-by: Nir Soffer <nsoffer@redhat.com> --- golang/aio_buffer.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/golang/aio_buffer.go b/golang/aio_buffer.go index 2b77d6ee..2cd8ceb2 100644 --- a/golang/aio_buffer.go +++ b/golang/aio_buffer.go @@ -32,37 +32,49 @@ package libnbd import "C" import "unsafe" /* Asynchronous I/O buffer. */ type AioBuffer struct { P unsafe.Pointer Size uint } +// MakeAioBuffer makes a new buffer backed by an uninitialized C allocated +// array. func MakeAioBuffer(size uint) AioBuffer { return AioBuffer{C.malloc(C.ulong(size)), size} } +// FromBytes makes a new buffer backed by a C allocated array, initialized by +// copying the given Go slice. func FromBytes(buf []byte) AioBuffer { size := len(buf) ret := MakeAioBuffer(uint(size)) for i := 0; i < len(buf); i++ { *ret.Get(uint(i)) = buf[i] } return ret } +// Free deallocates the underlying C allocated array. Using the buffer after +// Free() will panic. func (b *AioBuffer) Free() { if b.P != nil { C.free(b.P) b.P = nil } } +// Bytes copies the underlying C array to Go allocated memory and return a +// slice. Modifying the returned slice does not modify the underlying buffer +// backing array. func (b *AioBuffer) Bytes() []byte { return C.GoBytes(b.P, C.int(b.Size)) } +// Get returns a pointer to a byte in the underlying C array. The pointer can +// be used to modify the underlying array. The pointer must not be used after +// calling Free(). func (b *AioBuffer) Get(i uint) *byte { return (*byte)(unsafe.Pointer(uintptr(b.P) + uintptr(i))) } -- 2.34.1
Make it easy to create a zeroed buffer via calloc(), preventing leaking sensitive info from the heap. Benchmarking shows that creating a zeroed buffer is much slower compared with uninitialized buffer, but much faster compared with manually initializing the buffer with a loop. BenchmarkMakeAioBuffer-12 7252674 148.1 ns/op BenchmarkMakeAioBufferZero-12 262107 4181 ns/op BenchmarkAioBufferZero-12 17581 68759 ns/op It is interesting that creating a zeroed buffer is 3 times faster compared with making a new []byte slice: BenchmarkMakeAioBufferZero-12 247710 4440 ns/op BenchmarkMakeByteSlice-12 84117 13733 ns/op Signed-off-by: Nir Soffer <nsoffer@redhat.com> --- golang/aio_buffer.go | 6 ++++++ golang/libnbd_020_aio_buffer_test.go | 16 ++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/golang/aio_buffer.go b/golang/aio_buffer.go index 2cd8ceb2..d2e6e350 100644 --- a/golang/aio_buffer.go +++ b/golang/aio_buffer.go @@ -38,20 +38,26 @@ type AioBuffer struct { P unsafe.Pointer Size uint } // MakeAioBuffer makes a new buffer backed by an uninitialized C allocated // array. func MakeAioBuffer(size uint) AioBuffer { return AioBuffer{C.malloc(C.ulong(size)), size} } +// MakeAioBuffer makes a new buffer backed by a C allocated array. The +// underlying buffer is set to zero. +func MakeAioBufferZero(size uint) AioBuffer { + return AioBuffer{C.calloc(C.ulong(1), C.ulong(size)), size} +} + // FromBytes makes a new buffer backed by a C allocated array, initialized by // copying the given Go slice. func FromBytes(buf []byte) AioBuffer { size := len(buf) ret := MakeAioBuffer(uint(size)) for i := 0; i < len(buf); i++ { *ret.Get(uint(i)) = buf[i] } return ret } diff --git a/golang/libnbd_020_aio_buffer_test.go b/golang/libnbd_020_aio_buffer_test.go index cec74ddc..b3a2a8d9 100644 --- a/golang/libnbd_020_aio_buffer_test.go +++ b/golang/libnbd_020_aio_buffer_test.go @@ -51,20 +51,28 @@ func TestAioBuffer(t *testing.T) { t.Fatalf("Expected %v, got %v", zeroes, buf.Bytes()) } /* Create another buffer from Go slice. */ buf2 := FromBytes(zeroes) defer buf2.Free() if !bytes.Equal(buf2.Bytes(), zeroes) { t.Fatalf("Expected %v, got %v", zeroes, buf2.Bytes()) } + + /* Crated a zeroed buffer. */ + buf3 := MakeAioBufferZero(uint(32)) + defer buf.Free() + + if !bytes.Equal(buf3.Bytes(), zeroes) { + t.Fatalf("Expected %v, got %v", zeroes, buf2.Bytes()) + } } func TestAioBufferFree(t *testing.T) { buf := MakeAioBuffer(uint(32)) /* Free the underlying C array. */ buf.Free() /* And clear the pointer. */ if buf.P != nil { @@ -105,20 +113,28 @@ func TestAioBufferGetAfterFree(t *testing.T) { const bufferSize uint = 256 * 1024 // Benchmark creating an uninitialized buffer. func BenchmarkMakeAioBuffer(b *testing.B) { for i := 0; i < b.N; i++ { buf := MakeAioBuffer(bufferSize) buf.Free() } } +// Benchmark creating zeroed buffer. +func BenchmarkMakeAioBufferZero(b *testing.B) { + for i := 0; i < b.N; i++ { + buf := MakeAioBufferZero(bufferSize) + buf.Free() + } +} + // Benchmark zeroing a buffer. func BenchmarkAioBufferZero(b *testing.B) { for i := 0; i < b.N; i++ { buf := MakeAioBuffer(bufferSize) for i := uint(0); i < bufferSize; i++ { *buf.Get(i) = 0 } buf.Free() } } -- 2.34.1
AioBuffer.Bytes() cannot be used for copying images from NBD to other APis because it copies the entire image. Add a new Slice() function, creating a slice backed by the underlying buffer. Using Slice() is efficient, but less safe, like Get(). The returned slice must be used only before calling Free(). This should not be an issue with typical code. Testing shows that Slice() is much faster than Bytes() for typical 256k buffer: BenchmarkAioBufferBytes-12 86616 16529 ns/op BenchmarkAioBufferSlice-12 1000000000 0.4630 ns/op I modified the aio_copy example to use AioBuffer and compiled 2 versions, one using Bytes() and one using Slice(). When copying 6g fully allocated image, the version using Slice() is 1.6 times faster. $ hyperfine -r3 "./aio_copy-bytes $SRC >/dev/null" "./aio_copy-slice $SRC >/dev/null" Benchmark 1: ./aio_copy-bytes nbd+unix:///?socket=/tmp/src.sock >/dev/null Time (mean ± σ): 3.357 s ± 0.039 s [User: 2.656 s, System: 1.162 s] Range (min … max): 3.313 s … 3.387 s 3 runs Benchmark 2: ./aio_copy-slice nbd+unix:///?socket=/tmp/src.sock >/dev/null Time (mean ± σ): 2.046 s ± 0.009 s [User: 0.423 s, System: 0.892 s] Range (min … max): 2.037 s … 2.055 s 3 runs Summary './aio_copy-slice nbd+unix:///?socket=/tmp/src.sock >/dev/null' ran 1.64 ± 0.02 times faster than './aio_copy-bytes nbd+unix:///?socket=/tmp/src.sock >/dev/null' When copying a 6g empty image (qemu-nbd sends one hole chunk for every read), the version using Slice() is 2.6 times faster. $ hyperfine -r3 "./aio_copy-bytes $SRC >/dev/null" "./aio_copy-slice $SRC >/dev/null" Benchmark 1: ./aio_copy-bytes nbd+unix:///?socket=/tmp/src.sock >/dev/null Time (mean ± σ): 1.210 s ± 0.023 s [User: 1.428 s, System: 0.345 s] Range (min … max): 1.191 s … 1.235 s 3 runs Benchmark 2: ./aio_copy-slice nbd+unix:///?socket=/tmp/src.sock >/dev/null Time (mean ± σ): 461.4 ms ± 13.1 ms [User: 394.2 ms, System: 76.6 ms] Range (min … max): 450.6 ms … 476.0 ms 3 runs Summary './aio_copy-slice nbd+unix:///?socket=/tmp/src.sock >/dev/null' ran 2.62 ± 0.09 times faster than './aio_copy-bytes nbd+unix:///?socket=/tmp/src.sock >/dev/null' Signed-off-by: Nir Soffer <nsoffer@redhat.com> --- golang/aio_buffer.go | 9 ++++++ golang/libnbd_020_aio_buffer_test.go | 41 ++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/golang/aio_buffer.go b/golang/aio_buffer.go index d2e6e350..008d9ae0 100644 --- a/golang/aio_buffer.go +++ b/golang/aio_buffer.go @@ -71,16 +71,25 @@ func (b *AioBuffer) Free() { } } // Bytes copies the underlying C array to Go allocated memory and return a // slice. Modifying the returned slice does not modify the underlying buffer // backing array. func (b *AioBuffer) Bytes() []byte { return C.GoBytes(b.P, C.int(b.Size)) } +// Slice creates a slice backed by the underlying C array. The slice can be +// used to access or modify the contents of the underlying array. The slice +// must not be used after caling Free(). +func (b *AioBuffer) Slice() []byte { + // See https://github.com/golang/go/wiki/cgo#turning-c-arrays-into-go-slices + // TODO: Use unsafe.Slice() when we require Go 1.17. + return (*[1<<30]byte)(b.P)[:b.Size:b.Size] +} + // Get returns a pointer to a byte in the underlying C array. The pointer can // be used to modify the underlying array. The pointer must not be used after // calling Free(). func (b *AioBuffer) Get(i uint) *byte { return (*byte)(unsafe.Pointer(uintptr(b.P) + uintptr(i))) } diff --git a/golang/libnbd_020_aio_buffer_test.go b/golang/libnbd_020_aio_buffer_test.go index b3a2a8d9..4b1c5f93 100644 --- a/golang/libnbd_020_aio_buffer_test.go +++ b/golang/libnbd_020_aio_buffer_test.go @@ -44,20 +44,35 @@ func TestAioBuffer(t *testing.T) { /* Modifying returned slice does not modify the buffer. */ for i := 0; i < len(b); i++ { b[i] = 42 } /* Bytes() still returns zeroes. */ if !bytes.Equal(buf.Bytes(), zeroes) { t.Fatalf("Expected %v, got %v", zeroes, buf.Bytes()) } + /* Creating a slice without copying the underlying buffer. */ + s := buf.Slice() + if !bytes.Equal(s, zeroes) { + t.Fatalf("Expected %v, got %v", zeroes, s) + } + + /* Modifying the slice modifies the underlying buffer. */ + for i := 0; i < len(s); i++ { + s[i] = 42 + } + + if !bytes.Equal(buf.Slice(), s) { + t.Fatalf("Expected %v, got %v", s, buf.Slice()) + } + /* Create another buffer from Go slice. */ buf2 := FromBytes(zeroes) defer buf2.Free() if !bytes.Equal(buf2.Bytes(), zeroes) { t.Fatalf("Expected %v, got %v", zeroes, buf2.Bytes()) } /* Crated a zeroed buffer. */ buf3 := MakeAioBufferZero(uint(32)) @@ -89,20 +104,34 @@ func TestAioBufferBytesAfterFree(t *testing.T) { defer func() { if r := recover(); r == nil { t.Fatal("Did not recover from panic calling Bytes() after Free()") } }() buf.Bytes() } +func TestAioBufferSliceAfterFree(t *testing.T) { + buf := MakeAioBuffer(uint(32)) + buf.Free() + + defer func() { + if r := recover(); r == nil { + t.Fatal("Did not recover from panic calling Bytes() after Free()") + } + }() + + s := buf.Slice() + s[0] = 42 +} + func TestAioBufferGetAfterFree(t *testing.T) { buf := MakeAioBuffer(uint(32)) buf.Free() defer func() { if r := recover(); r == nil { t.Fatal("Did not recover from panic calling Get() after Free()") } }() @@ -152,10 +181,22 @@ func BenchmarkFromBytes(b *testing.B) { func BenchmarkAioBufferBytes(b *testing.B) { buf := MakeAioBuffer(bufferSize) defer buf.Free() var r int b.ResetTimer() for i := 0; i < b.N; i++ { r += len(buf.Bytes()) } } + +// Benchmark creating a slice without copying the underling buffer. +func BenchmarkAioBufferSlice(b *testing.B) { + buf := MakeAioBuffer(bufferSize) + defer buf.Free() + var r int + + b.ResetTimer() + for i := 0; i < b.N; i++ { + r += len(buf.Slice()) + } +} -- 2.34.1
Slice() is easier to use and faster than Get() or Bytes(). Let's use the new way. Signed-off-by: Nir Soffer <nsoffer@redhat.com> --- golang/libnbd_020_aio_buffer_test.go | 8 +++++--- golang/libnbd_500_aio_pread_test.go | 2 +- golang/libnbd_510_aio_pwrite_test.go | 8 +++++--- 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/golang/libnbd_020_aio_buffer_test.go b/golang/libnbd_020_aio_buffer_test.go index 4b1c5f93..e07f8973 100644 --- a/golang/libnbd_020_aio_buffer_test.go +++ b/golang/libnbd_020_aio_buffer_test.go @@ -22,22 +22,23 @@ import ( "bytes" "testing" ) func TestAioBuffer(t *testing.T) { /* Create a buffer with uninitialized backing array. */ buf := MakeAioBuffer(uint(32)) defer buf.Free() /* Initialize backing array contents. */ + s := buf.Slice() for i := uint(0); i < buf.Size; i++ { - *buf.Get(i) = 0 + s[i] = 0 } /* Create a slice by copying the backing array contents into Go memory. */ b := buf.Bytes() zeroes := make([]byte, 32) if !bytes.Equal(b, zeroes) { t.Fatalf("Expected %v, got %v", zeroes, buf.Bytes()) } @@ -45,21 +46,21 @@ func TestAioBuffer(t *testing.T) { for i := 0; i < len(b); i++ { b[i] = 42 } /* Bytes() still returns zeroes. */ if !bytes.Equal(buf.Bytes(), zeroes) { t.Fatalf("Expected %v, got %v", zeroes, buf.Bytes()) } /* Creating a slice without copying the underlying buffer. */ - s := buf.Slice() + s = buf.Slice() if !bytes.Equal(s, zeroes) { t.Fatalf("Expected %v, got %v", zeroes, s) } /* Modifying the slice modifies the underlying buffer. */ for i := 0; i < len(s); i++ { s[i] = 42 } if !bytes.Equal(buf.Slice(), s) { @@ -154,22 +155,23 @@ func BenchmarkMakeAioBufferZero(b *testing.B) { for i := 0; i < b.N; i++ { buf := MakeAioBufferZero(bufferSize) buf.Free() } } // Benchmark zeroing a buffer. func BenchmarkAioBufferZero(b *testing.B) { for i := 0; i < b.N; i++ { buf := MakeAioBuffer(bufferSize) + s := buf.Slice() for i := uint(0); i < bufferSize; i++ { - *buf.Get(i) = 0 + s[i] = 0 } buf.Free() } } // Benchmark creating a buffer by copying a Go slice. func BenchmarkFromBytes(b *testing.B) { for i := 0; i < b.N; i++ { zeroes := make([]byte, bufferSize) buf := FromBytes(zeroes) diff --git a/golang/libnbd_500_aio_pread_test.go b/golang/libnbd_500_aio_pread_test.go index 0811378c..bd0208ef 100644 --- a/golang/libnbd_500_aio_pread_test.go +++ b/golang/libnbd_500_aio_pread_test.go @@ -55,14 +55,14 @@ func Test500AioPRead(t *testing.T) { } h.Poll(-1) } // Expected data. expected := make([]byte, 512) for i := 0; i < 512; i += 8 { binary.BigEndian.PutUint64(expected[i:i+8], uint64(i)) } - if !bytes.Equal(buf.Bytes(), expected) { + if !bytes.Equal(buf.Slice(), expected) { t.Fatalf("did not read expected data") } } diff --git a/golang/libnbd_510_aio_pwrite_test.go b/golang/libnbd_510_aio_pwrite_test.go index 56cdcb05..493159f2 100644 --- a/golang/libnbd_510_aio_pwrite_test.go +++ b/golang/libnbd_510_aio_pwrite_test.go @@ -32,23 +32,25 @@ func Test510AioPWrite(t *testing.T) { "nbdkit", "-s", "--exit-with-parent", "-v", "memory", "size=512", }) if err != nil { t.Fatalf("could not connect: %s", err) } /* Write a pattern and read it back. */ buf := MakeAioBuffer(512) defer buf.Free() + + s := buf.Slice() for i := 0; i < 512; i += 2 { - *buf.Get(uint(i)) = 0x55 - *buf.Get(uint(i + 1)) = 0xAA + s[i] = 0x55 + s[i+1] = 0xAA } var cookie uint64 cookie, err = h.AioPwrite(buf, 0, nil) if err != nil { t.Fatalf("%s", err) } for { var b bool b, err = h.AioCommandCompleted(cookie) @@ -62,14 +64,14 @@ func Test510AioPWrite(t *testing.T) { } /* We already tested aio_pread, let's just read the data back in the regular synchronous way. */ buf2 := make([]byte, 512) err = h.Pread(buf2, 0, nil) if err != nil { t.Fatalf("%s", err) } - if !bytes.Equal(buf.Bytes(), buf2) { + if !bytes.Equal(buf.Slice(), buf2) { t.Fatalf("did not read back same data as written") } } -- 2.34.1
Using Slice() we can use builtin copy() instead of a manual loop, which is 4.6 times faster with a typical 256k buffer: Before: BenchmarkFromBytes-12 9806 111474 ns/op After: BenchmarkFromBytes-12 48193 24106 ns/op Signed-off-by: Nir Soffer <nsoffer@redhat.com> --- golang/aio_buffer.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/golang/aio_buffer.go b/golang/aio_buffer.go index 008d9ae0..52ea54de 100644 --- a/golang/aio_buffer.go +++ b/golang/aio_buffer.go @@ -47,25 +47,22 @@ func MakeAioBuffer(size uint) AioBuffer { // MakeAioBuffer makes a new buffer backed by a C allocated array. The // underlying buffer is set to zero. func MakeAioBufferZero(size uint) AioBuffer { return AioBuffer{C.calloc(C.ulong(1), C.ulong(size)), size} } // FromBytes makes a new buffer backed by a C allocated array, initialized by // copying the given Go slice. func FromBytes(buf []byte) AioBuffer { - size := len(buf) - ret := MakeAioBuffer(uint(size)) - for i := 0; i < len(buf); i++ { - *ret.Get(uint(i)) = buf[i] - } + ret := MakeAioBuffer(uint(len(buf))) + copy(ret.Slice(), buf) return ret } // Free deallocates the underlying C allocated array. Using the buffer after // Free() will panic. func (b *AioBuffer) Free() { if b.P != nil { C.free(b.P) b.P = nil } -- 2.34.1
Add benchmark for coping a buffer using 3 strategies - reusing same buffer, making a new uninitialized buffer per copy, and using a zeroed buffer per copy. This benchmark is the worst possible case, copying a buffer to memory. Any real I/O will be much slower, hiding the overhead of allocating or zeroing buffers. $ go test -run=AioBuffer -bench=Copy -benchtime=5s goos: linux goarch: amd64 pkg: libguestfs.org/libnbd cpu: Intel(R) Core(TM) i7-10850H CPU @ 2.70GHz BenchmarkAioBufferCopyBaseline-12 1142508 4523 ns/op BenchmarkAioBufferCopyMake-12 1000000 5320 ns/op BenchmarkAioBufferCopyMakeZero-12 728940 8218 ns/op Signed-off-by: Nir Soffer <nsoffer@redhat.com> --- golang/libnbd_020_aio_buffer_test.go | 32 ++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/golang/libnbd_020_aio_buffer_test.go b/golang/libnbd_020_aio_buffer_test.go index e07f8973..f38866e7 100644 --- a/golang/libnbd_020_aio_buffer_test.go +++ b/golang/libnbd_020_aio_buffer_test.go @@ -195,10 +195,42 @@ func BenchmarkAioBufferBytes(b *testing.B) { func BenchmarkAioBufferSlice(b *testing.B) { buf := MakeAioBuffer(bufferSize) defer buf.Free() var r int b.ResetTimer() for i := 0; i < b.N; i++ { r += len(buf.Slice()) } } + +var data = make([]byte, bufferSize) + +// Benchmark copying into same buffer, used as baseline for CopyMake and +// CopyMakeZero benchmarks. +func BenchmarkAioBufferCopyBaseline(b *testing.B) { + buf := MakeAioBufferZero(bufferSize) + defer buf.Free() + + b.ResetTimer() + for i := 0; i < b.N; i++ { + copy(buf.Slice(), data) + } +} + +// Benchmark overhead of making a new buffer per read. +func BenchmarkAioBufferCopyMake(b *testing.B) { + for i := 0; i < b.N; i++ { + buf := MakeAioBuffer(bufferSize) + copy(buf.Slice(), data) + buf.Free() + } +} + +// Benchmark overhead of making a new zero buffer per read. +func BenchmarkAioBufferCopyMakeZero(b *testing.B) { + for i := 0; i < b.N; i++ { + buf := MakeAioBufferZero(bufferSize) + copy(buf.Slice(), data) + buf.Free() + } +} -- 2.34.1
Now that we have an efficient way to use AioBuffer, we don't need the hacks to create AioBuffer from Go slice. Benchmarking AioBuffer shows that allocating a 256k buffer is practically free, so there is no need for the buffer pool. Now we allocate a new buffer per request, keep it in the command, and free it when the request is finished. Signed-off-by: Nir Soffer <nsoffer@redhat.com> --- golang/examples/aio_copy/aio_copy.go | 29 +++++----------------------- 1 file changed, 5 insertions(+), 24 deletions(-) diff --git a/golang/examples/aio_copy/aio_copy.go b/golang/examples/aio_copy/aio_copy.go index b6f5def1..bb20b478 100644 --- a/golang/examples/aio_copy/aio_copy.go +++ b/golang/examples/aio_copy/aio_copy.go @@ -37,53 +37,43 @@ // Example: // // ./aio_copy nbd+unix:///?socket=/tmp.nbd >/dev/null // package main import ( "container/list" "flag" "os" - "sync" "syscall" - "unsafe" "libguestfs.org/libnbd" ) var ( // These options give best performance with fast NVMe drive. requestSize = flag.Uint("request-size", 256*1024, "maximum request size in bytes") requests = flag.Uint("requests", 4, "maximum number of requests in flight") h *libnbd.Libnbd // Keeping commands in a queue ensures commands are written in the right // order, even if they complete out of order. This allows parallel reads // with non-seekable output. queue list.List - - // Buffer pool allocating buffers as needed and reusing them. - bufPool = sync.Pool{ - New: func() interface{} { - return make([]byte, *requestSize) - }, - } ) // command keeps state of single AioPread call while the read is handled by // libnbd, until the command reach the front of the queue and can be writen to // the output. type command struct { - buf []byte - length uint + buf libnbd.AioBuffer ready bool } func main() { flag.Parse() var err error h, err = libnbd.Create() if err != nil { @@ -139,60 +129,51 @@ func waitForCompletion() { panic(err) } if inflightRequests() < start { break // A read completed. } } } func startRead(offset uint64, length uint) { - buf := bufPool.Get().([]byte) - - // Keep buffer in command so we can put it back into the pool when the - // command completes. - cmd := &command{buf: buf, length: length} - - // Create aio buffer from pool buffer to avoid unneeded allocation for - // every read, and unneeded copy when completing the read. - abuf := libnbd.AioBuffer{P: unsafe.Pointer(&buf[0]), Size: length} + cmd := &command{buf: libnbd.MakeAioBuffer(length)} args := libnbd.AioPreadOptargs{ CompletionCallbackSet: true, CompletionCallback: func(error *int) int { if *error != 0 { // This is not documented, but *error is errno value translated // from the the NBD server error. err := syscall.Errno(*error).Error() panic(err) } cmd.ready = true return 1 }, } - _, err := h.AioPread(abuf, offset, &args) + _, err := h.AioPread(cmd.buf, offset, &args) if err != nil { panic(err) } queue.PushBack(cmd) } func readReady() bool { return queue.Len() > 0 && queue.Front().Value.(*command).ready } func finishRead() { e := queue.Front() queue.Remove(e) cmd := e.Value.(*command) - b := cmd.buf[:cmd.length] - _, err := os.Stdout.Write(b) + _, err := os.Stdout.Write(cmd.buf.Slice()) if err != nil { panic(err) } - bufPool.Put(cmd.buf) + cmd.buf.Free() } -- 2.34.1
participants (3)
-
Eric Blake -
Nir Soffer -
Richard W.M. Jones