7:04 p.m.
On Wed, Jun 07, 2023 at 04:23:27PM +0200, Laszlo Ersek wrote:
> +++ b/generator/GoLang.ml
> @@ -524,6 +528,18 @@ let
> copy(ret, s)
> return ret
> }
> +
> +func copy_extent_array (entries *C.nbd_extent, count C.size_t) []LibnbdExtent {
> + unsafePtr := unsafe.Pointer(entries)
> + arrayPtr := (*[1 << 20]C.nbd_extent)(unsafePtr)
> + slice := arrayPtr[:count:count]
> + ret := make([]LibnbdExtent, count)
> + for i := 0; i < int (count); i++ {
> + ret[i].Length = uint64 (slice[i].length)
> + ret[i].Flags = uint64 (slice[i].flags)
> + }
> + return ret
> +}
> ";
>
> List.iter (
The pre-existent copy_uint32_array() function uses the hideous trick at
<https://github.com/golang/go/wiki/cgo#turning-c-arrays-into-go-slices>;,
and needlessly so, IMO.
- The trick is (a) hideous because it requires us to use arbitrary sizes
such as "1<<30".
- The trick is (b) unnecessary because we don't intend to hang on to the
slice indefinitely. We only use it as a means to access the source
object. But at the noted URL, the trick is "being sold" with the pitch
"to create a Go slice backed by a C array (without copying the original
data)" -- and we copy the original data *anyway*! So it's better to use
pointer arithmetic IMO.
Regarding the new copy_extent_array(), my additional complaints are:
- whitespace usage before opening parens is inconsistent -- there is no
space after "make" and "Pointer".
- we cast "count" (a size_t in C) to a Go "int"; similarly the index
variable "i" has Go type "int".
(8) So how about this instead (should be split in two: the first part
should update copy_uint32_array() in a separate patch, and the second
part should be squashed into this patch):
> diff --git a/generator/GoLang.ml b/generator/GoLang.ml
> index 8922812b76a4..37b2240ef5bf 100644
> --- a/generator/GoLang.ml
> +++ b/generator/GoLang.ml
> @@ -521,22 +521,32 @@ let
> /* Closures. */
>
> func copy_uint32_array (entries *C.uint32_t, count C.size_t) []uint32 {
> - 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)
> + ret := make ([]uint32, count)
> + addr := uintptr (unsafe.Pointer (entries))
> + var i uint64 = 0
> + for i < uint64 (count) {
> + ptr := (*C.uint32_t)(unsafe.Pointer (addr))
> +
> + ret[i] = uint32 (*ptr)
> +
> + addr += unsafe.Sizeof (*ptr)
> + i++
> + }
Pointer arithmetic is straightforward, but not necessarily the best
use of hardware. For all I know, the copy() routine is vectorized,
and therefore can achieve better performance by copying multiple bytes
at once using better hardware primitives. So there may still be an
advantage to using the hideous hack for the sake of performance. But
as I have not bothered to benchmark that claim, I'm happy to change
back to linear copying.
> return ret
> }
>
> func copy_extent_array (entries *C.nbd_extent, count C.size_t) []LibnbdExtent {
> - unsafePtr := unsafe.Pointer(entries)
> - arrayPtr := (*[1 << 20]C.nbd_extent)(unsafePtr)
> - slice := arrayPtr[:count:count]
> - ret := make([]LibnbdExtent, count)
> - for i := 0; i < int (count); i++ {
> - ret[i].Length = uint64 (slice[i].length)
> - ret[i].Flags = uint64 (slice[i].flags)
> + ret := make ([]LibnbdExtent, count)
> + addr := uintptr (unsafe.Pointer (entries))
> + var i uint64 = 0
> + for i < uint64 (count) {
> + ptr := (*C.nbd_extent)(unsafe.Pointer (addr))
> +
> + ret[i].Length = uint64 ((*ptr).length)
> + ret[i].Flags = uint64 ((*ptr).flags)
> +
> + addr += unsafe.Sizeof (*ptr)
> + i++
That sentiment is further strengthened by the fact that neither my
original proposal nor your replacement can use copy() for the struct
purposes. Even if copy() can speed up []uint32 copies, I'd rather let
the Go compiler worry about vectorizing the loop if we can't find an
obvious library function that already takes advantage of hardware
vectors.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org