Cache reflect.MethodByName

The isolated benchmark for the function is obviously much faster:

```bash
name                old time/op    new time/op    delta
GetMethodByName-10    1.21µs ± 7%    0.23µs ± 5%   -81.42%  (p=0.029 n=4+4)

name                old alloc/op   new alloc/op   delta
GetMethodByName-10      680B ± 0%        0B       -100.00%  (p=0.029 n=4+4)

name                old allocs/op  new allocs/op  delta
GetMethodByName-10      20.0 ± 0%       0.0       -100.00%  (p=0.029 n=4+4)
```

But more pleasing is the overall performance looking at the site benchmarks:

```bash
name                                      old time/op    new time/op    delta
SiteNew/Regular_Bundle_with_image-10        6.25ms ± 2%    6.10ms ± 2%     ~     (p=0.057 n=4+4)
SiteNew/Regular_Bundle_with_JSON_file-10    6.30ms ± 2%    5.66ms ±11%     ~     (p=0.057 n=4+4)
SiteNew/Regular_Tags_and_categories-10      22.2ms ± 2%    17.4ms ± 1%  -21.88%  (p=0.029 n=4+4)
SiteNew/Regular_Canonify_URLs-10             108ms ± 0%     107ms ± 0%   -1.20%  (p=0.029 n=4+4)
SiteNew/Regular_Deep_content_tree-10        36.1ms ± 1%    33.8ms ± 1%   -6.44%  (p=0.029 n=4+4)
SiteNew/Regular_TOML_front_matter-10        24.9ms ± 1%    22.6ms ± 1%   -9.30%  (p=0.029 n=4+4)
SiteNew/Regular_Many_HTML_templates-10      17.9ms ± 1%    16.7ms ± 1%   -6.43%  (p=0.029 n=4+4)
SiteNew/Regular_Page_collections-10         23.3ms ± 1%    22.0ms ± 0%   -5.58%  (p=0.029 n=4+4)
SiteNew/Regular_List_terms-10               8.00ms ± 1%    7.63ms ± 0%   -4.62%  (p=0.029 n=4+4)

name                                      old alloc/op   new alloc/op   delta
SiteNew/Regular_Bundle_with_image-10        2.10MB ± 0%    2.07MB ± 0%   -1.46%  (p=0.029 n=4+4)
SiteNew/Regular_Bundle_with_JSON_file-10    1.88MB ± 0%    1.85MB ± 0%   -1.76%  (p=0.029 n=4+4)
SiteNew/Regular_Tags_and_categories-10      13.5MB ± 0%    11.6MB ± 0%  -13.99%  (p=0.029 n=4+4)
SiteNew/Regular_Canonify_URLs-10            96.1MB ± 0%    95.8MB ± 0%   -0.40%  (p=0.029 n=4+4)
SiteNew/Regular_Deep_content_tree-10        28.4MB ± 0%    27.3MB ± 0%   -3.83%  (p=0.029 n=4+4)
SiteNew/Regular_TOML_front_matter-10        16.9MB ± 0%    15.1MB ± 0%  -10.58%  (p=0.029 n=4+4)
SiteNew/Regular_Many_HTML_templates-10      8.98MB ± 0%    8.44MB ± 0%   -6.04%  (p=0.029 n=4+4)
SiteNew/Regular_Page_collections-10         17.1MB ± 0%    16.5MB ± 0%   -3.91%  (p=0.029 n=4+4)
SiteNew/Regular_List_terms-10               3.92MB ± 0%    3.72MB ± 0%   -5.03%  (p=0.029 n=4+4)

name                                      old allocs/op  new allocs/op  delta
SiteNew/Regular_Bundle_with_image-10         25.8k ± 0%     24.9k ± 0%   -3.49%  (p=0.029 n=4+4)
SiteNew/Regular_Bundle_with_JSON_file-10     25.8k ± 0%     24.9k ± 0%   -3.49%  (p=0.029 n=4+4)
SiteNew/Regular_Tags_and_categories-10        288k ± 0%      233k ± 0%  -18.90%  (p=0.029 n=4+4)
SiteNew/Regular_Canonify_URLs-10              375k ± 0%      364k ± 0%   -2.80%  (p=0.029 n=4+4)
SiteNew/Regular_Deep_content_tree-10          314k ± 0%      283k ± 0%   -9.77%  (p=0.029 n=4+4)
SiteNew/Regular_TOML_front_matter-10          302k ± 0%      252k ± 0%  -16.55%  (p=0.029 n=4+4)
SiteNew/Regular_Many_HTML_templates-10        133k ± 0%      117k ± 0%  -11.81%  (p=0.029 n=4+4)
SiteNew/Regular_Page_collections-10           202k ± 0%      183k ± 0%   -9.55%  (p=0.029 n=4+4)
SiteNew/Regular_List_terms-10                55.6k ± 0%     49.8k ± 0%  -10.40%  (p=0.029 n=4+4)
```

Thanks to @quasilyte for the suggestion.

Fixes 9386
This commit is contained in:
Bjørn Erik Pedersen 2022-03-08 10:06:12 +01:00
parent ff02d41721
commit 4576c82ed4
9 changed files with 117 additions and 17 deletions

View file

@ -19,6 +19,7 @@ package hreflect
import ( import (
"context" "context"
"reflect" "reflect"
"sync"
"github.com/gohugoio/hugo/common/types" "github.com/gohugoio/hugo/common/types"
) )
@ -115,6 +116,58 @@ func IsTruthfulValue(val reflect.Value) (truth bool) {
return return
} }
type methodKey struct {
typ reflect.Type
name string
}
type methods struct {
sync.RWMutex
cache map[methodKey]int
}
var methodCache = &methods{cache: make(map[methodKey]int)}
// GetMethodByName is the samve as reflect.Value.MethodByName, but it caches the
// type lookup.
func GetMethodByName(v reflect.Value, name string) reflect.Value {
index := GetMethodIndexByName(v.Type(), name)
if index == -1 {
return reflect.Value{}
}
return v.Method(index)
}
// GetMethodIndexByName returns the index of the method with the given name, or
// -1 if no such method exists.
func GetMethodIndexByName(tp reflect.Type, name string) int {
k := methodKey{tp, name}
methodCache.RLock()
index, found := methodCache.cache[k]
methodCache.RUnlock()
if found {
return index
}
methodCache.Lock()
defer methodCache.Unlock()
m, ok := tp.MethodByName(name)
index = m.Index
if !ok {
index = -1
}
methodCache.cache[k] = index
if !ok {
return -1
}
return m.Index
}
// Based on: https://github.com/golang/go/blob/178a2c42254166cffed1b25fb1d3c7a5727cada6/src/text/template/exec.go#L931 // Based on: https://github.com/golang/go/blob/178a2c42254166cffed1b25fb1d3c7a5727cada6/src/text/template/exec.go#L931
func indirectInterface(v reflect.Value) reflect.Value { func indirectInterface(v reflect.Value) reflect.Value {
if v.Kind() != reflect.Interface { if v.Kind() != reflect.Interface {

View file

@ -30,6 +30,16 @@ func TestIsTruthful(t *testing.T) {
c.Assert(IsTruthful(time.Time{}), qt.Equals, false) c.Assert(IsTruthful(time.Time{}), qt.Equals, false)
} }
func TestGetMethodByName(t *testing.T) {
c := qt.New(t)
v := reflect.ValueOf(&testStruct{})
tp := v.Type()
c.Assert(GetMethodIndexByName(tp, "Method1"), qt.Equals, 0)
c.Assert(GetMethodIndexByName(tp, "Method3"), qt.Equals, 2)
c.Assert(GetMethodIndexByName(tp, "Foo"), qt.Equals, -1)
}
func BenchmarkIsTruthFul(b *testing.B) { func BenchmarkIsTruthFul(b *testing.B) {
v := reflect.ValueOf("Hugo") v := reflect.ValueOf("Hugo")
@ -40,3 +50,37 @@ func BenchmarkIsTruthFul(b *testing.B) {
} }
} }
} }
type testStruct struct{}
func (t *testStruct) Method1() string {
return "Hugo"
}
func (t *testStruct) Method2() string {
return "Hugo"
}
func (t *testStruct) Method3() string {
return "Hugo"
}
func (t *testStruct) Method4() string {
return "Hugo"
}
func (t *testStruct) Method5() string {
return "Hugo"
}
func BenchmarkGetMethodByName(b *testing.B) {
v := reflect.ValueOf(&testStruct{})
methods := []string{"Method1", "Method2", "Method3", "Method4", "Method5"}
b.ResetTimer()
for i := 0; i < b.N; i++ {
for _, method := range methods {
_ = GetMethodByName(v, method)
}
}
}

View file

@ -160,7 +160,7 @@ func getPluralCount(v interface{}) interface{} {
if f.IsValid() { if f.IsValid() {
return toPluralCountValue(f.Interface()) return toPluralCountValue(f.Interface())
} }
m := vv.MethodByName(countFieldName) m := hreflect.GetMethodByName(vv, countFieldName)
if m.IsValid() && m.Type().NumIn() == 0 && m.Type().NumOut() == 1 { if m.IsValid() && m.Type().NumIn() == 0 && m.Type().NumOut() == 1 {
c := m.Call(nil) c := m.Call(nil)
return toPluralCountValue(c[0].Interface()) return toPluralCountValue(c[0].Interface())
@ -169,7 +169,6 @@ func getPluralCount(v interface{}) interface{} {
} }
return toPluralCountValue(v) return toPluralCountValue(v)
} }
// go-i18n expects floats to be represented by string. // go-i18n expects floats to be represented by string.

View file

@ -24,6 +24,7 @@ import (
"github.com/spf13/cast" "github.com/spf13/cast"
"github.com/gohugoio/hugo/common/collections" "github.com/gohugoio/hugo/common/collections"
"github.com/gohugoio/hugo/common/hreflect"
"github.com/gohugoio/hugo/compare" "github.com/gohugoio/hugo/compare"
"github.com/gohugoio/hugo/resources/resource" "github.com/gohugoio/hugo/resources/resource"
@ -112,8 +113,9 @@ func (p Pages) GroupBy(key string, order ...string) (PagesGroup, error) {
} }
var ft interface{} var ft interface{}
m, ok := pagePtrType.MethodByName(key) index := hreflect.GetMethodIndexByName(pagePtrType, key)
if ok { if index != -1 {
m := pagePtrType.Method(index)
if m.Type.NumOut() == 0 || m.Type.NumOut() > 2 { if m.Type.NumOut() == 0 || m.Type.NumOut() > 2 {
return nil, errors.New(key + " is a Page method but you can't use it with GroupBy") return nil, errors.New(key + " is a Page method but you can't use it with GroupBy")
} }
@ -125,6 +127,7 @@ func (p Pages) GroupBy(key string, order ...string) (PagesGroup, error) {
} }
ft = m ft = m
} else { } else {
var ok bool
ft, ok = pagePtrType.Elem().FieldByName(key) ft, ok = pagePtrType.Elem().FieldByName(key)
if !ok { if !ok {
return nil, errors.New(key + " is neither a field nor a method of Page") return nil, errors.New(key + " is neither a field nor a method of Page")
@ -146,7 +149,7 @@ func (p Pages) GroupBy(key string, order ...string) (PagesGroup, error) {
case reflect.StructField: case reflect.StructField:
fv = ppv.Elem().FieldByName(key) fv = ppv.Elem().FieldByName(key)
case reflect.Method: case reflect.Method:
fv = ppv.MethodByName(key).Call([]reflect.Value{})[0] fv = hreflect.GetMethodByName(ppv, key).Call([]reflect.Value{})[0]
} }
if !fv.IsValid() { if !fv.IsValid() {
continue continue

View file

@ -21,6 +21,7 @@ import (
"github.com/spf13/cast" "github.com/spf13/cast"
"github.com/gohugoio/hugo/common/hreflect"
"github.com/gohugoio/hugo/common/maps" "github.com/gohugoio/hugo/common/maps"
"github.com/gohugoio/hugo/media" "github.com/gohugoio/hugo/media"
"github.com/gohugoio/hugo/resources/resource" "github.com/gohugoio/hugo/resources/resource"
@ -125,7 +126,7 @@ func (r *PostPublishResource) fieldToString(receiver interface{}, path string) s
default: default:
v := receiverv.FieldByName(fieldname) v := receiverv.FieldByName(fieldname)
if !v.IsValid() { if !v.IsValid() {
method := receiverv.MethodByName(fieldname) method := hreflect.GetMethodByName(receiverv, fieldname)
if method.IsValid() { if method.IsValid() {
vals := method.Call(nil) vals := method.Call(nil)
if len(vals) > 0 { if len(vals) > 0 {

View file

@ -131,7 +131,7 @@ func (ns *Namespace) lookupFunc(fname string) (reflect.Value, bool) {
nv = reflect.ValueOf(v) nv = reflect.ValueOf(v)
// method // method
m := nv.MethodByName(ss[1]) m := hreflect.GetMethodByName(nv, ss[1])
if m.Kind() == reflect.Invalid { if m.Kind() == reflect.Invalid {
return reflect.Value{}, false return reflect.Value{}, false

View file

@ -19,6 +19,7 @@ import (
"reflect" "reflect"
"strings" "strings"
"github.com/gohugoio/hugo/common/hreflect"
"github.com/gohugoio/hugo/common/maps" "github.com/gohugoio/hugo/common/maps"
) )
@ -300,8 +301,9 @@ func evaluateSubElem(obj reflect.Value, elemName string) (reflect.Value, error)
objPtr = objPtr.Addr() objPtr = objPtr.Addr()
} }
mt, ok := objPtr.Type().MethodByName(elemName) index := hreflect.GetMethodIndexByName(objPtr.Type(), elemName)
if ok { if index != -1 {
mt := objPtr.Type().Method(index)
switch { switch {
case mt.PkgPath != "": case mt.PkgPath != "":
return zero, fmt.Errorf("%s is an unexported method of type %s", elemName, typ) return zero, fmt.Errorf("%s is an unexported method of type %s", elemName, typ)
@ -513,5 +515,5 @@ func toTimeUnix(v reflect.Value) int64 {
if v.Type() != timeType { if v.Type() != timeType {
panic("coding error: argument must be time.Time type reflect Value") panic("coding error: argument must be time.Time type reflect Value")
} }
return v.MethodByName("Unix").Call([]reflect.Value{})[0].Int() return hreflect.GetMethodByName(v, "Unix").Call([]reflect.Value{})[0].Int()
} }

View file

@ -21,6 +21,7 @@ import (
"testing" "testing"
qt "github.com/frankban/quicktest" qt "github.com/frankban/quicktest"
"github.com/gohugoio/hugo/common/hreflect"
) )
type TestStruct struct { type TestStruct struct {
@ -59,7 +60,7 @@ func (e *execHelper) GetMethod(ctx context.Context, tmpl Preparer, receiver refl
if name != "Hello1" { if name != "Hello1" {
return zero, zero return zero, zero
} }
m := receiver.MethodByName("Hello2") m := hreflect.GetMethodByName(receiver, "Hello2")
return m, reflect.ValueOf("v2") return m, reflect.ValueOf("v2")
} }

View file

@ -20,9 +20,9 @@ import (
"reflect" "reflect"
"strings" "strings"
"github.com/gohugoio/hugo/tpl" "github.com/gohugoio/hugo/common/hreflect"
"github.com/gohugoio/hugo/common/maps" "github.com/gohugoio/hugo/common/maps"
"github.com/gohugoio/hugo/tpl"
template "github.com/gohugoio/hugo/tpl/internal/go_templates/htmltemplate" template "github.com/gohugoio/hugo/tpl/internal/go_templates/htmltemplate"
texttemplate "github.com/gohugoio/hugo/tpl/internal/go_templates/texttemplate" texttemplate "github.com/gohugoio/hugo/tpl/internal/go_templates/texttemplate"
@ -111,9 +111,6 @@ func (t *templateExecHelper) GetMapValue(ctx context.Context, tmpl texttemplate.
func (t *templateExecHelper) GetMethod(ctx context.Context, tmpl texttemplate.Preparer, receiver reflect.Value, name string) (method reflect.Value, firstArg reflect.Value) { func (t *templateExecHelper) GetMethod(ctx context.Context, tmpl texttemplate.Preparer, receiver reflect.Value, name string) (method reflect.Value, firstArg reflect.Value) {
if t.running { if t.running {
// This is a hot path and receiver.MethodByName really shows up in the benchmarks,
// so we maintain a list of method names with that signature.
// TODO(bep) I have a branch that makes this construct superflous.
switch name { switch name {
case "GetPage", "Render": case "GetPage", "Render":
if info, ok := tmpl.(tpl.Info); ok { if info, ok := tmpl.(tpl.Info); ok {
@ -124,7 +121,7 @@ func (t *templateExecHelper) GetMethod(ctx context.Context, tmpl texttemplate.Pr
} }
} }
fn := receiver.MethodByName(name) fn := hreflect.GetMethodByName(receiver, name)
if !fn.IsValid() { if !fn.IsValid() {
return zero, zero return zero, zero
} }