6:36 a.m.
On Thu, Aug 3, 2023 at 4:57 AM Eric Blake <eblake(a)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 <lersek(a)redhat.com>
CC: Nir Soffer <nsoffer(a)redhat.com>
Signed-off-by: Eric Blake <eblake(a)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)