Re: [Libguestfs] [libnbd PATCH v3 07/22] generator: Add struct nbd_extent in prep for 64-bit extents

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.