IsListType uses reflection and is expensive for hot paths

IsListType was causing ~100 allocations for a non list object. It is
used in a wide range of code, so perform a more targeted check.

The use of `%#v` in a hot return path for `fmt.Errorf()` was the main
victim.

Replace `%#v` with a typed error and create a cache of types that are
lists with a bounded size (probably not necessary, but safer).

```
BenchmarkGet-12          	  100000	    119635 ns/op	   20110 B/op	     206 allocs/op
BenchmarkWatchHTTP-12    	  100000	     65761 ns/op	    7296 B/op	     139 allocs/op

BenchmarkGet-12          	  100000	    109085 ns/op	   17831 B/op	     152 allocs/op
BenchmarkWatchHTTP-12    	  200000	     33966 ns/op	    1913 B/op	      30 allocs/op
```
This commit is contained in:
Clayton Coleman 2019-03-21 20:43:26 -04:00
parent 2e1506558a
commit 58fb665646
No known key found for this signature in database
GPG Key ID: 3D16906B4F1C5CB3
2 changed files with 61 additions and 10 deletions

View File

@ -17,30 +17,76 @@ limitations under the License.
package meta package meta
import ( import (
"errors"
"fmt" "fmt"
"reflect" "reflect"
"sync"
"k8s.io/apimachinery/pkg/conversion" "k8s.io/apimachinery/pkg/conversion"
"k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime"
) )
// IsListType returns true if the provided Object has a slice called Items var (
// isListCache maintains a cache of types that are checked for lists
// which is used by IsListType.
// TODO: remove and replace with an interface check
isListCache = struct {
lock sync.RWMutex
byType map[reflect.Type]bool
}{
byType: make(map[reflect.Type]bool, 1024),
}
)
// IsListType returns true if the provided Object has a slice called Items.
// TODO: Replace the code in this check with an interface comparison by
// creating and enforcing that lists implement a list accessor.
func IsListType(obj runtime.Object) bool { func IsListType(obj runtime.Object) bool {
// if we're a runtime.Unstructured, check whether this is a list. switch t := obj.(type) {
// TODO: refactor GetItemsPtr to use an interface that returns []runtime.Object case runtime.Unstructured:
if unstructured, ok := obj.(runtime.Unstructured); ok { return t.IsList()
return unstructured.IsList() }
t := reflect.TypeOf(obj)
isListCache.lock.RLock()
ok, exists := isListCache.byType[t]
isListCache.lock.RUnlock()
if !exists {
_, err := getItemsPtr(obj)
ok = err == nil
// cache only the first 1024 types
isListCache.lock.Lock()
if len(isListCache.byType) < 1024 {
isListCache.byType[t] = ok
}
isListCache.lock.Unlock()
} }
_, err := GetItemsPtr(obj) return ok
return err == nil
} }
var (
errExpectFieldItems = errors.New("no Items field in this object")
errExpectSliceItems = errors.New("Items field must be a slice of objects")
)
// GetItemsPtr returns a pointer to the list object's Items member. // GetItemsPtr returns a pointer to the list object's Items member.
// If 'list' doesn't have an Items member, it's not really a list type // If 'list' doesn't have an Items member, it's not really a list type
// and an error will be returned. // and an error will be returned.
// This function will either return a pointer to a slice, or an error, but not both. // This function will either return a pointer to a slice, or an error, but not both.
// TODO: this will be replaced with an interface in the future
func GetItemsPtr(list runtime.Object) (interface{}, error) { func GetItemsPtr(list runtime.Object) (interface{}, error) {
obj, err := getItemsPtr(list)
if err != nil {
return nil, fmt.Errorf("%T is not a list: %v", err)
}
return obj, nil
}
// getItemsPtr returns a pointer to the list object's Items member or an error.
func getItemsPtr(list runtime.Object) (interface{}, error) {
v, err := conversion.EnforcePtr(list) v, err := conversion.EnforcePtr(list)
if err != nil { if err != nil {
return nil, err return nil, err
@ -48,19 +94,19 @@ func GetItemsPtr(list runtime.Object) (interface{}, error) {
items := v.FieldByName("Items") items := v.FieldByName("Items")
if !items.IsValid() { if !items.IsValid() {
return nil, fmt.Errorf("no Items field in %#v", list) return nil, errExpectFieldItems
} }
switch items.Kind() { switch items.Kind() {
case reflect.Interface, reflect.Ptr: case reflect.Interface, reflect.Ptr:
target := reflect.TypeOf(items.Interface()).Elem() target := reflect.TypeOf(items.Interface()).Elem()
if target.Kind() != reflect.Slice { if target.Kind() != reflect.Slice {
return nil, fmt.Errorf("items: Expected slice, got %s", target.Kind()) return nil, errExpectSliceItems
} }
return items.Interface(), nil return items.Interface(), nil
case reflect.Slice: case reflect.Slice:
return items.Addr().Interface(), nil return items.Addr().Interface(), nil
default: default:
return nil, fmt.Errorf("items: Expected slice, got %s", items.Kind()) return nil, errExpectSliceItems
} }
} }

View File

@ -292,7 +292,12 @@ func checkName(obj runtime.Object, name, namespace string, namer ScopeNamer) err
} }
// setObjectSelfLink sets the self link of an object as needed. // setObjectSelfLink sets the self link of an object as needed.
// TODO: remove the need for the namer LinkSetters by requiring objects implement either Object or List
// interfaces
func setObjectSelfLink(ctx context.Context, obj runtime.Object, req *http.Request, namer ScopeNamer) error { func setObjectSelfLink(ctx context.Context, obj runtime.Object, req *http.Request, namer ScopeNamer) error {
// We only generate list links on objects that implement ListInterface - historically we duck typed this
// check via reflection, but as we move away from reflection we require that you not only carry Items but
// ListMeta into order to be identified as a list.
if !meta.IsListType(obj) { if !meta.IsListType(obj) {
requestInfo, ok := request.RequestInfoFrom(ctx) requestInfo, ok := request.RequestInfoFrom(ctx)
if !ok { if !ok {