Skip to content

Commit 8c4c4ae

Browse files
pgimalacstapelberg
pgimalac
authored and
stapelberg
committed
internal/descfmt: replace uses of MethodByName with direct calls when possible
Update internal/descfmt/stringer.go to avoid calling MethodByName when possible. MethodByName allows calling a method even when the signature is unknown, but it forces the linker to keep all methods with a matching name, increasing binary size with dead code. On the other hand casting to an interface only keeps methods with a matching signature. Using MethodByName also bypasses compiler typing, so making a direct call is safer. In this specific case, the code checks whether a type implements an interface or is a specific type before using MethodByName and FieldByName to access its methods and fields, so we can just access them directly. I checked the size difference for the datadog-agent and kubernetes binaries (ranging from 20 to 100MiB), and the resulting gains were between 100 and 300kiB, which is small but not negligible. This is a follow-up of https://go.dev/cl/734280. Fixes golang/protobuf#1708 Change-Id: Ib6ca16f7f7a871c8625d934c6115b47e525a42e9 Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/735401 Reviewed-by: Lasse Folger <lassefolger@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Michael Stapelberg <stapelberg@google.com>
1 parent 9b1c943 commit 8c4c4ae

1 file changed

Lines changed: 83 additions & 96 deletions

File tree

‎internal/descfmt/stringer.go‎

Lines changed: 83 additions & 96 deletions
Original file line numberDiff line numberDiff line change
@@ -83,12 +83,13 @@ func formatListOpt(vs list, isRoot, allowMulti bool) string {
8383
case protoreflect.FileImports:
8484
for i := 0; i < vs.Len(); i++ {
8585
var rs records
86-
rv := reflect.ValueOf(vs.Get(i))
87-
rs.Append(rv, []methodAndName{
88-
{rv.MethodByName("Path"), "Path"},
89-
{rv.MethodByName("Package"), "Package"},
90-
{rv.MethodByName("IsPublic"), "IsPublic"},
91-
{rv.MethodByName("IsWeak"), "IsWeak"},
86+
fi := vs.Get(i)
87+
rv := reflect.ValueOf(fi)
88+
rs.Append(rv, []attrAndName{
89+
{fi.Path(), "Path"},
90+
{fi.Package(), "Package"},
91+
{fi.IsPublic, "IsPublic"},
92+
{fi.IsWeak, "IsWeak"},
9293
}...)
9394
ss = append(ss, "{"+rs.Join()+"}")
9495
}
@@ -104,9 +105,9 @@ func formatListOpt(vs list, isRoot, allowMulti bool) string {
104105
}
105106
}
106107

107-
type methodAndName struct {
108-
method reflect.Value
109-
name string
108+
type attrAndName struct {
109+
attr any
110+
name string
110111
}
111112

112113
func FormatDesc(s fmt.State, r rune, t protoreflect.Descriptor) {
@@ -126,58 +127,58 @@ func formatDescOpt(t protoreflect.Descriptor, isRoot, allowMulti bool, record fu
126127
start = rt.Name() + "{"
127128
}
128129

129-
_, isFile := t.(protoreflect.FileDescriptor)
130+
fd, isFile := t.(protoreflect.FileDescriptor)
130131
rs := records{
131132
allowMulti: allowMulti,
132133
record: record,
133134
}
134135
if t.IsPlaceholder() {
135136
if isFile {
136-
rs.Append(rv, []methodAndName{
137-
{rv.MethodByName("Path"), "Path"},
138-
{rv.MethodByName("Package"), "Package"},
139-
{rv.MethodByName("IsPlaceholder"), "IsPlaceholder"},
137+
rs.Append(rv, []attrAndName{
138+
{fd.Path(), "Path"},
139+
{fd.Package(), "Package"},
140+
{fd.IsPlaceholder(), "IsPlaceholder"},
140141
}...)
141142
} else {
142-
rs.Append(rv, []methodAndName{
143-
{rv.MethodByName("FullName"), "FullName"},
144-
{rv.MethodByName("IsPlaceholder"), "IsPlaceholder"},
143+
rs.Append(rv, []attrAndName{
144+
{t.FullName(), "FullName"},
145+
{t.IsPlaceholder(), "IsPlaceholder"},
145146
}...)
146147
}
147148
} else {
148149
switch {
149150
case isFile:
150-
rs.Append(rv, methodAndName{rv.MethodByName("Syntax"), "Syntax"})
151+
rs.Append(rv, attrAndName{fd.Syntax(), "Syntax"})
151152
case isRoot:
152-
rs.Append(rv, []methodAndName{
153-
{rv.MethodByName("Syntax"), "Syntax"},
154-
{rv.MethodByName("FullName"), "FullName"},
153+
rs.Append(rv, []attrAndName{
154+
{t.Syntax(), "Syntax"},
155+
{t.FullName(), "FullName"},
155156
}...)
156157
default:
157-
rs.Append(rv, methodAndName{rv.MethodByName("Name"), "Name"})
158+
rs.Append(rv, attrAndName{t.Name(), "Name"})
158159
}
159160
switch t := t.(type) {
160161
case protoreflect.FieldDescriptor:
161-
accessors := []methodAndName{
162-
{rv.MethodByName("Number"), "Number"},
163-
{rv.MethodByName("Cardinality"), "Cardinality"},
164-
{rv.MethodByName("Kind"), "Kind"},
165-
{rv.MethodByName("HasJSONName"), "HasJSONName"},
166-
{rv.MethodByName("JSONName"), "JSONName"},
167-
{rv.MethodByName("HasPresence"), "HasPresence"},
168-
{rv.MethodByName("IsExtension"), "IsExtension"},
169-
{rv.MethodByName("IsPacked"), "IsPacked"},
170-
{rv.MethodByName("IsWeak"), "IsWeak"},
171-
{rv.MethodByName("IsList"), "IsList"},
172-
{rv.MethodByName("IsMap"), "IsMap"},
173-
{rv.MethodByName("MapKey"), "MapKey"},
174-
{rv.MethodByName("MapValue"), "MapValue"},
175-
{rv.MethodByName("HasDefault"), "HasDefault"},
176-
{rv.MethodByName("Default"), "Default"},
177-
{rv.MethodByName("ContainingOneof"), "ContainingOneof"},
178-
{rv.MethodByName("ContainingMessage"), "ContainingMessage"},
179-
{rv.MethodByName("Message"), "Message"},
180-
{rv.MethodByName("Enum"), "Enum"},
162+
accessors := []attrAndName{
163+
{t.Number(), "Number"},
164+
{t.Cardinality(), "Cardinality"},
165+
{t.Kind(), "Kind"},
166+
{t.HasJSONName(), "HasJSONName"},
167+
{t.JSONName(), "JSONName"},
168+
{t.HasPresence(), "HasPresence"},
169+
{t.IsExtension(), "IsExtension"},
170+
{t.IsPacked(), "IsPacked"},
171+
{t.IsWeak(), "IsWeak"},
172+
{t.IsList(), "IsList"},
173+
{t.IsMap(), "IsMap"},
174+
{t.MapKey(), "MapKey"},
175+
{t.MapValue(), "MapValue"},
176+
{t.HasDefault(), "HasDefault"},
177+
{t.Default(), "Default"},
178+
{t.ContainingOneof(), "ContainingOneof"},
179+
{t.ContainingMessage(), "ContainingMessage"},
180+
{t.Message(), "Message"},
181+
{t.Enum(), "Enum"},
181182
}
182183
for _, s := range accessors {
183184
switch s.name {
@@ -223,62 +224,54 @@ func formatDescOpt(t protoreflect.Descriptor, isRoot, allowMulti bool, record fu
223224
}
224225

225226
case protoreflect.FileDescriptor:
226-
rs.Append(rv, []methodAndName{
227-
{rv.MethodByName("Path"), "Path"},
228-
{rv.MethodByName("Package"), "Package"},
229-
{rv.MethodByName("Imports"), "Imports"},
230-
{rv.MethodByName("Messages"), "Messages"},
231-
{rv.MethodByName("Enums"), "Enums"},
232-
{rv.MethodByName("Extensions"), "Extensions"},
233-
{rv.MethodByName("Services"), "Services"},
227+
rs.Append(rv, []attrAndName{
228+
{t.Path(), "Path"},
229+
{t.Package(), "Package"},
230+
{t.Imports(), "Imports"},
231+
{t.Messages(), "Messages"},
232+
{t.Enums(), "Enums"},
233+
{t.Extensions(), "Extensions"},
234+
{t.Services(), "Services"},
234235
}...)
235236

236237
case protoreflect.MessageDescriptor:
237-
rs.Append(rv, []methodAndName{
238-
{rv.MethodByName("IsMapEntry"), "IsMapEntry"},
239-
{rv.MethodByName("Fields"), "Fields"},
240-
{rv.MethodByName("Oneofs"), "Oneofs"},
241-
{rv.MethodByName("ReservedNames"), "ReservedNames"},
242-
{rv.MethodByName("ReservedRanges"), "ReservedRanges"},
243-
{rv.MethodByName("RequiredNumbers"), "RequiredNumbers"},
244-
{rv.MethodByName("ExtensionRanges"), "ExtensionRanges"},
245-
{rv.MethodByName("Messages"), "Messages"},
246-
{rv.MethodByName("Enums"), "Enums"},
247-
{rv.MethodByName("Extensions"), "Extensions"},
238+
rs.Append(rv, []attrAndName{
239+
{t.IsMapEntry(), "IsMapEntry"},
240+
{t.Fields(), "Fields"},
241+
{t.Oneofs(), "Oneofs"},
242+
{t.ReservedNames(), "ReservedNames"},
243+
{t.ReservedRanges(), "ReservedRanges"},
244+
{t.RequiredNumbers(), "RequiredNumbers"},
245+
{t.ExtensionRanges(), "ExtensionRanges"},
246+
{t.Messages(), "Messages"},
247+
{t.Enums(), "Enums"},
248+
{t.Extensions(), "Extensions"},
248249
}...)
249250

250251
case protoreflect.EnumDescriptor:
251-
rs.Append(rv, []methodAndName{
252-
{rv.MethodByName("Values"), "Values"},
253-
{rv.MethodByName("ReservedNames"), "ReservedNames"},
254-
{rv.MethodByName("ReservedRanges"), "ReservedRanges"},
255-
{rv.MethodByName("IsClosed"), "IsClosed"},
252+
rs.Append(rv, []attrAndName{
253+
{t.Values(), "Values"},
254+
{t.ReservedNames(), "ReservedNames"},
255+
{t.ReservedRanges(), "ReservedRanges"},
256+
{t.IsClosed(), "IsClosed"},
256257
}...)
257258

258259
case protoreflect.EnumValueDescriptor:
259-
rs.Append(rv, []methodAndName{
260-
{rv.MethodByName("Number"), "Number"},
261-
}...)
260+
rs.Append(rv, attrAndName{t.Number(), "Number"})
262261

263262
case protoreflect.ServiceDescriptor:
264-
// MethodByName(<constant-string>) makes the linker keep all methods with
265-
// the given name for all reachable types.
266-
// In particular for the name "Methods" it means the linker will keep
267-
// `reflect.Value.Methods()` and `reflect.Type.Methods()` with Go 1.26+,
268-
// which disable method dead code elimination entirely.
269-
// So we avoid using MethodByName for Methods.
270-
rs.appendCallResult(rv, "Methods", reflect.ValueOf(t.Methods()))
263+
rs.Append(rv, attrAndName{t.Methods(), "Methods"})
271264

272265
case protoreflect.MethodDescriptor:
273-
rs.Append(rv, []methodAndName{
274-
{rv.MethodByName("Input"), "Input"},
275-
{rv.MethodByName("Output"), "Output"},
276-
{rv.MethodByName("IsStreamingClient"), "IsStreamingClient"},
277-
{rv.MethodByName("IsStreamingServer"), "IsStreamingServer"},
266+
rs.Append(rv, []attrAndName{
267+
{t.Input(), "Input"},
268+
{t.Output(), "Output"},
269+
{t.IsStreamingClient(), "IsStreamingClient"},
270+
{t.IsStreamingServer(), "IsStreamingServer"},
278271
}...)
279272
}
280-
if m := rv.MethodByName("GoType"); m.IsValid() {
281-
rs.Append(rv, methodAndName{m, "GoType"})
273+
if m, ok := t.(interface{ GoType() reflect.Type }); ok {
274+
rs.Append(rv, attrAndName{m.GoType(), "GoType"})
282275
}
283276
}
284277
return start + rs.Join() + end
@@ -301,26 +294,20 @@ func (rs *records) AppendRecs(fieldName string, newRecs [2]string) {
301294
rs.recs = append(rs.recs, newRecs)
302295
}
303296

304-
func (rs *records) Append(v reflect.Value, accessors ...methodAndName) {
305-
for _, a := range accessors {
306-
var rv reflect.Value
307-
if a.method.IsValid() {
308-
rv = a.method.Call(nil)[0]
309-
}
310-
rs.appendCallResult(v, a.name, rv)
297+
func (rs *records) Append(v reflect.Value, results ...attrAndName) {
298+
for _, r := range results {
299+
rs.appendAttribute(v, r.name, r.attr)
311300
}
312301
}
313302

314-
func (rs *records) appendCallResult(val reflect.Value, name string, rv reflect.Value) {
303+
func (rs *records) appendAttribute(val reflect.Value, name string, attrVal any) {
315304
if rs.record != nil {
316305
rs.record(name)
317306
}
318-
if val.Kind() == reflect.Struct && !rv.IsValid() {
319-
rv = val.FieldByName(name)
320-
}
321-
if !rv.IsValid() {
322-
panic(fmt.Sprintf("unknown accessor: %v.%s", val.Type(), name))
307+
if attrVal == nil {
308+
return
323309
}
310+
rv := reflect.ValueOf(attrVal)
324311
if _, ok := rv.Interface().(protoreflect.Value); ok {
325312
rv = rv.MethodByName("Interface").Call(nil)[0]
326313
if !rv.IsNil() {

0 commit comments

Comments
 (0)