On 8/4/23 11:16, Richard W.M. Jones wrote:
> 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 <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\")
>> + }
>
> 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\")
Sounds helpful!
>
>> 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 <rjo...@redhat.com>
Reviewed-by: Laszlo Ersek <ler...@redhat.com>
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
