From 4576c82ed462bc9c3934f76181101df1c5a4157e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B8rn=20Erik=20Pedersen?= Date: Tue, 8 Mar 2022 10:06:12 +0100 Subject: [PATCH] Cache reflect.MethodByName MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- common/hreflect/helpers.go | 53 +++++++++++++++++++ common/hreflect/helpers_test.go | 44 +++++++++++++++ langs/i18n/i18n.go | 3 +- resources/page/pagegroup.go | 9 ++-- resources/postpub/postpub.go | 3 +- tpl/collections/apply.go | 2 +- tpl/collections/where.go | 8 +-- .../texttemplate/hugo_template_test.go | 3 +- tpl/tplimpl/template_funcs.go | 9 ++-- 9 files changed, 117 insertions(+), 17 deletions(-) diff --git a/common/hreflect/helpers.go b/common/hreflect/helpers.go index beab182bb..7087a9677 100644 --- a/common/hreflect/helpers.go +++ b/common/hreflect/helpers.go @@ -19,6 +19,7 @@ package hreflect import ( "context" "reflect" + "sync" "github.com/gohugoio/hugo/common/types" ) @@ -115,6 +116,58 @@ func IsTruthfulValue(val reflect.Value) (truth bool) { 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 func indirectInterface(v reflect.Value) reflect.Value { if v.Kind() != reflect.Interface { diff --git a/common/hreflect/helpers_test.go b/common/hreflect/helpers_test.go index 480ccb27a..d16b9b9b3 100644 --- a/common/hreflect/helpers_test.go +++ b/common/hreflect/helpers_test.go @@ -30,6 +30,16 @@ func TestIsTruthful(t *testing.T) { 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) { 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) + } + } +} diff --git a/langs/i18n/i18n.go b/langs/i18n/i18n.go index e45a16822..64340c857 100644 --- a/langs/i18n/i18n.go +++ b/langs/i18n/i18n.go @@ -160,7 +160,7 @@ func getPluralCount(v interface{}) interface{} { if f.IsValid() { return toPluralCountValue(f.Interface()) } - m := vv.MethodByName(countFieldName) + m := hreflect.GetMethodByName(vv, countFieldName) if m.IsValid() && m.Type().NumIn() == 0 && m.Type().NumOut() == 1 { c := m.Call(nil) return toPluralCountValue(c[0].Interface()) @@ -169,7 +169,6 @@ func getPluralCount(v interface{}) interface{} { } return toPluralCountValue(v) - } // go-i18n expects floats to be represented by string. diff --git a/resources/page/pagegroup.go b/resources/page/pagegroup.go index e07efa7ca..4c0adb68b 100644 --- a/resources/page/pagegroup.go +++ b/resources/page/pagegroup.go @@ -24,6 +24,7 @@ import ( "github.com/spf13/cast" "github.com/gohugoio/hugo/common/collections" + "github.com/gohugoio/hugo/common/hreflect" "github.com/gohugoio/hugo/compare" "github.com/gohugoio/hugo/resources/resource" @@ -112,8 +113,9 @@ func (p Pages) GroupBy(key string, order ...string) (PagesGroup, error) { } var ft interface{} - m, ok := pagePtrType.MethodByName(key) - if ok { + index := hreflect.GetMethodIndexByName(pagePtrType, key) + if index != -1 { + m := pagePtrType.Method(index) 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") } @@ -125,6 +127,7 @@ func (p Pages) GroupBy(key string, order ...string) (PagesGroup, error) { } ft = m } else { + var ok bool ft, ok = pagePtrType.Elem().FieldByName(key) if !ok { 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: fv = ppv.Elem().FieldByName(key) case reflect.Method: - fv = ppv.MethodByName(key).Call([]reflect.Value{})[0] + fv = hreflect.GetMethodByName(ppv, key).Call([]reflect.Value{})[0] } if !fv.IsValid() { continue diff --git a/resources/postpub/postpub.go b/resources/postpub/postpub.go index c11dda577..f1012b7da 100644 --- a/resources/postpub/postpub.go +++ b/resources/postpub/postpub.go @@ -21,6 +21,7 @@ import ( "github.com/spf13/cast" + "github.com/gohugoio/hugo/common/hreflect" "github.com/gohugoio/hugo/common/maps" "github.com/gohugoio/hugo/media" "github.com/gohugoio/hugo/resources/resource" @@ -125,7 +126,7 @@ func (r *PostPublishResource) fieldToString(receiver interface{}, path string) s default: v := receiverv.FieldByName(fieldname) if !v.IsValid() { - method := receiverv.MethodByName(fieldname) + method := hreflect.GetMethodByName(receiverv, fieldname) if method.IsValid() { vals := method.Call(nil) if len(vals) > 0 { diff --git a/tpl/collections/apply.go b/tpl/collections/apply.go index 0833e5507..55bab5d1a 100644 --- a/tpl/collections/apply.go +++ b/tpl/collections/apply.go @@ -131,7 +131,7 @@ func (ns *Namespace) lookupFunc(fname string) (reflect.Value, bool) { nv = reflect.ValueOf(v) // method - m := nv.MethodByName(ss[1]) + m := hreflect.GetMethodByName(nv, ss[1]) if m.Kind() == reflect.Invalid { return reflect.Value{}, false diff --git a/tpl/collections/where.go b/tpl/collections/where.go index 8ffcf6165..ac604760d 100644 --- a/tpl/collections/where.go +++ b/tpl/collections/where.go @@ -19,6 +19,7 @@ import ( "reflect" "strings" + "github.com/gohugoio/hugo/common/hreflect" "github.com/gohugoio/hugo/common/maps" ) @@ -300,8 +301,9 @@ func evaluateSubElem(obj reflect.Value, elemName string) (reflect.Value, error) objPtr = objPtr.Addr() } - mt, ok := objPtr.Type().MethodByName(elemName) - if ok { + index := hreflect.GetMethodIndexByName(objPtr.Type(), elemName) + if index != -1 { + mt := objPtr.Type().Method(index) switch { case mt.PkgPath != "": 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 { 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() } diff --git a/tpl/internal/go_templates/texttemplate/hugo_template_test.go b/tpl/internal/go_templates/texttemplate/hugo_template_test.go index 150802bf4..cc88151e3 100644 --- a/tpl/internal/go_templates/texttemplate/hugo_template_test.go +++ b/tpl/internal/go_templates/texttemplate/hugo_template_test.go @@ -21,6 +21,7 @@ import ( "testing" qt "github.com/frankban/quicktest" + "github.com/gohugoio/hugo/common/hreflect" ) type TestStruct struct { @@ -59,7 +60,7 @@ func (e *execHelper) GetMethod(ctx context.Context, tmpl Preparer, receiver refl if name != "Hello1" { return zero, zero } - m := receiver.MethodByName("Hello2") + m := hreflect.GetMethodByName(receiver, "Hello2") return m, reflect.ValueOf("v2") } diff --git a/tpl/tplimpl/template_funcs.go b/tpl/tplimpl/template_funcs.go index 8692b9ee2..0ecd5df46 100644 --- a/tpl/tplimpl/template_funcs.go +++ b/tpl/tplimpl/template_funcs.go @@ -20,9 +20,9 @@ import ( "reflect" "strings" - "github.com/gohugoio/hugo/tpl" - + "github.com/gohugoio/hugo/common/hreflect" "github.com/gohugoio/hugo/common/maps" + "github.com/gohugoio/hugo/tpl" template "github.com/gohugoio/hugo/tpl/internal/go_templates/htmltemplate" 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) { 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 { case "GetPage", "Render": 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() { return zero, zero }