Merge pull request #36207 from smarterclayton/optimize_self_link

Automatic merge from submit-queue

SetSelfLink is inefficient

Generating self links, especially for lists, is inefficient.  Replace
use of net.URL.String() call with direct encoding that reduces number of
allocations. Switch from calling meta.ExtractList|SetList to a function
that iterates over each object in the list.

In steady state for nodes performing frequently small get/list
operations, and for larger LISTs significantly reduces CPU and
allocations.

@wojtek-t this is the next big chunk of CPU use during the large N nodes simulation test (11% of master CPU). Takes a few allocations out of the critical path

pkg/api/meta/help.go

@@ -66,6 +66,57 @@ func GetItemsPtr(list runtime.Object) (interface{}, error) {
 	}
 }
 
+// EachListItem invokes fn on each runtime.Object in the list. Any error immediately terminates
+// the loop.
+func EachListItem(obj runtime.Object, fn func(runtime.Object) error) error {
+	// TODO: Change to an interface call?
+	itemsPtr, err := GetItemsPtr(obj)
+	if err != nil {
+		return err
+	}
+	items, err := conversion.EnforcePtr(itemsPtr)
+	if err != nil {
+		return err
+	}
+	len := items.Len()
+	if len == 0 {
+		return nil
+	}
+	takeAddr := false
+	if elemType := items.Type().Elem(); elemType.Kind() != reflect.Ptr && elemType.Kind() != reflect.Interface {
+		if !items.Index(0).CanAddr() {
+			return fmt.Errorf("unable to take address of items in %T for EachListItem", obj)
+		}
+		takeAddr = true
+	}
+
+	for i := 0; i < len; i++ {
+		raw := items.Index(i)
+		if takeAddr {
+			raw = raw.Addr()
+		}
+		switch item := raw.Interface().(type) {
+		case *runtime.RawExtension:
+			if err := fn(item.Object); err != nil {
+				return err
+			}
+		case runtime.Object:
+			if err := fn(item); err != nil {
+				return err
+			}
+		default:
+			obj, ok := item.(runtime.Object)
+			if !ok {
+				return fmt.Errorf("%v: item[%v]: Expected object, got %#v(%s)", obj, i, raw.Interface(), raw.Kind())
+			}
+			if err := fn(obj); err != nil {
+				return err
+			}
+		}
+	}
+	return nil
+}
+
 // ExtractList returns obj's Items element as an array of runtime.Objects.
 // Returns an error if obj is not a List type (does not have an Items member).
 func ExtractList(obj runtime.Object) ([]runtime.Object, error) {

pkg/api/meta/help_test.go

@@ -46,94 +46,185 @@ func TestIsList(t *testing.T) {
 }
 
 func TestExtractList(t *testing.T) {
-	pl := &api.PodList{
-		Items: []api.Pod{
-			{ObjectMeta: api.ObjectMeta{Name: "1"}},
-			{ObjectMeta: api.ObjectMeta{Name: "2"}},
-			{ObjectMeta: api.ObjectMeta{Name: "3"}},
-		},
+	list1 := []runtime.Object{
+		&api.Pod{ObjectMeta: api.ObjectMeta{Name: "1"}},
+		&api.Service{ObjectMeta: api.ObjectMeta{Name: "2"}},
 	}
-	list, err := meta.ExtractList(pl)
-	if err != nil {
-		t.Fatalf("Unexpected error %v", err)
-	}
-	if e, a := len(list), len(pl.Items); e != a {
-		t.Fatalf("Expected %v, got %v", e, a)
-	}
-	for i := range list {
-		if e, a := list[i].(*api.Pod).Name, pl.Items[i].Name; e != a {
-			t.Fatalf("Expected %v, got %v", e, a)
-		}
-	}
-}
-
-func TestExtractListV1(t *testing.T) {
-	pl := &v1.PodList{
-		Items: []v1.Pod{
-			{ObjectMeta: v1.ObjectMeta{Name: "1"}},
-			{ObjectMeta: v1.ObjectMeta{Name: "2"}},
-			{ObjectMeta: v1.ObjectMeta{Name: "3"}},
-		},
-	}
-	list, err := meta.ExtractList(pl)
-	if err != nil {
-		t.Fatalf("Unexpected error %v", err)
-	}
-	if e, a := len(list), len(pl.Items); e != a {
-		t.Fatalf("Expected %v, got %v", e, a)
-	}
-	for i := range list {
-		if e, a := list[i].(*v1.Pod).Name, pl.Items[i].Name; e != a {
-			t.Fatalf("Expected %v, got %v", e, a)
-		}
-	}
-}
-
-func TestExtractListGeneric(t *testing.T) {
-	pl := &api.List{
-		Items: []runtime.Object{
-			&api.Pod{ObjectMeta: api.ObjectMeta{Name: "1"}},
-			&api.Service{ObjectMeta: api.ObjectMeta{Name: "2"}},
-		},
-	}
-	list, err := meta.ExtractList(pl)
-	if err != nil {
-		t.Fatalf("Unexpected error %v", err)
-	}
-	if e, a := len(list), len(pl.Items); e != a {
-		t.Fatalf("Expected %v, got %v", e, a)
-	}
-	if obj, ok := list[0].(*api.Pod); !ok {
-		t.Fatalf("Expected list[0] to be *api.Pod, it is %#v", obj)
-	}
-	if obj, ok := list[1].(*api.Service); !ok {
-		t.Fatalf("Expected list[1] to be *api.Service, it is %#v", obj)
-	}
-}
-
-func TestExtractListGenericV1(t *testing.T) {
-	pl := &v1.List{
+	list2 := &v1.List{
 		Items: []runtime.RawExtension{
 			{Raw: []byte("foo")},
 			{Raw: []byte("bar")},
 			{Object: &v1.Pod{ObjectMeta: v1.ObjectMeta{Name: "other"}}},
 		},
 	}
-	list, err := meta.ExtractList(pl)
-	if err != nil {
-		t.Fatalf("Unexpected error %v", err)
+	list3 := &fakePtrValueList{
+		Items: []*api.Pod{
+			{ObjectMeta: api.ObjectMeta{Name: "1"}},
+			{ObjectMeta: api.ObjectMeta{Name: "2"}},
+		},
 	}
-	if e, a := len(list), len(pl.Items); e != a {
-		t.Fatalf("Expected %v, got %v", e, a)
+	list4 := &api.PodList{
+		Items: []api.Pod{
+			{ObjectMeta: api.ObjectMeta{Name: "1"}},
+			{ObjectMeta: api.ObjectMeta{Name: "2"}},
+			{ObjectMeta: api.ObjectMeta{Name: "3"}},
+		},
 	}
-	if obj, ok := list[0].(*runtime.Unknown); !ok {
-		t.Fatalf("Expected list[0] to be *runtime.Unknown, it is %#v", obj)
+	list5 := &v1.PodList{
+		Items: []v1.Pod{
+			{ObjectMeta: v1.ObjectMeta{Name: "1"}},
+			{ObjectMeta: v1.ObjectMeta{Name: "2"}},
+			{ObjectMeta: v1.ObjectMeta{Name: "3"}},
+		},
 	}
-	if obj, ok := list[1].(*runtime.Unknown); !ok {
-		t.Fatalf("Expected list[1] to be *runtime.Unknown, it is %#v", obj)
+
+	testCases := []struct {
+		in    runtime.Object
+		out   []interface{}
+		equal bool
+	}{
+		{
+			in:  &api.List{},
+			out: []interface{}{},
+		},
+		{
+			in:  &v1.List{},
+			out: []interface{}{},
+		},
+		{
+			in:  &v1.PodList{},
+			out: []interface{}{},
+		},
+		{
+			in:  &api.List{Items: list1},
+			out: []interface{}{list1[0], list1[1]},
+		},
+		{
+			in:    list2,
+			out:   []interface{}{&runtime.Unknown{Raw: list2.Items[0].Raw}, &runtime.Unknown{Raw: list2.Items[1].Raw}, list2.Items[2].Object},
+			equal: true,
+		},
+		{
+			in:  list3,
+			out: []interface{}{list3.Items[0], list3.Items[1]},
+		},
+		{
+			in:  list4,
+			out: []interface{}{&list4.Items[0], &list4.Items[1], &list4.Items[2]},
+		},
+		{
+			in:  list5,
+			out: []interface{}{&list5.Items[0], &list5.Items[1], &list5.Items[2]},
+		},
 	}
-	if obj, ok := list[2].(*v1.Pod); !ok {
-		t.Fatalf("Expected list[2] to be *runtime.Unknown, it is %#v", obj)
+	for i, test := range testCases {
+		list, err := meta.ExtractList(test.in)
+		if err != nil {
+			t.Fatalf("%d: extract: Unexpected error %v", i, err)
+		}
+		if e, a := len(test.out), len(list); e != a {
+			t.Fatalf("%d: extract: Expected %v, got %v", i, e, a)
+		}
+		for j, e := range test.out {
+			if e != list[j] {
+				if !test.equal {
+					t.Fatalf("%d: extract: Expected list[%d] to be %#v, but found %#v", i, j, e, list[j])
+				}
+				if !reflect.DeepEqual(e, list[j]) {
+					t.Fatalf("%d: extract: Expected list[%d] to be %#v, but found %#v", i, j, e, list[j])
+				}
+			}
+		}
+	}
+}
+
+func TestEachListItem(t *testing.T) {
+	list1 := []runtime.Object{
+		&api.Pod{ObjectMeta: api.ObjectMeta{Name: "1"}},
+		&api.Service{ObjectMeta: api.ObjectMeta{Name: "2"}},
+	}
+	list2 := &v1.List{
+		Items: []runtime.RawExtension{
+			{Raw: []byte("foo")},
+			{Raw: []byte("bar")},
+			{Object: &v1.Pod{ObjectMeta: v1.ObjectMeta{Name: "other"}}},
+		},
+	}
+	list3 := &fakePtrValueList{
+		Items: []*api.Pod{
+			{ObjectMeta: api.ObjectMeta{Name: "1"}},
+			{ObjectMeta: api.ObjectMeta{Name: "2"}},
+		},
+	}
+	list4 := &api.PodList{
+		Items: []api.Pod{
+			{ObjectMeta: api.ObjectMeta{Name: "1"}},
+			{ObjectMeta: api.ObjectMeta{Name: "2"}},
+			{ObjectMeta: api.ObjectMeta{Name: "3"}},
+		},
+	}
+	list5 := &v1.PodList{
+		Items: []v1.Pod{
+			{ObjectMeta: v1.ObjectMeta{Name: "1"}},
+			{ObjectMeta: v1.ObjectMeta{Name: "2"}},
+			{ObjectMeta: v1.ObjectMeta{Name: "3"}},
+		},
+	}
+
+	testCases := []struct {
+		in  runtime.Object
+		out []interface{}
+	}{
+		{
+			in:  &api.List{},
+			out: []interface{}{},
+		},
+		{
+			in:  &v1.List{},
+			out: []interface{}{},
+		},
+		{
+			in:  &v1.PodList{},
+			out: []interface{}{},
+		},
+		{
+			in:  &api.List{Items: list1},
+			out: []interface{}{list1[0], list1[1]},
+		},
+		{
+			in:  list2,
+			out: []interface{}{nil, nil, list2.Items[2].Object},
+		},
+		{
+			in:  list3,
+			out: []interface{}{list3.Items[0], list3.Items[1]},
+		},
+		{
+			in:  list4,
+			out: []interface{}{&list4.Items[0], &list4.Items[1], &list4.Items[2]},
+		},
+		{
+			in:  list5,
+			out: []interface{}{&list5.Items[0], &list5.Items[1], &list5.Items[2]},
+		},
+	}
+	for i, test := range testCases {
+		list := []runtime.Object{}
+		err := meta.EachListItem(test.in, func(obj runtime.Object) error {
+			list = append(list, obj)
+			return nil
+		})
+		if err != nil {
+			t.Fatalf("%d: each: Unexpected error %v", i, err)
+		}
+		if e, a := len(test.out), len(list); e != a {
+			t.Fatalf("%d: each: Expected %v, got %v", i, e, a)
+		}
+		for j, e := range test.out {
+			if e != list[j] {
+				t.Fatalf("%d: each: Expected list[%d] to be %#v, but found %#v", i, j, e, list[j])
+			}
+		}
 	}
 }
 
@@ -166,27 +257,6 @@ func (obj fakePtrValueList) GetObjectKind() unversioned.ObjectKind {
 	return unversioned.EmptyObjectKind
 }
 
-func TestExtractListOfValuePtrs(t *testing.T) {
-	pl := &fakePtrValueList{
-		Items: []*api.Pod{
-			{ObjectMeta: api.ObjectMeta{Name: "1"}},
-			{ObjectMeta: api.ObjectMeta{Name: "2"}},
-		},
-	}
-	list, err := meta.ExtractList(pl)
-	if err != nil {
-		t.Fatalf("Unexpected error %v", err)
-	}
-	if e, a := len(list), len(pl.Items); e != a {
-		t.Fatalf("Expected %v, got %v", e, a)
-	}
-	for i := range list {
-		if obj, ok := list[i].(*api.Pod); !ok {
-			t.Fatalf("Expected list[%d] to be *api.Pod, it is %#v", i, obj)
-		}
-	}
-}
-
 func TestSetList(t *testing.T) {
 	pl := &api.PodList{}
 	list := []runtime.Object{

pkg/apiserver/api_installer.go

@@ -17,8 +17,10 @@ limitations under the License.
 package apiserver
 
 import (
+	"bytes"
 	"fmt"
 	"net/http"
+	"net/url"
 	gpath "path"
 	"reflect"
 	"sort"
@@ -357,15 +359,17 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag
 		itemPath := resourcePath + "/{name}"
 		nameParams := append(params, nameParam)
 		proxyParams := append(nameParams, pathParam)
+		suffix := ""
 		if hasSubresource {
-			itemPath = itemPath + "/" + subresource
+			suffix = "/" + subresource
+			itemPath = itemPath + suffix
 			resourcePath = itemPath
 			resourceParams = nameParams
 		}
 		apiResource.Name = path
 		apiResource.Namespaced = false
 		apiResource.Kind = resourceKind
-		namer := rootScopeNaming{scope, a.group.Linker, gpath.Join(a.prefix, itemPath)}
+		namer := rootScopeNaming{scope, a.group.Linker, gpath.Join(a.prefix, resourcePath, "/"), suffix}
 
 		// Handler for standard REST verbs (GET, PUT, POST and DELETE).
 		// Add actions at the resource path: /api/apiVersion/resource
@@ -417,8 +421,14 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag
 		apiResource.Namespaced = true
 		apiResource.Kind = resourceKind
 
-		itemPathFn := func(name, namespace string) string {
-			return itemPathPrefix + namespace + itemPathMiddle + name + itemPathSuffix
+		itemPathFn := func(name, namespace string) bytes.Buffer {
+			var buf bytes.Buffer
+			buf.WriteString(itemPathPrefix)
+			buf.WriteString(url.QueryEscape(namespace))
+			buf.WriteString(itemPathMiddle)
+			buf.WriteString(url.QueryEscape(name))
+			buf.WriteString(itemPathSuffix)
+			return buf
 		}
 		namer := scopeNaming{scope, a.group.Linker, itemPathFn, false}
 
@@ -751,7 +761,8 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag
 type rootScopeNaming struct {
 	scope meta.RESTScope
 	runtime.SelfLinker
-	itemPath string
+	pathPrefix string
+	pathSuffix string
 }
 
 // rootScopeNaming implements ScopeNamer
@@ -773,25 +784,26 @@ func (n rootScopeNaming) Name(req *restful.Request) (namespace, name string, err
 }
 
 // GenerateLink returns the appropriate path and query to locate an object by its canonical path.
-func (n rootScopeNaming) GenerateLink(req *restful.Request, obj runtime.Object) (path, query string, err error) {
+func (n rootScopeNaming) GenerateLink(req *restful.Request, obj runtime.Object) (uri string, err error) {
 	_, name, err := n.ObjectName(obj)
 	if err != nil {
-		return "", "", err
+		return "", err
 	}
 	if len(name) == 0 {
 		_, name, err = n.Name(req)
 		if err != nil {
-			return "", "", err
+			return "", err
 		}
 	}
-	path = strings.Replace(n.itemPath, "{name}", name, 1)
-	return path, "", nil
+	return n.pathPrefix + url.QueryEscape(name) + n.pathSuffix, nil
 }
 
 // GenerateListLink returns the appropriate path and query to locate a list by its canonical path.
-func (n rootScopeNaming) GenerateListLink(req *restful.Request) (path, query string, err error) {
-	path = req.Request.URL.Path
-	return path, "", nil
+func (n rootScopeNaming) GenerateListLink(req *restful.Request) (uri string, err error) {
+	if len(req.Request.URL.RawPath) > 0 {
+		return req.Request.URL.RawPath, nil
+	}
+	return req.Request.URL.EscapedPath(), nil
 }
 
 // ObjectName returns the name set on the object, or an error if the
@@ -813,7 +825,7 @@ func (n rootScopeNaming) ObjectName(obj runtime.Object) (namespace, name string,
 type scopeNaming struct {
 	scope meta.RESTScope
 	runtime.SelfLinker
-	itemPathFn    func(name, namespace string) string
+	itemPathFn    func(name, namespace string) bytes.Buffer
 	allNamespaces bool
 }
 
@@ -846,28 +858,30 @@ func (n scopeNaming) Name(req *restful.Request) (namespace, name string, err err
 }
 
 // GenerateLink returns the appropriate path and query to locate an object by its canonical path.
-func (n scopeNaming) GenerateLink(req *restful.Request, obj runtime.Object) (path, query string, err error) {
+func (n scopeNaming) GenerateLink(req *restful.Request, obj runtime.Object) (uri string, err error) {
 	namespace, name, err := n.ObjectName(obj)
 	if err != nil {
-		return "", "", err
+		return "", err
 	}
 	if len(namespace) == 0 && len(name) == 0 {
 		namespace, name, err = n.Name(req)
 		if err != nil {
-			return "", "", err
+			return "", err
 		}
 	}
 	if len(name) == 0 {
-		return "", "", errEmptyName
+		return "", errEmptyName
 	}
-
-	return n.itemPathFn(name, namespace), "", nil
+	result := n.itemPathFn(name, namespace)
+	return result.String(), nil
 }
 
 // GenerateListLink returns the appropriate path and query to locate a list by its canonical path.
-func (n scopeNaming) GenerateListLink(req *restful.Request) (path, query string, err error) {
-	path = req.Request.URL.Path
-	return path, "", nil
+func (n scopeNaming) GenerateListLink(req *restful.Request) (uri string, err error) {
+	if len(req.Request.URL.RawPath) > 0 {
+		return req.Request.URL.RawPath, nil
+	}
+	return req.Request.URL.EscapedPath(), nil
 }
 
 // ObjectName returns the name and namespace set on the object, or an error if the

pkg/apiserver/api_installer_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package apiserver
 
 import (
+	"bytes"
 	"testing"
 
 	"k8s.io/kubernetes/pkg/api"
@@ -36,8 +37,8 @@ func TestScopeNamingGenerateLink(t *testing.T) {
 	s := scopeNaming{
 		meta.RESTScopeNamespace,
 		selfLinker,
-		func(name, namespace string) string {
-			return "/api/v1/namespaces/" + namespace + "/services/" + name
+		func(name, namespace string) bytes.Buffer {
+			return *bytes.NewBufferString("/api/v1/namespaces/" + namespace + "/services/" + name)
 		},
 		true,
 	}
@@ -50,7 +51,7 @@ func TestScopeNamingGenerateLink(t *testing.T) {
 			Kind: "Service",
 		},
 	}
-	_, _, err := s.GenerateLink(&restful.Request{}, service)
+	_, err := s.GenerateLink(&restful.Request{}, service)
 	if err != nil {
 		t.Errorf("Unexpected error %v", err)
 	}

pkg/apiserver/resthandler.go

@@ -60,10 +60,11 @@ type ScopeNamer interface {
 	// SetSelfLink sets the provided URL onto the object. The method should return nil if the object
 	// does not support selfLinks.
 	SetSelfLink(obj runtime.Object, url string) error
-	// GenerateLink creates a path and query for a given runtime object that represents the canonical path.
-	GenerateLink(req *restful.Request, obj runtime.Object) (path, query string, err error)
-	// GenerateLink creates a path and query for a list that represents the canonical path.
-	GenerateListLink(req *restful.Request) (path, query string, err error)
+	// GenerateLink creates an encoded URI for a given runtime object that represents the canonical path
+	// and query.
+	GenerateLink(req *restful.Request, obj runtime.Object) (uri string, err error)
+	// GenerateLink creates an encoded URI for a list that represents the canonical path and query.
+	GenerateListLink(req *restful.Request) (uri string, err error)
 }
 
 // RequestScope encapsulates common fields across all RESTful handler methods.
@@ -997,18 +998,12 @@ func transformDecodeError(typer runtime.ObjectTyper, baseErr error, into runtime
 // plus the path and query generated by the provided linkFunc
 func setSelfLink(obj runtime.Object, req *restful.Request, namer ScopeNamer) error {
 	// TODO: SelfLink generation should return a full URL?
-	path, query, err := namer.GenerateLink(req, obj)
+	uri, err := namer.GenerateLink(req, obj)
 	if err != nil {
 		return nil
 	}
 
-	newURL := *req.Request.URL
-	// use only canonical paths
-	newURL.Path = path
-	newURL.RawQuery = query
-	newURL.Fragment = ""
-
-	return namer.SetSelfLink(obj, newURL.String())
+	return namer.SetSelfLink(obj, uri)
 }
 
 func hasUID(obj runtime.Object) (bool, error) {
@@ -1052,31 +1047,20 @@ func setListSelfLink(obj runtime.Object, req *restful.Request, namer ScopeNamer)
 		return 0, nil
 	}
 
-	// TODO: List SelfLink generation should return a full URL?
-	path, query, err := namer.GenerateListLink(req)
+	uri, err := namer.GenerateListLink(req)
 	if err != nil {
 		return 0, err
 	}
-	newURL := *req.Request.URL
-	newURL.Path = path
-	newURL.RawQuery = query
-	// use the path that got us here
-	newURL.Fragment = ""
-	if err := namer.SetSelfLink(obj, newURL.String()); err != nil {
+	if err := namer.SetSelfLink(obj, uri); err != nil {
 		glog.V(4).Infof("Unable to set self link on object: %v", err)
 	}
 
-	// Set self-link of objects in the list.
-	items, err := meta.ExtractList(obj)
-	if err != nil {
-		return 0, err
-	}
-	for i := range items {
-		if err := setSelfLink(items[i], req, namer); err != nil {
-			return len(items), err
-		}
-	}
-	return len(items), meta.SetList(obj, items)
+	count := 0
+	err = meta.EachListItem(obj, func(obj runtime.Object) error {
+		count++
+		return setSelfLink(obj, req, namer)
+	})
+	return count, err
 }
 
 func getPatchedJS(patchType api.PatchType, originalJS, patchJS []byte, obj runtime.Object) ([]byte, error) {

pkg/apiserver/resthandler_test.go

@@ -134,13 +134,13 @@ func (p *testNamer) SetSelfLink(obj runtime.Object, url string) error {
 }
 
 // GenerateLink creates a path and query for a given runtime object that represents the canonical path.
-func (p *testNamer) GenerateLink(req *restful.Request, obj runtime.Object) (path, query string, err error) {
-	return "", "", errors.New("not implemented")
+func (p *testNamer) GenerateLink(req *restful.Request, obj runtime.Object) (uri string, err error) {
+	return "", errors.New("not implemented")
 }
 
 // GenerateLink creates a path and query for a list that represents the canonical path.
-func (p *testNamer) GenerateListLink(req *restful.Request) (path, query string, err error) {
-	return "", "", errors.New("not implemented")
+func (p *testNamer) GenerateListLink(req *restful.Request) (uri string, err error) {
+	return "", errors.New("not implemented")
 }
 
 type patchTestCase struct {