Eric Blake
2023-Aug-03 01:50 UTC
[Libguestfs] [libnbd PATCH v4 05/25] golang: Change logic of copy_uint32_array
Commit 6725fa0e12 changed copy_uint32_array() to utilize a Go hack for accessing a C array as a Go slice in order to potentially benefit from any optimizations in Go's copy() for bulk transfer of memory over naive one-at-a-time iteration. But that commit also acknowledged that no benchmark timings were performed, which would have been useful to demonstrat an actual benefit for using hack in the first place. And since we are copying data anyways (rather than using the slice to avoid a copy), and network transmission costs have a higher impact to performance than in-memory copying speed, it's hard to justify keeping the hack without hard data. What's more, while using Go's copy() on an array of C uint32_t makes sense for 32-bit extents, our corresponding 64-bit code uses a struct which does not map as nicely to Go's copy(). Using a common style between both list copying helpers is beneficial to mainenance. Additionally, at face value, converting C.size_t to int may truncate; we could avoid that risk if we were to uniformly use uint64 instead of int. But we can equally just panic if the count is oversized: our state machine guarantees that the server's response fits within 64M bytes (count will be smaller than that, since it is multiple bytes per extent entry). Suggested-by: Laszlo Ersek <lersek at redhat.com> CC: Nir Soffer <nsoffer at redhat.com> Signed-off-by: Eric Blake <eblake at redhat.com> --- v4: new patch to the series, but previously posted as part of the golang cleanups. Since then: rework the commit message as it is no longer a true revert, and add a panic() if count exceeds expected bounds. --- generator/GoLang.ml | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/generator/GoLang.ml b/generator/GoLang.ml index 73df5254..cc7d78b6 100644 --- a/generator/GoLang.ml +++ b/generator/GoLang.ml @@ -516,11 +516,16 @@ let /* Closures. */ func copy_uint32_array(entries *C.uint32_t, count C.size_t) []uint32 { + if (uint64(count) > 64*1024*1024) { + panic(\"violation of state machine guarantee\") + } ret := make([]uint32, int(count)) - // See https://github.com/golang/go/wiki/cgo#turning-c-arrays-into-go-slices - // TODO: Use unsafe.Slice() when we require Go 1.17. - s := (*[1 << 30]uint32)(unsafe.Pointer(entries))[:count:count] - copy(ret, s) + addr := uintptr(unsafe.Pointer(entries)) + for i := 0; i < int(count); i++ { + ptr := (*C.uint32_t)(unsafe.Pointer(addr)) + ret[i] = uint32(*ptr) + addr += unsafe.Sizeof(*ptr) + } return ret } "; -- 2.41.0
Richard W.M. Jones
2023-Aug-04 09:16 UTC
[Libguestfs] [libnbd PATCH v4 05/25] golang: Change logic of copy_uint32_array
On Wed, Aug 02, 2023 at 08:50:25PM -0500, Eric Blake wrote:> Commit 6725fa0e12 changed copy_uint32_array() to utilize a Go hack for > accessing a C array as a Go slice in order to potentially benefit from > any optimizations in Go's copy() for bulk transfer of memory over > naive one-at-a-time iteration. But that commit also acknowledged that > no benchmark timings were performed, which would have been useful to > demonstrat an actual benefit for using hack in the first place. And"demonstrate"> since we are copying data anyways (rather than using the slice to > avoid a copy), and network transmission costs have a higher impact to > performance than in-memory copying speed, it's hard to justify keeping > the hack without hard data. > > What's more, while using Go's copy() on an array of C uint32_t makes > sense for 32-bit extents, our corresponding 64-bit code uses a struct > which does not map as nicely to Go's copy(). Using a common style > between both list copying helpers is beneficial to mainenance. > > Additionally, at face value, converting C.size_t to int may truncate; > we could avoid that risk if we were to uniformly use uint64 instead of > int. But we can equally just panic if the count is oversized: our > state machine guarantees that the server's response fits within 64M > bytes (count will be smaller than that, since it is multiple bytes per > extent entry). > > Suggested-by: Laszlo Ersek <lersek at redhat.com> > CC: Nir Soffer <nsoffer at redhat.com> > Signed-off-by: Eric Blake <eblake at redhat.com> > --- > > v4: new patch to the series, but previously posted as part of the > golang cleanups. Since then: rework the commit message as it is no > longer a true revert, and add a panic() if count exceeds expected > bounds. > --- > generator/GoLang.ml | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/generator/GoLang.ml b/generator/GoLang.ml > index 73df5254..cc7d78b6 100644 > --- a/generator/GoLang.ml > +++ b/generator/GoLang.ml > @@ -516,11 +516,16 @@ let > /* Closures. */ > > func copy_uint32_array(entries *C.uint32_t, count C.size_t) []uint32 { > + if (uint64(count) > 64*1024*1024) { > + panic(\"violation of state machine guarantee\") > + }Not sure if golang includes the line number when it panics, but a brief bit of explanation might help here. Something like: panic(\"internal error: state machine should guarantee <= 64M of data \ in the extents reply, but this was exceeded unexpectedly\")> ret := make([]uint32, int(count)) > - // See https://github.com/golang/go/wiki/cgo#turning-c-arrays-into-go-slices > - // TODO: Use unsafe.Slice() when we require Go 1.17. > - s := (*[1 << 30]uint32)(unsafe.Pointer(entries))[:count:count]The whole magical 1 << 30 was another thing to dislike about golang, so glad to see it gone here.> - copy(ret, s) > + addr := uintptr(unsafe.Pointer(entries)) > + for i := 0; i < int(count); i++ { > + ptr := (*C.uint32_t)(unsafe.Pointer(addr)) > + ret[i] = uint32(*ptr) > + addr += unsafe.Sizeof(*ptr) > + } > return ret > }Reviewed-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 libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
Nir Soffer
2023-Aug-08 11:36 UTC
[Libguestfs] [libnbd PATCH v4 05/25] golang: Change logic of copy_uint32_array
On Thu, Aug 3, 2023 at 4:57?AM Eric Blake <eblake at redhat.com> wrote:> > Commit 6725fa0e12 changed copy_uint32_array() to utilize a Go hack for > accessing a C array as a Go slice in order to potentially benefit from > any optimizations in Go's copy() for bulk transfer of memory over > naive one-at-a-time iteration. But that commit also acknowledged that > no benchmark timings were performed, which would have been useful to > demonstrat an actual benefit for using hack in the first place. AndWhy do you call this a hack? This is the documented way to create a Go slice from memory.> since we are copying data anyways (rather than using the slice to > avoid a copy), and network transmission costs have a higher impact to > performance than in-memory copying speed, it's hard to justify keeping > the hack without hard data.Since this is not a hack we don't need to justify it :-)> What's more, while using Go's copy() on an array of C uint32_t makes > sense for 32-bit extents, our corresponding 64-bit code uses a struct > which does not map as nicely to Go's copy().If we return a slice of the C extent type, copy() can work, but it is probably not what we want to return.> Using a common style > between both list copying helpers is beneficial to mainenance. > > Additionally, at face value, converting C.size_t to int may truncate; > we could avoid that risk if we were to uniformly use uint64 instead of > int. But we can equally just panic if the count is oversized: our > state machine guarantees that the server's response fits within 64M > bytes (count will be smaller than that, since it is multiple bytes per > extent entry).Good to check this, but not related to changing the way we copy the array.> Suggested-by: Laszlo Ersek <lersek at redhat.com> > CC: Nir Soffer <nsoffer at redhat.com> > Signed-off-by: Eric Blake <eblake at redhat.com> > --- > > v4: new patch to the series, but previously posted as part of the > golang cleanups. Since then: rework the commit message as it is no > longer a true revert, and add a panic() if count exceeds expected > bounds. > --- > generator/GoLang.ml | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/generator/GoLang.ml b/generator/GoLang.ml > index 73df5254..cc7d78b6 100644 > --- a/generator/GoLang.ml > +++ b/generator/GoLang.ml > @@ -516,11 +516,16 @@ let > /* Closures. */ > > func copy_uint32_array(entries *C.uint32_t, count C.size_t) []uint32 { > + if (uint64(count) > 64*1024*1024) { > + panic(\"violation of state machine guarantee\")This is unwanted in a library, it means the entire application will crash because of a bug in the library. Can we convert this to an error in the caller?> + } > ret := make([]uint32, int(count)) > - // See https://github.com/golang/go/wiki/cgo#turning-c-arrays-into-go-slices > - // TODO: Use unsafe.Slice() when we require Go 1.17. > - s := (*[1 << 30]uint32)(unsafe.Pointer(entries))[:count:count]Can we require Go 1.17? (current version is 1.20) In Go >= 1.17, we can use something like: s := unsafe.Slice(C.uint32_t, length)> - copy(ret, s) > + addr := uintptr(unsafe.Pointer(entries)) > + for i := 0; i < int(count); i++ { > + ptr := (*C.uint32_t)(unsafe.Pointer(addr)) > + ret[i] = uint32(*ptr) > + addr += unsafe.Sizeof(*ptr) > + }This loop is worse than the ugly line creating a slice. With a slice we can do: for i, item := range s { ret[i] = uint32(item) } (I did not try to compile this)
Apparently Analagous Threads
- [libnbd PATCH v4 05/25] golang: Change logic of copy_uint32_array
- [libnbd PATCH v4 05/25] golang: Change logic of copy_uint32_array
- [libnbd PATCH] golang: Bump minimum Go version to 1.17
- [PATCH libnbd v3] Add Go language bindings (golang) (RHBZ#1814538).
- [PATCH libnbd v4] Add Go language bindings (golang) (RHBZ#1814538).