Re: [Libguestfs] [libnbd PATCH 4/7] Revert "generator/Go.ml: Simplify copy_uint32_array"

On 7/26/23 19:50, Eric Blake wrote: > This reverts commit 6725fa0e129f9a60d7b89707ef8604e0aeeeaf43, although > with a more modern style. > > Casting a C array to a Go slice just to benefit from potential > optimizations in Go's copy(), is rather complex to understand, > especially when we are still copying things (the main reason to treat > a C array as a Go slice is when avoiding a copy has a benefit). > Without a benchmark showing measurable difference in runtime speed, > and considering that network transit time probably dominates the time > spent on block status and its callback, it is not worth the > complexity. Furthermore, an upcoming patch wants to add a similar > helper function for converting between a list of C and Go structs, > where the copy() trick will not work; and having the two helpers look > alike is beneficial. > > Suggested-by: Laszlo Ersek <lersek(a)redhat.com&gt; I've needed to dig a bit, but indeed, bullet (8) in: http://mid.mail-archive.com/0e4ff751-88d6-837b-15a5-6f6c370a2f09@redhat.com https://listman.redhat.com/archives/libguestfs/2023-June/031736.html > CC: Nir Soffer <nsoffer(a)redhat.com&gt; > Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; > --- > generator/GoLang.ml | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/generator/GoLang.ml b/generator/GoLang.ml > index 0aa83bdc..77dacadb 100644 > --- a/generator/GoLang.ml > +++ b/generator/GoLang.ml > @@ -509,12 +509,14 @@ 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) > +func copy_uint32_array(entries *C.uint32_t, count C.size_t) []uint32 { > + ret := make([]uint32, int(count)) > + 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 > } > "; This patch mixes four things: - whitespace changes (due to style modernization / gofmt, presumably), - reverting commit 6725fa0e129f, - changes proposed in my email, - functional changes on top of my email. The "func" line matches my proposal (OK), with additional whitespace updates (OK), but has nothing to do with reverting 6725fa0e129f, so calling the patch a "revert" is misleading.

The initialization of "ret" undergoes a whitespace update (OK), but is neither a revert (6725fa0e129f did not change the initialization of "ret"), nor does it match my proposal. In my proposal, I had removed the "int" cast (or conversion) intentionally. Casting a C size_t to a Go int seems wrong. (IIRC I had verified the widths of the Go integer types from the Go documentation.)

The initialization of "addr" matches my proposal, with some whitespace updates (OK), but is not a revert of 6725fa0e129f. The "for" statement is a revert, but does not match my proposal. My proposal made sure that the loop variable was a Go uint64, so that it could accommodate the C size_t. The only Go syntax I found suitable for that was to replace the ":=" embedded in the "for", with a separate definition/initialization of "i" (where I could spell out the uint64 type), and then to use the "for" variant that was effectively a "while" (using C terminology): var i uint64 = 0 for i < uint64(count) { ... i++ } Here we cast the C size_t to uint64, which is OK. A different approach to the same end, preserving the ":=" syntax in "for", could be: for i := uint64(0); i < uint64(count); i++ {

... } This keeps the "count" cast safe, and it also forces -- I think? -- "i" to have type "uint64", by casting "0" to "uint64" in the ":=" operation. Anyway, I was super careful about those nuances when I wrote my proposal, so I'd like to stick with them. I suggest that: - we not call this patch a "revert", just update the code incrementally; perhaps reference commit 6725fa0e129f in the commit message - we stick with the exact code I proposed (unless there are specific problems with it), applying whitespace updates on top that are now required by gofmt.