On Fri, Feb 11, 2022 at 9:08 PM Nir Soffer <nsoffer(a)redhat.com&gt; wrote: 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.

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(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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