-
-
Notifications
You must be signed in to change notification settings - Fork 148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
does not build with Go HEAD #506
Comments
The buildstats package (used by coordinator for its internal needs) currently fails to build at Go tip because one of its dependencies relies on unexported internals of the reflect package not changing. See goccy/go-json#506. The coordinator is deployed with Go 1.21 now, and probably won't ever get to be deployed with Go 1.23 since it will be deleted after the LUCI migration is complete and it stops being needed. Delete the buildstats implementation for now to avoid the breakage in x/build at Go tip. Change-Id: Ica48eeb5ebe9f82cedda9385ced5d7a00d32b377 Reviewed-on: https://go-review.googlesource.com/c/build/+/584299 Reviewed-by: Michael Knyszek <[email protected]> Auto-Submit: Dmitri Shuralyov <[email protected]> Reviewed-by: Dmitri Shuralyov <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
CL 583755 removed all uses of the ifaceIndir function, and the function itself. Unfortunately, ifaceIndir is accessed using go:linkname by the popular github.com/goccy/go-json package. A bug has been filed to fix this upstream: goccy/go-json#506 Until that bug is fixed and the fix is distributed, keep this function available. With luck we can remove this in the 1.24 release. For #67279 Change-Id: I15fccf82d7a172a0b15cdbefb0a0a48381998938 Reviewed-on: https://go-review.googlesource.com/c/go/+/584676 Reviewed-by: Dmitri Shuralyov <[email protected]> Auto-Submit: Ian Lance Taylor <[email protected]> Reviewed-by: Dmitri Shuralyov <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Ian Lance Taylor <[email protected]>
This library has 47 Alternatively, I'm not sure that much can be done in terms of new Go versions not breaking you. It is bound to happen when you depend on dozens of internal reflect and runtime implementation details. @ianlancetaylor personally, I would not restore functions like |
@ianlancetaylor Thank you for notification to me. Since the current code related to the reflect package can be replaced with the use of only public APIs, I plan to address this soon. |
Previously, there was a trick implemented to avoid the forced escaping of argument values with |
Also, as a general recommendation. Have every linkname & poking into Go runtime protected by a specific Go version tag, because there's no guarantee on how map, reflect etc. internals are implemented in the future. (e.g. see upcoming swissmap in https://go-review.googlesource.com/c/go/+/580778). |
Thank you. I'm already aware of that, and I implemented it with that understanding. |
Is there a way to iter a map with zero-alloc? |
@trim21 Yes: use the |
Thanks. for future readers, it would look like this: it will have 3 allocs/op for map with any size func(ctx *Ctx, b []byte, rv reflect.Value) ([]byte, error) {
if rv.IsNil() {
return appendNull(b), nil
}
size := rv.Len()
b = appendArrayBegin(b, int64(size))
iter := rv.MapRange()
kv := reflect.New(keyType).Elem()
vv := reflect.New(valueType).Elem()
for iter.Next() {
kv.SetIterKey(iter)
b, err = keyEncoder(ctx, b, kv)
if err != nil {
return b, err
}
vv.SetIterValue(iter)
b, err = valueEncoder(ctx, b, vv)
if err != nil {
return b, err
}
}
return append(b, '}'), nil
} |
This package does some extremely unsafe and unsupported operations. One of them has broken with Go HEAD, the future 1.23 release.
The internal function
reflect.ifaceIndir
no longer exists. This package should not be referencing it.If there are performance issues with the reflect package, please file them upstream. Please don't try to work around them using unsupported operations. Thanks.
The text was updated successfully, but these errors were encountered: