Laszlo Ersek
2023-Jul-27 11:23 UTC
[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 at redhat.com>I've needed to dig a bit, but indeed, bullet (8) in: http://mid.mail-archive.com/0e4ff751-88d6-837b-15a5-6f6c370a2f09 at redhat.com https://listman.redhat.com/archives/libguestfs/2023-June/031736.html> CC: Nir Soffer <nsoffer at redhat.com> > Signed-off-by: Eric Blake <eblake at redhat.com> > --- > 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. Thanks, Laszlo
Eric Blake
2023-Jul-27 17:36 UTC
[Libguestfs] [libnbd PATCH 4/7] Revert "generator/Go.ml: Simplify copy_uint32_array"
On Thu, Jul 27, 2023 at 01:23:24PM +0200, Laszlo Ersek wrote:> 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 at redhat.com> > > I've needed to dig a bit, but indeed, bullet (8) in: > > http://mid.mail-archive.com/0e4ff751-88d6-837b-15a5-6f6c370a2f09 at redhat.com > https://listman.redhat.com/archives/libguestfs/2023-June/031736.html > > > CC: Nir Soffer <nsoffer at redhat.com> > > Signed-off-by: Eric Blake <eblake at redhat.com> > > --- > > 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.Fair enough. It is undoing the effects of the earlier patch, but not in the same way as a straight revert (in part because enough else has changed in the meantime that a straight revert is no longer possible). Plus the decision on where to stage this in the series (at one point, I had it after 6/7 - and you already noted there how much rebase churn gets caused as we mix this series around).> > 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.)I completely missed that point. You do make a valid point that a C size_t might be larger than a Go int (does Go even try to run on 32-bit platforms?) - but we DO have the additional knowledge that because our block status reply results cannot exceed 64M over the wire, any count passed to the callback function will fit in 32 bits regardless of the width of C's size_t. Maybe an assertion that count does not lose precision when cast to Go int is sufficient?> > 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++ {I don't know enough Go syntax to quickly state if there is some other way to coerce a := expression into having a guaranteed 64-bit native type.> ... > } > > 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.Okay, I'll need to rethink how to do this patch (perhaps by rearranging it to occur after 7/7, by folding it into the rest of the 64-bit series where I add the nbd_extent list conversion code). -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org