Closed stapelberg closed 1 year ago
I apologize for all the MethodByName
calls. Where is my cone of shame?
For issue 3, would the following suffice?
rs.Append(rv, []methodAndName{
{rv.MethodByName("Path"), "Path"},
{rv.MethodByName("Package"), "Package"},
{rv.MethodByName("IsPlaceholder"), "IsPlaceholder"},
})
type methodAndName struct {
method reflect.Value
name string
}
func (rs *records) Append(v reflect.Value, accessors []methodAndName) {
for _, a := range accessors {
var rv reflect.Value
if a.method.IsValid() {
rv = a.method.Call(nil)[0]
}
if v.Kind() == reflect.Struct && !rv.IsValid() {
rv = v.FieldByName(a.name)
}
...
}
}
I apologize for all the
MethodByName
calls. Where is my cone of shame?
Hah, no worries! Before the toolchain change it wasn’t that big a deal :)
For issue 3, would the following suffice?
Almost: passing in the MethodByName() result is what I had in mind, too.
But, the more involved bit is around removing the descriptorAccessors map.
I sent you what I have so far: http://go.dev/cl/527896
Any thoughts on how to best adapt the test? Maybe we need to refactor formatDescOpt() so that it returns the records, and then the test could use that? Or maybe you have a more elegant idea :)
Background: dead code elimination
The Go linker looks at 3 properties of a method to determine whether it needs to be kept:
interface{ Foo() int }
results in theFoo() int
method of all types that are ever used as an interface to be kept.reflect.{Value,Type}.Method{,ByName}
, all exported methods of all reachable types are kept.What changed?
https://go.dev/cl/522436 recently landed in Go and will hopefully be released with Go 1.22.
This change introduces special handling for
reflect.Value.MethodByName()
calls where the parameter is a constant.This makes it possible to avoid condition 3 from above for many programs.
What should we change in protobuf?
We currently have a few MethodByName calls that are not constant:
If we turned all 3 into constants, dead code elimination will work better for many programs.
The first two are relatively straight-forward refactorings, the third one is a bit more effort.
I’ll try and get my change cleaned up and sent for review over the next couple of days, but if anyone is particularly eager to work on this, leave a note here and feel free to race me to it :)