From 0339fbd5a3a2d972496b4fb3d94f1ffd5031760c Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Fri, 6 Jun 2025 15:02:58 +0100 Subject: [PATCH 1/4] [TESTS] PromQL: Add benchmark for PreprocessExpr Signed-off-by: Bryan Boreham --- promql/bench_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/promql/bench_test.go b/promql/bench_test.go index 695b2c1eb4..773ba2acfb 100644 --- a/promql/bench_test.go +++ b/promql/bench_test.go @@ -660,6 +660,15 @@ func BenchmarkParser(b *testing.B) { } }) } + for _, c := range cases { + b.Run("preprocess "+c, func(b *testing.B) { + expr, _ := parser.ParseExpr(c) + start, end := time.Now().Add(-time.Hour), time.Now() + for i := 0; i < b.N; i++ { + promql.PreprocessExpr(expr, start, end) + } + }) + } for _, c := range errCases { name := fmt.Sprintf("%s (should fail)", c) b.Run(name, func(b *testing.B) { From ceac4d24188b5daad528dfbd99c5e39d9747efa4 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Sun, 11 May 2025 11:49:15 +0100 Subject: [PATCH 2/4] [PERF] PromQL: Walk syntax tree using iterator Saves memory allocations. This is called a few times from the PromQL Engine. Signed-off-by: Bryan Boreham --- promql/parser/ast.go | 106 +++++++++++++++++++++------------------ promql/parser/printer.go | 2 +- 2 files changed, 58 insertions(+), 50 deletions(-) diff --git a/promql/parser/ast.go b/promql/parser/ast.go index dc3e36b5b5..882d3ccb78 100644 --- a/promql/parser/ast.go +++ b/promql/parser/ast.go @@ -336,7 +336,7 @@ func Walk(v Visitor, node Node, path []Node) error { } path = append(path, node) - for _, e := range Children(node) { + for e := range ChildrenIter(node) { if err := Walk(v, e, path); err != nil { return err } @@ -375,59 +375,67 @@ func Inspect(node Node, f inspector) { Walk(f, node, nil) //nolint:errcheck } -// Children returns a list of all child nodes of a syntax tree node. -func Children(node Node) []Node { - // For some reasons these switches have significantly better performance than interfaces - switch n := node.(type) { - case *EvalStmt: - return []Node{n.Expr} - case Expressions: - // golang cannot convert slices of interfaces - ret := make([]Node, len(n)) - for i, e := range n { - ret[i] = e - } - return ret - case *AggregateExpr: - // While this does not look nice, it should avoid unnecessary allocations - // caused by slice resizing - switch { - case n.Expr == nil && n.Param == nil: - return nil - case n.Expr == nil: - return []Node{n.Param} - case n.Param == nil: - return []Node{n.Expr} +// ChildrenIter returns an iterator over all child nodes of a syntax tree node. +func ChildrenIter(node Node) func(func(Node) bool) { + return func(yield func(Node) bool) { + // According to lore, these switches have significantly better performance than interfaces + switch n := node.(type) { + case *EvalStmt: + yield(n.Expr) + case Expressions: + for _, e := range n { + if !yield(e) { + return + } + } + case *AggregateExpr: + if n.Param != nil { + if !yield(n.Param) { + return + } + } + if n.Expr != nil { + yield(n.Expr) + } + case *BinaryExpr: + if !yield(n.LHS) { + return + } + yield(n.RHS) + case *Call: + for _, e := range n.Args { + if !yield(e) { + return + } + } + case *SubqueryExpr: + yield(n.Expr) + case *ParenExpr: + yield(n.Expr) + case *UnaryExpr: + yield(n.Expr) + case *MatrixSelector: + yield(n.VectorSelector) + case *StepInvariantExpr: + yield(n.Expr) + case *NumberLiteral, *StringLiteral, *VectorSelector: + // nothing to do default: - return []Node{n.Expr, n.Param} + panic(fmt.Errorf("promql.Children: unhandled node type %T", node)) } - case *BinaryExpr: - return []Node{n.LHS, n.RHS} - case *Call: - // golang cannot convert slices of interfaces - ret := make([]Node, len(n.Args)) - for i, e := range n.Args { - ret[i] = e - } - return ret - case *SubqueryExpr: - return []Node{n.Expr} - case *ParenExpr: - return []Node{n.Expr} - case *UnaryExpr: - return []Node{n.Expr} - case *MatrixSelector: - return []Node{n.VectorSelector} - case *StepInvariantExpr: - return []Node{n.Expr} - case *NumberLiteral, *StringLiteral, *VectorSelector: - // nothing to do - return []Node{} - default: - panic(fmt.Errorf("promql.Children: unhandled node type %T", node)) } } +// Children returns a list of all child nodes of a syntax tree node. +// Implemented for backwards-compatibility; prefer ChildrenIter(). +func Children(node Node) []Node { + ret := []Node{} + for e := range ChildrenIter(node) { + ret = append(ret, e) + } + return ret +} + // mergeRanges is a helper function to merge the PositionRanges of two Nodes. // Note that the arguments must be in the same order as they // occur in the input string. diff --git a/promql/parser/printer.go b/promql/parser/printer.go index 9dae10a70e..f64544f4f2 100644 --- a/promql/parser/printer.go +++ b/promql/parser/printer.go @@ -41,7 +41,7 @@ func tree(node Node, level string) string { level += " · · ·" - for _, e := range Children(node) { + for e := range ChildrenIter(node) { t += tree(e, level) } From e4789cf89f81a5dfaaabf813f316af267f8630bc Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Sun, 11 May 2025 12:17:22 +0100 Subject: [PATCH 3/4] [PERF] PromQL: Reduce allocations when walking syntax tree Currently it allocates at least once on every recursion, and since `append` gives no extra space for 1 or 2 elements, will allocate several times for even modestly complex expressions. Instead, allocate space for 4 elements to begin, and defer adding another until we need it. Add a note that `path` may get overwritten; this was true previously. Signed-off-by: Bryan Boreham --- promql/parser/ast.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/promql/parser/ast.go b/promql/parser/ast.go index 882d3ccb78..2e54f3fe67 100644 --- a/promql/parser/ast.go +++ b/promql/parser/ast.go @@ -334,10 +334,13 @@ func Walk(v Visitor, node Node, path []Node) error { if v, err = v.Visit(node, path); v == nil || err != nil { return err } - path = append(path, node) + var pathToHere []Node // Initialized only when needed. for e := range ChildrenIter(node) { - if err := Walk(v, e, path); err != nil { + if pathToHere == nil { + pathToHere = append(path, node) + } + if err := Walk(v, e, pathToHere); err != nil { return err } } @@ -371,8 +374,10 @@ func (f inspector) Visit(node Node, path []Node) (Visitor, error) { // Inspect traverses an AST in depth-first order: It starts by calling // f(node, path); node must not be nil. If f returns a nil error, Inspect invokes f // for all the non-nil children of node, recursively. +// Note: path may be overwritten after f returns; copy path if you need to retain it. func Inspect(node Node, f inspector) { - Walk(f, node, nil) //nolint:errcheck + var pathBuf [4]Node // To reduce allocations during recursion. + Walk(f, node, pathBuf[:0]) //nolint:errcheck } // ChildrenIter returns an iterator over all child nodes of a syntax tree node. From 75b72c2e42fba65e6a869edbaa3104cf3bac307d Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Fri, 11 Jul 2025 14:03:38 +0100 Subject: [PATCH 4/4] Apply review feedback Make the order of aggregate parts the same as before. Make error message match. Fix up benchmark for changes elsewhere. Signed-off-by: Bryan Boreham --- promql/bench_test.go | 2 +- promql/parser/ast.go | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/promql/bench_test.go b/promql/bench_test.go index 773ba2acfb..091ac71347 100644 --- a/promql/bench_test.go +++ b/promql/bench_test.go @@ -665,7 +665,7 @@ func BenchmarkParser(b *testing.B) { expr, _ := parser.ParseExpr(c) start, end := time.Now().Add(-time.Hour), time.Now() for i := 0; i < b.N; i++ { - promql.PreprocessExpr(expr, start, end) + promql.PreprocessExpr(expr, start, end, 0) } }) } diff --git a/promql/parser/ast.go b/promql/parser/ast.go index 2e54f3fe67..11b3beb43e 100644 --- a/promql/parser/ast.go +++ b/promql/parser/ast.go @@ -394,13 +394,13 @@ func ChildrenIter(node Node) func(func(Node) bool) { } } case *AggregateExpr: - if n.Param != nil { - if !yield(n.Param) { + if n.Expr != nil { + if !yield(n.Expr) { return } } - if n.Expr != nil { - yield(n.Expr) + if n.Param != nil { + yield(n.Param) } case *BinaryExpr: if !yield(n.LHS) { @@ -426,7 +426,7 @@ func ChildrenIter(node Node) func(func(Node) bool) { case *NumberLiteral, *StringLiteral, *VectorSelector: // nothing to do default: - panic(fmt.Errorf("promql.Children: unhandled node type %T", node)) + panic(fmt.Errorf("promql.ChildrenIter: unhandled node type %T", node)) } } }