Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

eskip: sort built-in predicates #2576

Merged
merged 2 commits into from
Sep 6, 2023

Conversation

AlexanderYastrebov
Copy link
Member

@AlexanderYastrebov AlexanderYastrebov commented Sep 5, 2023

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]>
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]>
@szuecs
Copy link
Member

szuecs commented Sep 5, 2023

👍

1 similar comment
@MustafaSaber
Copy link
Member

👍

@AlexanderYastrebov AlexanderYastrebov merged commit 15513a5 into master Sep 6, 2023
6 checks passed
@AlexanderYastrebov AlexanderYastrebov deleted the eskip/refactor-sort-predicates branch September 6, 2023 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants