On Thu, Aug 3, 2023 at 4:57 AM Eric Blake <ebl...@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. And
Why 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 <ler...@redhat.com>
> CC: Nir Soffer <nsof...@redhat.com>
> Signed-off-by: Eric Blake <ebl...@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)
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
