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

[Eric Blake <eblake@redhat.com>]

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