On Tue, Feb 8, 2022 at 9:33 PM Eric Blake <ebl...@redhat.com> wrote:
>
> On Sun, Jan 30, 2022 at 01:33:29AM +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 how efficient is allocation a new buffer, zeroing it, and
> > interfacing with Go code.
>
> Wording suggestion:
>
> The benchmarks show the efficiency of allocating a new buffer, zeroing
> it, and interfacing with Go code.
Thanks, I will use this.
> >
> > This tests will also ensure that we don't break anything by the next
>
> Either "These tests" or "This test"
Right
> > 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
> >
> > Signed-off-by: Nir Soffer <nsof...@redhat.com>
> > ---
> > golang/Makefile.am | 1 +
> > golang/libnbd_620_aio_buffer_test.go | 104 +++++++++++++++++++++++++++
> > 2 files changed, 105 insertions(+)
> > create mode 100644 golang/libnbd_620_aio_buffer_test.go
> >
> > diff --git a/golang/Makefile.am b/golang/Makefile.am
> > index 10fb8934..ae0486dd 100644
> > --- a/golang/Makefile.am
> > +++ b/golang/Makefile.am
> > @@ -37,20 +37,21 @@ source_files = \
> > libnbd_300_get_size_test.go \
> > libnbd_400_pread_test.go \
> > libnbd_405_pread_structured_test.go \
> > libnbd_410_pwrite_test.go \
> > libnbd_460_block_status_test.go \
> > libnbd_500_aio_pread_test.go \
> > libnbd_510_aio_pwrite_test.go \
> > libnbd_590_aio_copy_test.go \
> > libnbd_600_debug_callback_test.go \
> > libnbd_610_error_test.go \
> > + libnbd_620_aio_buffer_test.go \
>
> As discussed in a different thread, the numbering here groups
> somewhat-related functionality, and helps us keep cross-language tests
> correlated over testing the same features. Since you aren't adding
> counterpart tests to python or ocaml, I don't know what number would
> be best. But our existing numbering is more along the lines of 0xx
> for language-level loading, 1xx for NBD handle tests, 2xx for
> connection tests, 3xx for initial APIs after connecting, 4xx for
> synchronous APIs, 5xx for asynch APIs, and 6xx for high-level usage
> patterns. This feels like it might fit better in the 0xx series,
> since the benchmark does not use any NBD handle.
I agree. When I posted this I did not understand the semantics
and assumed the numbers reflect the order the tests were added.
I'll add this to the 0xx group.
>
> > $(NULL)
> >
> > generator_built = \
> > bindings.go \
> > closures.go \
> > wrappers.go \
> > wrappers.h \
> > $(NULL)
> >
> > EXTRA_DIST = \
> > diff --git a/golang/libnbd_620_aio_buffer_test.go
> > b/golang/libnbd_620_aio_buffer_test.go
> > new file mode 100644
> > index 00000000..2632f87f
> > --- /dev/null
> > +++ b/golang/libnbd_620_aio_buffer_test.go
> > @@ -0,0 +1,104 @@
> > +/* libnbd golang tests
> > + * Copyright (C) 2013-2021 Red Hat Inc.
>
> You may want to add 2022.
>
> Take the rest of my review with a grain of salt; I'm not (yet?) a Go expert.
>
> > +
> > +package libnbd
> > +
> > +import (
> > + "bytes"
> > + "testing"
> > +)
> > +
> > +func TestAioBuffer(t *testing.T) {
> > + /* Create a buffer with unitinialized backing array. */
>
> uninitialized
>
> > + 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 arrary 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 a nother buffer from Go slice. */
>
> another
>
> > + 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 uninitilized buffer.
>
> an uninitialized
>
> > +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 coping 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())
> > + }
> > +}
> > --
> > 2.34.1
> >
>
> How long do these benchmark tests take to run? The longer you run a
> benchmark, the more accurate of a number we can give for a
> per-operation average, but it also eats up a lot of CPU.
Go does magic in the tests framework, modifying b.N as the test run
so the test will alway run for 1 second.
It looks like they so preflight step since the total run time is much
larger than test count * 1s:
$ 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 7259940 143.9 ns/op
BenchmarkMakeAioBufferZero-12 237223 5004 ns/op
BenchmarkAioBufferZero-12 16834 68028 ns/op
BenchmarkFromBytes-12 35013 29428 ns/op
BenchmarkAioBufferBytes-12 51853 20343 ns/op
BenchmarkAioBufferSlice-12 1000000000 0.4807 ns/op
BenchmarkAioBufferCopyBaseline-12 252368 4965 ns/op
BenchmarkAioBufferCopyMake-12 220610 5184 ns/op
BenchmarkAioBufferCopyMakeZero-12 136143 8487 ns/op
PASS
ok libguestfs.org/libnbd 12.225s
> Is this
> something we want running on every 'make check' to ensure no
> regressions (since it doesn't even involve an NBD handle)?
Yes, running in "make check" is a good idea. Currently we run only
the tests.
> Is there a
> way to tune things so that 'make check' runs a bare-bones version to
> avoid bit rot, but another 'make benchmark' (or some such name) runs
> the full length of a benchmark?
Yes, we can use -benchtime=100ms to run quickly in make check:
$ go test -run=xxx -bench=. -benchtime=10ms
goos: linux
goarch: amd64
pkg: libguestfs.org/libnbd
cpu: Intel(R) Core(TM) i7-10850H CPU @ 2.70GHz
BenchmarkMakeAioBuffer-12 59668 178.0 ns/op
BenchmarkMakeAioBufferZero-12 1795 6266 ns/op
BenchmarkAioBufferZero-12 138 87568 ns/op
BenchmarkFromBytes-12 352 49706 ns/op
BenchmarkAioBufferBytes-12 456 29881 ns/op
BenchmarkAioBufferSlice-12 12102031 0.8438 ns/op
BenchmarkAioBufferCopyBaseline-12 1318 8421 ns/op
BenchmarkAioBufferCopyMake-12 1612 8345 ns/op
BenchmarkAioBufferCopyMakeZero-12 932 11420 ns/op
PASS
ok libguestfs.org/libnbd 0.154s
I'll post v2 with the changes suggested.
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
