eskip: sort built-in predicates

PRs #2202 and #2208 added SortPredicates flag to PrettyPrintInfo and -sort-predicates flag to eskip tool which enables sorting of built-in and regular predicates. Header and HeaderRegexp built-in predicates are represented as maps internally therefore sorting is necessary to obtain stable eskip serialization of a route. This was planned to be used in routesrv which serves routes in eskip format and supports HTTP caching via ETag. Regular predicates are evaluated in ordered and short-circuit manner which makes it possible to put "cheap" predicates before "expensive". Sorting regular predicates therefore breaks short-circuit evaluation. With the above in mind it makes sense to sort built-in Header and HeaderRegexp predicates unconditionally and leave regular predicates intact. This change: * sorts built-in Header and HeaderRegexp predicates unconditionally in-place using string representation to reduce allocations * does not sort regular predicates * removes SortPredicates flag from PrettyPrintInfo * removes -sort-predicates from eskip Benchmark results to demonstrate the cost of sorting: ``` goos: linux goarch: amd64 pkg: github.com/zalando/skipper/eskip cpu: Intel(R) Core(TM) i5-8350U CPU @ 1.70GHz │ /tmp/BenchmarkRouteString.old │ /tmp/BenchmarkRouteString.new │ │ sec/op │ sec/op vs base │ RouteString-8 13.70µ ± 2% 14.07µ ± 2% +2.74% (p=0.009 n=10) RouteStringNoRepeatedPredicates-8 7.083µ ± 3% 7.102µ ± 1% ~ (p=0.079 n=10) geomean 9.850µ 9.997µ +1.50% │ /tmp/BenchmarkRouteString.old │ /tmp/BenchmarkRouteString.new │ │ B/op │ B/op vs base │ RouteString-8 2.430Ki ± 0% 2.477Ki ± 0% +1.93% (p=0.000 n=10) RouteStringNoRepeatedPredicates-8 1.211Ki ± 0% 1.211Ki ± 0% ~ (p=1.000 n=10) ¹ geomean 1.715Ki 1.732Ki +0.96% ¹ all samples are equal │ /tmp/BenchmarkRouteString.old │ /tmp/BenchmarkRouteString.new │ │ allocs/op │ allocs/op vs base │ RouteString-8 94.00 ± 0% 96.00 ± 0% +2.13% (p=0.000 n=10) RouteStringNoRepeatedPredicates-8 53.00 ± 0% 53.00 ± 0% ~ (p=1.000 n=10) ¹ geomean 70.58 71.33 +1.06% ¹ all samples are equal ``` Note that sorting only happens when there are several Header or HeaderRegexp predicates in the route. Extra allocations should go away in the future thanks to golang/go#61180 Followup on #2208 Updates #2519 Signed-off-by: Alexander Yastrebov <[email protected]>
zalando · Sep 5, 2023 · e8d6c31 · e8d6c31
1 parent bc6bc82
commit e8d6c31
Show file tree

Hide file tree

Showing 5 changed files with 39 additions and 76 deletions.
diff --git a/cmd/eskip/args.go b/cmd/eskip/args.go
@@ -39,7 +39,6 @@ const (
 	appendFiltersFlag  = "append"
 	appendFileFlag     = "append-file"
 	prettyFlag         = "pretty"
-	sortPredicatesFlag = "sort-predicates"
 	indentStrFlag      = "indent"
 	jsonFlag           = "json"
 
@@ -76,7 +75,6 @@ var (
 	appendFiltersArg  string
 	appendFileArg     string
 	pretty            bool
-	sortPredicates    bool
 	indentStr         string
 	printJson         bool
 )
@@ -112,7 +110,6 @@ func initFlags() {
 	flags.StringVar(&appendFileArg, appendFileFlag, "", appendFileUsage)
 
 	flags.BoolVar(&pretty, prettyFlag, false, prettyUsage)
-	flags.BoolVar(&sortPredicates, sortPredicatesFlag, false, sortPredicateUsage)
 	flags.StringVar(&indentStr, indentStrFlag, "  ", indentStrUsage)
 	flags.BoolVar(&printJson, jsonFlag, false, jsonUsage)
 }

diff --git a/cmd/eskip/doc.go b/cmd/eskip/doc.go
@@ -83,7 +83,6 @@ const (
 	appendFiltersUsage  = "append filters to each patched route"
 	appendFileUsage     = "append filters from a file to each patched route"
 	prettyUsage         = "prints routes in a more readable format"
-	sortPredicateUsage  = "sort routes predicates"
 	indentStrUsage      = "indent string used in pretty printing. Must match regexp \\s"
 	jsonUsage           = "prints routes as JSON"
 

diff --git a/cmd/eskip/load.go b/cmd/eskip/load.go
@@ -142,8 +142,7 @@ func printCmd(a cmdArgs) error {
 			}
 		}
 
-		eskip.Fprint(stdout, eskip.PrettyPrintInfo{Pretty: pretty, IndentStr: indentStr, SortPredicates: sortPredicates}, lr.routes...)
-
+		eskip.Fprint(stdout, eskip.PrettyPrintInfo{Pretty: pretty, IndentStr: indentStr}, lr.routes...)
 	}
 
 	if len(lr.parseErrors) > 0 {

diff --git a/eskip/string.go b/eskip/string.go
@@ -10,9 +10,8 @@ import (
 )
 
 type PrettyPrintInfo struct {
-	Pretty         bool
-	IndentStr      string
-	SortPredicates bool
+	Pretty    bool
+	IndentStr string
 }
 
 func escape(s string, chars string) string {
@@ -80,91 +79,46 @@ func argsString(args []interface{}) string {
 	return strings.Join(sargs, ", ")
 }
 
-func sortedKeys[V interface{}](m map[string]V) []string {
-	keys := make([]string, 0, len(m))
-	for k := range m {
-		keys = append(keys, k)
+func sortTail(s []string, from int) {
+	if len(s)-from > 1 {
+		sort.Strings(s[from:])
 	}
-	sort.Strings(keys)
-	return keys
 }
 
-func sortedCopy(values []string) []string {
-	toSortValues := make([]string, len(values))
-	copy(toSortValues, values)
-	sort.Strings(toSortValues)
-	return toSortValues
-}
-
-func (r *Route) predicateString(sortPredicates bool) string {
+func (r *Route) predicateString() string {
 	var predicates []string
 
 	if r.Path != "" {
 		predicates = appendFmtEscape(predicates, `Path("%s")`, `"`, r.Path)
 	}
 
-	hostRegexps := r.HostRegexps
-
-	if sortPredicates {
-		hostRegexps = sortedCopy(r.HostRegexps)
-	}
-
-	for _, h := range hostRegexps {
+	for _, h := range r.HostRegexps {
 		predicates = appendFmtEscape(predicates, "Host(/%s/)", "/", h)
 	}
 
-	pathRegexps := r.PathRegexps
-
-	if sortPredicates {
-		pathRegexps = sortedCopy(r.PathRegexps)
-	}
-
-	for _, p := range pathRegexps {
+	for _, p := range r.PathRegexps {
 		predicates = appendFmtEscape(predicates, "PathRegexp(/%s/)", "/", p)
 	}
 
 	if r.Method != "" {
 		predicates = appendFmtEscape(predicates, `Method("%s")`, `"`, r.Method)
 	}
 
-	if sortPredicates {
-		headerKeys := sortedKeys(r.Headers)
-		for _, key := range headerKeys {
-			predicates = appendFmtEscape(predicates, `Header("%s", "%s")`, `"`, key, r.Headers[key])
-		}
-
-	} else {
-		for k, v := range r.Headers {
-			predicates = appendFmtEscape(predicates, `Header("%s", "%s")`, `"`, k, v)
-		}
+	from := len(predicates)
+	for k, v := range r.Headers {
+		predicates = appendFmtEscape(predicates, `Header("%s", "%s")`, `"`, k, v)
 	}
+	sortTail(predicates, from)
 
-	if sortPredicates {
-		headerKeys := sortedKeys(r.HeaderRegexps)
-		for _, k := range headerKeys {
-			for _, rx := range r.HeaderRegexps[k] {
-				predicates = appendFmt(predicates, `HeaderRegexp("%s", /%s/)`, escape(k, `"`), escape(rx, "/"))
-			}
-		}
-	} else {
-		for k, rxs := range r.HeaderRegexps {
-			for _, rx := range rxs {
-				predicates = appendFmt(predicates, `HeaderRegexp("%s", /%s/)`, escape(k, `"`), escape(rx, "/"))
-			}
+	from = len(predicates)
+	for k, rxs := range r.HeaderRegexps {
+		for _, rx := range rxs {
+			predicates = appendFmt(predicates, `HeaderRegexp("%s", /%s/)`, escape(k, `"`), escape(rx, "/"))
 		}
 	}
+	sortTail(predicates, from)
 
-	routePredicates := r.Predicates
-
-	if sortPredicates {
-		routePredicates = make([]*Predicate, len(r.Predicates))
-		copy(routePredicates, r.Predicates)
-		sort.SliceStable(routePredicates, func(i, j int) bool {
-			return routePredicates[i].String() < routePredicates[j].String()
-		})
-	}
-
-	for _, p := range routePredicates {
+	for _, p := range r.Predicates {
 		if p.Name != "Any" {
 			predicates = appendFmt(predicates, "%s(%s)", p.Name, argsString(p.Args))
 		}
@@ -238,7 +192,7 @@ func (r *Route) String() string {
 }
 
 func (r *Route) Print(prettyPrintInfo PrettyPrintInfo) string {
-	s := []string{r.predicateString(prettyPrintInfo.SortPredicates)}
+	s := []string{r.predicateString()}
 
 	fs := r.filterString(prettyPrintInfo)
 	if fs != "" {

diff --git a/eskip/string_test.go b/eskip/string_test.go
@@ -180,35 +180,49 @@ func TestPrintNonPretty(t *testing.T) {
 
 func TestPrintSortedPredicates(t *testing.T) {
 	for i, item := range []struct {
+		name     string
 		route    string
 		expected string
 	}{
 		{
+			"preserves order of regular predicates",
 			`routeWithoutDefaultPredicates: True() && Cookie("alpha", "/^enabled$/") -> "https://www.example.org"`,
-			`Cookie("alpha", "/^enabled$/") && True() -> "https://www.example.org"`,
+			`True() && Cookie("alpha", "/^enabled$/") -> "https://www.example.org"`,
 		},
 		{
+			"puts builtin predicate before regular predicates",
 			`routeWithDefaultPredicates: True() && Cookie("alpha", "/^enabled$/") && Method("GET") -> "https://www.example.org"`,
-			`Method("GET") && Cookie("alpha", "/^enabled$/") && True() -> "https://www.example.org"`,
+			`Method("GET") && True() && Cookie("alpha", "/^enabled$/") -> "https://www.example.org"`,
 		},
 		{
+			"puts Method before Header",
 			`routeWithDefaultPredicatesOnly: Header("Accept", "application/json") && Method("GET") -> "https://www.example.org"`,
 			`Method("GET") && Header("Accept", "application/json") -> "https://www.example.org"`,
 		},
 		{
+			"sorts Header",
 			`routeWithMultipleHeaders: Header("x-frontend-type", "mobile-app") && Header("X-Forwarded-Proto", "http") -> "https://www.example.org"`,
 			`Header("X-Forwarded-Proto", "http") && Header("x-frontend-type", "mobile-app") -> "https://www.example.org"`,
 		},
 		{
+			"sorts HeaderRegexp",
 			`routeWithMultipleHeadersRegex: HeaderRegexp("User-Agent", /Zelt-(.*)/) && HeaderRegexp("age", /\\d/) -> "https://www.example.org"`,
 			`HeaderRegexp("User-Agent", /Zelt-(.*)/) && HeaderRegexp("age", /\\d/) -> "https://www.example.org"`,
 		},
 		{
+			"sorts HeaderRegexp with the same name",
+			`routeWithMultipleHeadersRegex: HeaderRegexp("B", /3/) && HeaderRegexp("B", /2/) && HeaderRegexp("A", /1/) -> "https://www.example.org"`,
+			`HeaderRegexp("A", /1/) && HeaderRegexp("B", /2/) && HeaderRegexp("B", /3/) -> "https://www.example.org"`,
+		},
+		{
+			"puts Method before Header, Header before HeaderRegexp and sorts",
 			`routeComplex: True() && Cookie("alpha", "/^enabled$/") && Method("GET") && Header("x-frontend-type", "mobile-app") && Header("X-Forwarded-Proto", "http") && HeaderRegexp("User-Agent", /Zelt-(.*)/) && HeaderRegexp("age", /\\d/) -> "https://www.example.org"`,
-			`Method("GET") && Header("X-Forwarded-Proto", "http") && Header("x-frontend-type", "mobile-app") && HeaderRegexp("User-Agent", /Zelt-(.*)/) && HeaderRegexp("age", /\\d/) && Cookie("alpha", "/^enabled$/") && True() -> "https://www.example.org"`,
+			`Method("GET") && Header("X-Forwarded-Proto", "http") && Header("x-frontend-type", "mobile-app") && HeaderRegexp("User-Agent", /Zelt-(.*)/) && HeaderRegexp("age", /\\d/) && True() && Cookie("alpha", "/^enabled$/") -> "https://www.example.org"`,
 		},
 	} {
-		testPrinting(item.route, item.expected, t, i, PrettyPrintInfo{Pretty: false, IndentStr: "", SortPredicates: true}, false)
+		t.Run(item.name, func(t *testing.T) {
+			testPrinting(item.route, item.expected, t, i, PrettyPrintInfo{Pretty: false, IndentStr: ""}, false)
+		})
 	}
 }
 
@@ -252,7 +266,7 @@ func TestPrintMultiRouteNonPretty(t *testing.T) {
 func testPrinting(routestr string, expected string, t *testing.T, i int, prettyPrintInfo PrettyPrintInfo, multi bool) {
 	routes, err := Parse(routestr)
 	if err != nil {
-		t.Error(err)
+		t.Fatal(err)
 	}
 	var printedRoute string