Skip to content

Commit

Permalink
eskip: separate parsing of predicates and filters (#2873)
Browse files Browse the repository at this point in the history
Introduce start tokens to allow separate parsing of
eskip document, predicates and filters, see
https://www.gnu.org/software/bison/manual/html_node/Multiple-start_002dsymbols.html

Also remove `stringval` and `regexpval` rules.
These rules are aliases for `stringliteral` which can be used directly.
This reduces parser stack depth and item size.

The change fixes parse error position value and speeds up predicates/filters parsing.

```
goos: linux
goarch: amd64
pkg: github.com/zalando/skipper/eskip
cpu: Intel(R) Core(TM) i5-8350U CPU @ 1.70GHz
                  │   HEAD~1    │                HEAD                 │
                  │   sec/op    │   sec/op     vs base                │
ParsePredicates-8   8.531µ ± 2%   7.464µ ± 5%  -12.51% (p=0.000 n=10)
Parse-8             257.3m ± 1%   251.4m ± 2%   -2.32% (p=0.000 n=10)
geomean             1.482m        1.370m        -7.55%

                  │    HEAD~1    │                 HEAD                 │
                  │     B/op     │     B/op      vs base                │
ParsePredicates-8   2.375Ki ± 0%   1.773Ki ± 0%  -25.33% (p=0.000 n=10)
Parse-8             61.79Mi ± 0%   61.79Mi ± 0%        ~ (p=0.698 n=10)
geomean             387.7Ki        335.0Ki       -13.59%

                  │   HEAD~1    │                HEAD                │
                  │  allocs/op  │  allocs/op   vs base               │
ParsePredicates-8    53.00 ± 0%    50.00 ± 0%  -5.66% (p=0.000 n=10)
Parse-8             1.510M ± 0%   1.510M ± 0%       ~ (p=0.700 n=10)
geomean             8.946k        8.689k       -2.87%

        │    HEAD~1    │               HEAD               │
        │   bytes/op   │   bytes/op    vs base            │
Parse-8   15.99Mi ± 0%   15.99Mi ± 0%  ~ (p=1.000 n=10) ¹
¹ all samples are equal
```

Signed-off-by: Alexander Yastrebov <[email protected]>
  • Loading branch information
AlexanderYastrebov authored Feb 9, 2024
1 parent cbc4e2d commit f35824c
Show file tree
Hide file tree
Showing 10 changed files with 256 additions and 150 deletions.
10 changes: 5 additions & 5 deletions cmd/webhook/admission/admission_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ func TestRouteGroupAdmitter(t *testing.T) {
{
name: "invalid eskip filters",
inputFile: "rg-with-invalid-eskip-filters.json",
message: "parse failed after token status, position 11: syntax error",
message: "parse failed after token status, position 6: syntax error",
},
{
name: "valid eskip predicates",
Expand All @@ -121,7 +121,7 @@ func TestRouteGroupAdmitter(t *testing.T) {
{
name: "invalid eskip filters and predicates",
inputFile: "rg-with-invalid-eskip-filters-and-predicates.json",
message: "parse failed after token status, position 11: syntax error\\nparse failed after token Method, position 6: syntax error",
message: "parse failed after token status, position 6: syntax error\\nparse failed after token Method, position 6: syntax error",
},
{
name: "invalid routgroup multiple filters per json/yaml array item",
Expand Down Expand Up @@ -190,12 +190,12 @@ func TestIngressAdmitter(t *testing.T) {
{
name: "invalid eskip filters",
inputFile: "invalid-filters.json",
message: `invalid \"zalando.org/skipper-filter\" annotation: parse failed after token this, position 9: syntax error`,
message: `invalid \"zalando.org/skipper-filter\" annotation: parse failed after token this, position 4: syntax error`,
},
{
name: "invalid eskip predicates",
inputFile: "invalid-predicates.json",
message: `invalid \"zalando.org/skipper-predicate\" annotation: parse failed after token ), position 15: syntax error`,
message: `invalid \"zalando.org/skipper-predicate\" annotation: parse failed after token ), position 15: unexpected token`,
},
{
name: "invalid eskip routes",
Expand All @@ -205,7 +205,7 @@ func TestIngressAdmitter(t *testing.T) {
{
name: "invalid eskip filters and predicates",
inputFile: "invalid-filters-and-predicates.json",
message: `invalid \"zalando.org/skipper-filter\" annotation: parse failed after token this, position 9: syntax error\ninvalid \"zalando.org/skipper-predicate\" annotation: parse failed after token ), position 15: syntax error`,
message: `invalid \"zalando.org/skipper-filter\" annotation: parse failed after token this, position 4: syntax error\ninvalid \"zalando.org/skipper-predicate\" annotation: parse failed after token ), position 15: unexpected token`,
},
} {
t.Run(tc.name, func(t *testing.T) {
Expand Down
2 changes: 1 addition & 1 deletion config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ func Test_Validate(t *testing.T) {
c.KubernetesEastWestRangePredicatesString = "WrongEastWestMode"
},
wantErr: true,
want: errors.New("invalid east-west-range-predicates: parse failed after token ->, position 20: syntax error"),
want: errors.New("invalid east-west-range-predicates: parse failed after token WrongEastWestMode, position 17: syntax error"),
},
{
name: "test wrong HistoGramBuckets",
Expand Down
Original file line number Diff line number Diff line change
@@ -1 +1 @@
\[routegroup\] parse failed after token foo, position 8: syntax error
\[routegroup\] parse failed after token foo, position 3: syntax error
Original file line number Diff line number Diff line change
@@ -1 +1 @@
\[routegroup\] parse failed after token foo, position 8: syntax error
\[routegroup\] parse failed after token foo, position 3: syntax error
43 changes: 27 additions & 16 deletions eskip/eskip.go
Original file line number Diff line number Diff line change
Expand Up @@ -588,8 +588,22 @@ type eskipLexParser struct {

var parserPool sync.Pool

// executes the parser.
func parse(code string) ([]*parsedRoute, error) {
func parseDocument(code string) ([]*parsedRoute, error) {
routes, _, _, err := parse(start_document, code)
return routes, err
}

func parsePredicates(code string) ([]*Predicate, error) {
_, predicates, _, err := parse(start_predicates, code)
return predicates, err
}

func parseFilters(code string) ([]*Filter, error) {
_, _, filters, err := parse(start_filters, code)
return filters, err
}

func parse(start int, code string) ([]*parsedRoute, []*Predicate, []*Filter, error) {
lp, ok := parserPool.Get().(*eskipLexParser)
if !ok {
lp = &eskipLexParser{}
Expand All @@ -598,16 +612,19 @@ func parse(code string) ([]*parsedRoute, error) {
}
defer parserPool.Put(lp)

lp.lexer.init(code)
lexer := &lp.lexer
lexer.init(start, code)

lp.parser.Parse(&lp.lexer)
lp.parser.Parse(lexer)

return lp.lexer.routes, lp.lexer.err
// Do not return lexer to avoid reading lexer fields after returning eskipLexParser to the pool.
// Let the caller decide which of return values to use based on the start token.
return lexer.routes, lexer.predicates, lexer.filters, lexer.err
}

// Parses a route expression or a routing document to a set of route definitions.
func Parse(code string) ([]*Route, error) {
parsedRoutes, err := parse(code)
parsedRoutes, err := parseDocument(code)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -662,12 +679,7 @@ func ParseFilters(f string) ([]*Filter, error) {
return nil, nil
}

rs, err := parse("* -> " + f + " -> <shunt>")
if err != nil {
return nil, err
}

return rs[0].filters, nil
return parseFilters(f)
}

// ParsePredicates parses a set of predicates (combined by '&&') into
Expand All @@ -678,7 +690,7 @@ func ParsePredicates(p string) ([]*Predicate, error) {
return nil, nil
}

rs, err := parse(p + " -> <shunt>")
rs, err := parsePredicates(p)
if err != nil {
return nil, err
}
Expand All @@ -687,9 +699,8 @@ func ParsePredicates(p string) ([]*Predicate, error) {
return nil, nil
}

r := rs[0]
ps := make([]*Predicate, 0, len(r.predicates))
for _, p := range r.predicates {
ps := make([]*Predicate, 0, len(rs))
for _, p := range rs {
if p.Name != "*" {
ps = append(ps, p)
}
Expand Down
56 changes: 52 additions & 4 deletions eskip/eskip_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -239,19 +239,39 @@ func TestParseFilters(t *testing.T) {
"error",
"trallala",
nil,
// TODO: fix position
"parse failed after token ->, position 16: syntax error",
"parse failed after token trallala, position 8: syntax error",
}, {
"error 2",
"foo-bar",
nil,
// TODO: fix position
"parse failed after token foo, position 8: syntax error",
"parse failed after token foo, position 3: syntax error",
}, {
"success",
`filter1(3.14) -> filter2("key", 42)`,
[]*Filter{{Name: "filter1", Args: []interface{}{3.14}}, {Name: "filter2", Args: []interface{}{"key", float64(42)}}},
"",
}, {
"a comment produces nil filters without error",
"// a comment",
nil,
"",
}, {
"a trailing comment is ignored",
`filter1(3.14) -> filter2("key", 42) // a trailing comment`,
[]*Filter{{Name: "filter1", Args: []interface{}{3.14}}, {Name: "filter2", Args: []interface{}{"key", float64(42)}}},
"",
}, {
"a comment before is ignored",
`// a comment on a separate line
filter1(3.14) -> filter2("key", 42)`,
[]*Filter{{Name: "filter1", Args: []interface{}{3.14}}, {Name: "filter2", Args: []interface{}{"key", float64(42)}}},
"",
}, {
"a comment after is ignored",
`filter1(3.14) -> filter2("key", 42)
// a comment on a separate line`,
[]*Filter{{Name: "filter1", Args: []interface{}{3.14}}, {Name: "filter2", Args: []interface{}{"key", float64(42)}}},
"",
}} {
t.Run(ti.msg, func(t *testing.T) {
fs, err := ParseFilters(ti.expression)
Expand Down Expand Up @@ -306,6 +326,34 @@ func TestParsePredicates(t *testing.T) {
title: "comment fuzz 2", // "\x2f\x2f..." == "//..."
input: "\x2f\x2f\x00\x00\x00\xe6\xfe\x00\x00\x2f\x00\x00\x00\x00\x00\x00\x00\xe6\xfe\x00\x00\x2f\x00\x00\x00\x00",
expected: nil,
}, {
"a comment produces nil predicates without error",
"// a comment",
nil,
"",
}, {
title: "a trailing comment is ignored",
input: `Foo("bar") && Baz("qux") // a trailing comment`,
expected: []*Predicate{
{Name: "Foo", Args: []interface{}{"bar"}},
{Name: "Baz", Args: []interface{}{"qux"}},
},
}, {
title: "a comment before is ignored",
input: `// a comment on a separate line
Foo("bar") && Baz("qux")`,
expected: []*Predicate{
{Name: "Foo", Args: []interface{}{"bar"}},
{Name: "Baz", Args: []interface{}{"qux"}},
},
}, {
title: "a comment after is ignored",
input: `Foo("bar") && Baz("qux")
// a comment on a separate line`,
expected: []*Predicate{
{Name: "Foo", Args: []interface{}{"bar"}},
{Name: "Baz", Args: []interface{}{"qux"}},
},
}} {
t.Run(test.title, func(t *testing.T) {
ps, err := ParsePredicates(test.input)
Expand Down
13 changes: 12 additions & 1 deletion eskip/lexer.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,15 @@ type token struct {
type charPredicate func(byte) bool

type eskipLex struct {
start int
code string
lastToken string
lastRouteID string
err error
initialLength int
routes []*parsedRoute
predicates []*Predicate
filters []*Filter
}

type fixedScanner token
Expand Down Expand Up @@ -62,7 +65,8 @@ func (fs *fixedScanner) scan(code string) (t token, rest string, err error) {
return token(*fs), code[len(fs.val):], nil
}

func (l *eskipLex) init(code string) {
func (l *eskipLex) init(start int, code string) {
l.start = start
l.code = code
l.initialLength = len(code)
}
Expand Down Expand Up @@ -354,6 +358,13 @@ func (l *eskipLex) next() (token, error) {
}

func (l *eskipLex) Lex(lval *eskipSymType) int {
// first emit the start token
if l.start != 0 {
start := l.start
l.start = 0
return start
}

t, err := l.next()
if err == eof {
return -1
Expand Down
Loading

0 comments on commit f35824c

Please sign in to comment.