Skip to content

Commit

Permalink
eskip: sort built-in predicates
Browse files Browse the repository at this point in the history 
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.

Several built-in predicates 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 predicates
unconditionally and leave regular predicates intact.

This change:

* sorts built-in 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.67µ ± 1%   14.43µ ± 1%  +5.52% (p=0.000 n=10)
RouteStringNoRepeatedPredicates-8                     7.086µ ± 1%   7.092µ ± 1%       ~ (p=0.424 n=10)
geomean                                               9.843µ        10.11µ       +2.77%

                                  │ /tmp/BenchmarkRouteString.old │     /tmp/BenchmarkRouteString.new     │
                                  │             B/op              │     B/op      vs base                 │
RouteString-8                                        2.430Ki ± 0%   2.523Ki ± 0%  +3.86% (p=0.000 n=10)
RouteStringNoRepeatedPredicates-8                    1.211Ki ± 0%   1.211Ki ± 0%       ~ (p=1.000 n=10) ¹
geomean                                              1.715Ki        1.748Ki       +1.91%
¹ all samples are equal

                                  │ /tmp/BenchmarkRouteString.old │    /tmp/BenchmarkRouteString.new    │
                                  │           allocs/op           │ allocs/op   vs base                 │
RouteString-8                                          94.00 ± 0%   98.00 ± 0%  +4.26% (p=0.000 n=10)
RouteStringNoRepeatedPredicates-8                      53.00 ± 0%   53.00 ± 0%       ~ (p=1.000 n=10) ¹
geomean                                                70.58        72.07       +2.11%
¹ all samples are equal
```

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]>
  • Loading branch information
@AlexanderYastrebov
AlexanderYastrebov committed Sep 5, 2023
1 parent bc6bc82 commit 988636d
Show file tree
Hide file tree
Showing 5 changed files with 43 additions and 76 deletions.
3 changes: 0 additions & 3 deletions cmd/eskip/args.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ const (
appendFiltersFlag = "append"
appendFileFlag = "append-file"
prettyFlag = "pretty"
sortPredicatesFlag = "sort-predicates"
indentStrFlag = "indent"
jsonFlag = "json"

Expand Down Expand Up @@ -76,7 +75,6 @@ var (
appendFiltersArg string
appendFileArg string
pretty bool
sortPredicates bool
indentStr string
printJson bool
)
Expand Down Expand Up @@ -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)
}
Expand Down
1 change: 0 additions & 1 deletion cmd/eskip/doc.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down
3 changes: 1 addition & 2 deletions cmd/eskip/load.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
88 changes: 23 additions & 65 deletions eskip/string.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -80,91 +79,50 @@ 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 sortChunk(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 {
from := len(predicates)
for _, h := range r.HostRegexps {
predicates = appendFmtEscape(predicates, "Host(/%s/)", "/", h)
}
sortChunk(predicates, from)

pathRegexps := r.PathRegexps

if sortPredicates {
pathRegexps = sortedCopy(r.PathRegexps)
}

for _, p := range pathRegexps {
from = len(predicates)
for _, p := range r.PathRegexps {
predicates = appendFmtEscape(predicates, "PathRegexp(/%s/)", "/", p)
}
sortChunk(predicates, from)

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)
}
sortChunk(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, "/"))
}
}
sortChunk(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))
}
Expand Down Expand Up @@ -238,7 +196,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 != "" {
Expand Down
24 changes: 19 additions & 5 deletions eskip/string_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})
}
}

Expand Down Expand Up @@ -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

Expand Down

0 comments on commit 988636d

Please sign in to comment.