Skip to content

Commit 58fb665

Browse files
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 ```
1 parent 2e15065 commit 58fb665

File tree

2 files changed

+61
-10
lines changed
  • staging/src/k8s.io
    • apimachinery/pkg/api/meta
    • apiserver/pkg/endpoints/handlers

2 files changed

+61
-10
lines changed

staging/src/k8s.io/apimachinery/pkg/api/meta/help.go

Lines changed: 56 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -17,50 +17,96 @@ limitations under the License.
1717
package meta
1818

1919
import (
20+
"errors"
2021
"fmt"
2122
"reflect"
23+
"sync"
2224

2325
"k8s.io/apimachinery/pkg/conversion"
2426
"k8s.io/apimachinery/pkg/runtime"
2527
)
2628

27-
// IsListType returns true if the provided Object has a slice called Items
29+
var (
30+
// isListCache maintains a cache of types that are checked for lists
31+
// which is used by IsListType.
32+
// TODO: remove and replace with an interface check
33+
isListCache = struct {
34+
lock sync.RWMutex
35+
byType map[reflect.Type]bool
36+
}{
37+
byType: make(map[reflect.Type]bool, 1024),
38+
}
39+
)
40+
41+
// IsListType returns true if the provided Object has a slice called Items.
42+
// TODO: Replace the code in this check with an interface comparison by
43+
// creating and enforcing that lists implement a list accessor.
2844
func IsListType(obj runtime.Object) bool {
29-
// if we're a runtime.Unstructured, check whether this is a list.
30-
// TODO: refactor GetItemsPtr to use an interface that returns []runtime.Object
31-
if unstructured, ok := obj.(runtime.Unstructured); ok {
32-
return unstructured.IsList()
45+
switch t := obj.(type) {
46+
case runtime.Unstructured:
47+
return t.IsList()
3348
}
49+
t := reflect.TypeOf(obj)
3450

35-
_, err := GetItemsPtr(obj)
36-
return err == nil
51+
isListCache.lock.RLock()
52+
ok, exists := isListCache.byType[t]
53+
isListCache.lock.RUnlock()
54+
55+
if !exists {
56+
_, err := getItemsPtr(obj)
57+
ok = err == nil
58+
59+
// cache only the first 1024 types
60+
isListCache.lock.Lock()
61+
if len(isListCache.byType) < 1024 {
62+
isListCache.byType[t] = ok
63+
}
64+
isListCache.lock.Unlock()
65+
}
66+
67+
return ok
3768
}
3869

70+
var (
71+
errExpectFieldItems = errors.New("no Items field in this object")
72+
errExpectSliceItems = errors.New("Items field must be a slice of objects")
73+
)
74+
3975
// GetItemsPtr returns a pointer to the list object's Items member.
4076
// If 'list' doesn't have an Items member, it's not really a list type
4177
// and an error will be returned.
4278
// This function will either return a pointer to a slice, or an error, but not both.
79+
// TODO: this will be replaced with an interface in the future
4380
func GetItemsPtr(list runtime.Object) (interface{}, error) {
81+
obj, err := getItemsPtr(list)
82+
if err != nil {
83+
return nil, fmt.Errorf("%T is not a list: %v", err)
84+
}
85+
return obj, nil
86+
}
87+
88+
// getItemsPtr returns a pointer to the list object's Items member or an error.
89+
func getItemsPtr(list runtime.Object) (interface{}, error) {
4490
v, err := conversion.EnforcePtr(list)
4591
if err != nil {
4692
return nil, err
4793
}
4894

4995
items := v.FieldByName("Items")
5096
if !items.IsValid() {
51-
return nil, fmt.Errorf("no Items field in %#v", list)
97+
return nil, errExpectFieldItems
5298
}
5399
switch items.Kind() {
54100
case reflect.Interface, reflect.Ptr:
55101
target := reflect.TypeOf(items.Interface()).Elem()
56102
if target.Kind() != reflect.Slice {
57-
return nil, fmt.Errorf("items: Expected slice, got %s", target.Kind())
103+
return nil, errExpectSliceItems
58104
}
59105
return items.Interface(), nil
60106
case reflect.Slice:
61107
return items.Addr().Interface(), nil
62108
default:
63-
return nil, fmt.Errorf("items: Expected slice, got %s", items.Kind())
109+
return nil, errExpectSliceItems
64110
}
65111
}
66112

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -292,7 +292,12 @@ func checkName(obj runtime.Object, name, namespace string, namer ScopeNamer) err
292292
}
293293

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

0 commit comments

Comments
 (0)