Skip to content

Commit 15821df

Browse files
authored
Benchmark and performance improvements (#289)
This patch updates our benchmarks to be more focused and "micro" which should make it easier to identify and address particular perf bottlenecks. Only a couple of benchmarks have been added so far, covering singular scalar fields, repeated scalar and message fields, and repeated fields including a unique rule. The benchmarks can be run consistently with `make bench` which has some args that may be customized (see the Makefile). --- This patch also includes a handful of performance improvements, focused on heap usage (though there was a ~5% CPU time improvement): - We use cel-go's `ReduceResiduals` to minimize/optimize the CEL programs. This means the `rule` & `rules` globals variables used by standard and predefined CEL expressions can be eliminated from the final program (since the values we use from it are injected as constant literals in the reduced AST). However, these globals were persisted in the `cel.Env` which caused cel-go to allocate a composite Activation to make them accessible alongside the `this` variable. Instead of using CEL globals, this patch uses them as normal variables prior to computing residuals, and elides them during actual execution of the CEL program, avoiding the allocation. - In order to keep `repeated.unique` `O(n)`, during validation we build up a `map[T]struct{}{}` to check for uniqueness in the list. This rule is particularly expensive, resulting in this map being allocated and thrown away on every validation. While this rule could avoid allocations altogether by making the comparison O(n^2) (effectively the CEL expression `this.all(x, this.exists_one(y, x == y))`), I instead opted to have the unique maps pull from a `sync.Pool`. Since the O(n^2) is only an issue for large lists, in the future we could either use a heuristic to swap between the CEL above or the map-based solution. - `errors.As` ends up allocating when you take the double-pointer to the target error, even when the source error is nil. (The escape analysis can't see that far, unfortunately). Since the majority of the time validation is successful, err is almost always nil. Performing a nil check before calls to `errors.As` eliminates this allocation (albeit small). - For every call to `Validate`, we construct a config struct that's drives the behavior of that single validation (things like fail-fast mode, filtering, and the now CEL function). Typically, these are set globally on the Validator instance itself, but can be overridden at validation time. However, even if they weren't set at validation time, we were still computing a new config object for every call, causing an extra allocation. Now, the config is constructed only once with the Validator and only copied and overwritten if validation time options are provided. These changes resulted in the following improvements on the (admittedly limited) set of benchmarks added in d716bad: ``` → benchstat .tmp/bench/2025-11-18:12:58:39.bench.txt .tmp/bench/2025-11-18:13:01:39.bench.txt goos: darwin goarch: arm64:52:03.cpu.profile 2025-11-18:12:58:39.bench.txt 2025-11-18:12:58:39.mem.profile 2025-11-18:13:01:39.cpu.profile pkg: buf.build/go/protovalidate cpu: Apple M1 Max │ .tmp/bench/2025-11-18:12:58:39.bench.txt │ .tmp/bench/2025-11-18:13:01:39.bench.txt │ │ sec/op │ sec/op vs base │ Scalar-10 421.8n ± 1% 396.0n ± 3% -6.12% (p=0.000 n=10) Repeated/Scalar-10 480.5n ± 1% 455.0n ± 2% -5.30% (p=0.001 n=10) Repeated/Message-10 607.0n ± 1% 561.2n ± 1% -7.55% (p=0.000 n=10) Repeated/Unique/Scalar-10 735.4n ± 3% 686.2n ± 2% -6.68% (p=0.000 n=10) Repeated/Unique/Bytes-10 987.1n ± 4% 933.9n ± 3% -5.39% (p=0.000 n=10) geomean 616.8n 578.5n -6.21% │ .tmp/bench/2025-11-18:12:58:39.bench.txt │ .tmp/bench/2025-11-18:13:01:39.bench.txt │ │ B/op │ B/op vs base │ Scalar-10 72.00 ± 0% 0.00 ± 0% -100.00% (p=0.000 n=10) Repeated/Scalar-10 192.0 ± 0% 120.0 ± 0% -37.50% (p=0.000 n=10) Repeated/Message-10 256.0 ± 0% 120.0 ± 0% -53.12% (p=0.000 n=10) Repeated/Unique/Scalar-10 1064.0 ± 0% 536.0 ± 0% -49.62% (p=0.000 n=10) Repeated/Unique/Bytes-10 2.398Ki ± 0% 1.743Ki ± 0% -27.32% (p=0.000 n=10) geomean 391.9 ? ¹ ² ¹ summaries must be >0 to compute geomean ² ratios must be >0 to compute geomean │ .tmp/bench/2025-11-18:12:58:39.bench.txt │ .tmp/bench/2025-11-18:13:01:39.bench.txt │ │ allocs/op │ allocs/op vs base │ Scalar-10 3.000 ± 0% 0.000 ± 0% -100.00% (p=0.000 n=10) Repeated/Scalar-10 6.000 ± 0% 3.000 ± 0% -50.00% (p=0.000 n=10) Repeated/Message-10 8.000 ± 0% 3.000 ± 0% -62.50% (p=0.000 n=10) Repeated/Unique/Scalar-10 40.00 ± 0% 34.00 ± 0% -15.00% (p=0.000 n=10) Repeated/Unique/Bytes-10 88.00 ± 0% 73.00 ± 0% -17.05% (p=0.000 n=10) geomean 13.84 ? ¹ ² ¹ summaries must be >0 to compute geomean ² ratios must be >0 to compute geomean ```
1 parent 2473f44 commit 15821df

File tree

18 files changed

+967
-131
lines changed

18 files changed

+967
-131
lines changed

.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,3 +2,4 @@
22
*.pprof
33
*.svg
44
cover.out
5+
*.test

.golangci.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ linters:
2121
- noinlineerr # inline is fine
2222
- nonamedreturns # usage of named returns should be selective
2323
- testpackage # internal tests are fine
24+
- thelper # overzealous breaking of stack traces
2425
- wrapcheck # don't _always_ need to wrap errors
2526
- wsl # over-generous whitespace violates house style
2627
- wsl_v5 # over-generous whitespace violates house style

Makefile

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,9 @@ SHELL := bash
66
MAKEFLAGS += --warn-undefined-variables
77
MAKEFLAGS += --no-builtin-rules
88
MAKEFLAGS += --no-print-directory
9-
BIN := .tmp/bin
9+
TMP := .tmp
10+
BIN := $(TMP)/bin
11+
BENCH_TMP := $(TMP)/bench
1012
COPYRIGHT_YEARS := 2023-2025
1113
LICENSE_IGNORE := -e internal/testdata/
1214
# Set to use a different compiler. For example, `GO=go1.18rc1 make test`.
@@ -94,10 +96,26 @@ checkgenerate: generate
9496
@# Used in CI to verify that `make generate` doesn't produce a diff.
9597
test -z "$$(git status --porcelain | tee /dev/stderr)"
9698

99+
100+
BENCH ?= .
101+
BENCH_COUNT ?= 10
102+
BENCH_NAME ?= $(shell date +%F:%T)
103+
.PHONY: bench
104+
bench: $(BENCH_TMP)
105+
go test -bench="$(BENCH)" -benchmem \
106+
-memprofile "$(BENCH_TMP)/$(BENCH_NAME).mem.profile" \
107+
-cpuprofile "$(BENCH_TMP)/$(BENCH_NAME).cpu.profile" \
108+
-count $(BENCH_COUNT) \
109+
| tee "$(BENCH_TMP)/$(BENCH_NAME).bench.txt"
110+
111+
97112
.PHONY: upgrade-go
98113
upgrade-go:
99114
$(GO) get -u -t ./... && $(GO) mod tidy -v
100115

116+
$(BENCH_TMP):
117+
@mkdir -p $(BENCH_TMP)
118+
101119
$(BIN):
102120
@mkdir -p $(BIN)
103121

ast.go

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ import (
2121
"buf.build/gen/go/bufbuild/protovalidate/protocolbuffers/go/buf/validate"
2222
pvcel "buf.build/go/protovalidate/cel"
2323
"github.com/google/cel-go/cel"
24-
"github.com/google/cel-go/interpreter"
2524
"google.golang.org/protobuf/reflect/protoreflect"
2625
)
2726

@@ -41,7 +40,7 @@ func (set astSet) Merge(other astSet) astSet {
4140
// either a true or empty string constant result, no compiledProgram is
4241
// generated for it. The main usage of this is to elide tautological expressions
4342
// from the final result.
44-
func (set astSet) ReduceResiduals(opts ...cel.ProgramOption) (programSet, error) {
43+
func (set astSet) ReduceResiduals(rules protoreflect.Message, opts ...cel.ProgramOption) (programSet, error) {
4544
residuals := make(astSet, 0, len(set))
4645
options := append([]cel.ProgramOption{
4746
cel.EvalOptions(
@@ -52,17 +51,26 @@ func (set astSet) ReduceResiduals(opts ...cel.ProgramOption) (programSet, error)
5251
),
5352
}, opts...)
5453

54+
baseActivation := &variable{
55+
Name: "rules",
56+
Val: rules.Interface(),
57+
}
58+
5559
for _, ast := range set {
56-
options := slices.Clone(options)
60+
activation := baseActivation
5761
if ast.Value.IsValid() {
58-
options = append(options, cel.Globals(&variable{Name: "rule", Val: ast.Value.Interface()}))
62+
activation = &variable{
63+
Name: "rule",
64+
Val: ast.Value.Interface(),
65+
Next: activation,
66+
}
5967
}
6068
program, err := ast.toProgram(ast.Env, options...)
6169
if err != nil {
6270
residuals = append(residuals, ast)
6371
continue
6472
}
65-
val, details, _ := program.Program.Eval(interpreter.EmptyActivation())
73+
val, details, _ := program.Program.Eval(activation)
6674
if val != nil {
6775
switch value := val.Value().(type) {
6876
case bool:

ast_test.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,10 @@ func TestASTSet_ReduceResiduals(t *testing.T) {
9191
)
9292
require.NoError(t, err)
9393
assert.Len(t, asts, 1)
94-
set, err := asts.ReduceResiduals(cel.Globals(&variable{Name: "foo", Val: true}))
94+
set, err := asts.ReduceResiduals(
95+
(&validate.StringRules{}).ProtoReflect(),
96+
cel.Globals(&variable{Name: "foo", Val: true}),
97+
)
9598
require.NoError(t, err)
9699
assert.Empty(t, set)
97100
}

buf.gen.yaml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ managed:
44
disable:
55
- file_option: go_package
66
module: buf.build/bufbuild/protovalidate
7+
- file_option: go_package
8+
module: buf.build/rodaine/protogofakeit
79
override:
810
- file_option: go_package_prefix
911
value: buf.build/go/protovalidate/internal/gen

buf.lock

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,3 +4,6 @@ deps:
44
- name: buf.build/bufbuild/protovalidate
55
commit: 52f32327d4b045a79293a6ad4e7e1236
66
digest: b5:cbabc98d4b7b7b0447c9b15f68eeb8a7a44ef8516cb386ac5f66e7fd4062cd6723ed3f452ad8c384b851f79e33d26e7f8a94e2b807282b3def1cd966c7eace97
7+
- name: buf.build/rodaine/protogofakeit
8+
commit: 9caf0fc578d3413590962a1764b81b94
9+
digest: b5:eeead7373f2f598ebc8f91aa3a68d6b50630076341d875b22dc6760126bc56c82cf1e98f5a2eff9815ba55fa48ab81745c93a5aeefd5e4697bf43c9ea4694735

buf.yaml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ modules:
33
- path: proto
44
deps:
55
- buf.build/bufbuild/protovalidate:v1.0.0
6+
- buf.build/rodaine/protogofakeit
67
lint:
78
use:
89
- STANDARD

cache.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -100,8 +100,7 @@ func (c *cache) Build(
100100
return nil, err
101101
}
102102

103-
rulesGlobal := cel.Globals(&variable{Name: "rules", Val: rules.Interface()})
104-
set, err = asts.ReduceResiduals(rulesGlobal)
103+
set, err = asts.ReduceResiduals(rules)
105104
return set, err
106105
}
107106

cel/library.go

Lines changed: 34 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"slices"
2323
"strconv"
2424
"strings"
25+
"sync"
2526
"unicode/utf8"
2627

2728
"github.com/google/cel-go/cel"
@@ -49,7 +50,14 @@ var (
4950
// Using this function, you can create a CEL environment that is identical to
5051
// the one used to evaluate protovalidate CEL expressions.
5152
func NewLibrary() cel.Library {
52-
return library{}
53+
return &library{
54+
uniqueScalarPool: sync.Pool{New: func() any {
55+
return map[ref.Val]struct{}{}
56+
}},
57+
uniqueBytesPool: sync.Pool{New: func() any {
58+
return map[string]struct{}{}
59+
}},
60+
}
5361
}
5462

5563
// library is the collection of functions and settings required by protovalidate
@@ -59,9 +67,12 @@ func NewLibrary() cel.Library {
5967
//
6068
// All implementations of protovalidate MUST implement these functions and
6169
// should avoid exposing additional functions as they will not be portable.
62-
type library struct{}
70+
type library struct {
71+
uniqueScalarPool sync.Pool
72+
uniqueBytesPool sync.Pool
73+
}
6374

64-
func (l library) CompileOptions() []cel.EnvOption { //nolint:funlen,gocyclo
75+
func (l *library) CompileOptions() []cel.EnvOption { //nolint:funlen,gocyclo
6576
return []cel.EnvOption{
6677
cel.TypeDescs(protoregistry.GlobalFiles),
6778
cel.DefaultUTCTimeZone(true),
@@ -375,15 +386,15 @@ func (l library) CompileOptions() []cel.EnvOption { //nolint:funlen,gocyclo
375386
}
376387
}
377388

378-
func (l library) ProgramOptions() []cel.ProgramOption {
389+
func (l *library) ProgramOptions() []cel.ProgramOption {
379390
return []cel.ProgramOption{
380391
cel.EvalOptions(
381392
cel.OptOptimize,
382393
),
383394
}
384395
}
385396

386-
func (l library) uniqueMemberOverload(itemType *cel.Type, overload func(lister traits.Lister) ref.Val) cel.FunctionOpt {
397+
func (l *library) uniqueMemberOverload(itemType *cel.Type, overload func(lister traits.Lister) ref.Val) cel.FunctionOpt {
387398
return cel.MemberOverload(
388399
itemType.String()+"_unique_bool",
389400
[]*cel.Type{cel.ListType(itemType)},
@@ -398,15 +409,19 @@ func (l library) uniqueMemberOverload(itemType *cel.Type, overload func(lister t
398409
)
399410
}
400411

401-
func (l library) uniqueScalar(list traits.Lister) ref.Val {
412+
func (l *library) uniqueScalar(list traits.Lister) ref.Val {
402413
size, ok := list.Size().Value().(int64)
403414
if !ok {
404415
return types.UnsupportedRefValConversionErr(list.Size().Value())
405416
}
406417
if size <= 1 {
407418
return types.Bool(true)
408419
}
409-
exist := make(map[ref.Val]struct{}, size)
420+
exist := l.uniqueScalarPool.Get().(map[ref.Val]struct{}) //nolint:errcheck // guaranteed to match
421+
defer func() {
422+
clear(exist)
423+
l.uniqueScalarPool.Put(exist)
424+
}()
410425
for i := range size {
411426
val := list.Get(types.Int(i))
412427
if _, ok := exist[val]; ok {
@@ -421,24 +436,30 @@ func (l library) uniqueScalar(list traits.Lister) ref.Val {
421436
// compares bytes type CEL values. This function is used instead of uniqueScalar
422437
// as the bytes ([]uint8) type is not hashable in Go; we cheat this by converting
423438
// the value to a string.
424-
func (l library) uniqueBytes(list traits.Lister) ref.Val {
439+
func (l *library) uniqueBytes(list traits.Lister) ref.Val {
425440
size, ok := list.Size().Value().(int64)
426441
if !ok {
427442
return types.UnsupportedRefValConversionErr(list.Size().Value())
428443
}
429444
if size <= 1 {
430445
return types.Bool(true)
431446
}
432-
exist := make(map[any]struct{}, size)
447+
exist := l.uniqueBytesPool.Get().(map[string]struct{}) //nolint:errcheck // guaranteed to match
448+
defer func() {
449+
clear(exist)
450+
l.uniqueBytesPool.Put(exist)
451+
}()
433452
for i := range size {
434453
val := list.Get(types.Int(i)).Value()
435-
if b, ok := val.([]uint8); ok {
436-
val = string(b)
454+
b, ok := val.([]byte)
455+
if !ok {
456+
return types.NewErr("expected bytes, got %v", val)
437457
}
438-
if _, ok := exist[val]; ok {
458+
str := string(b)
459+
if _, ok := exist[str]; ok {
439460
return types.Bool(false)
440461
}
441-
exist[val] = struct{}{}
462+
exist[str] = struct{}{}
442463
}
443464
return types.Bool(true)
444465
}

0 commit comments

Comments
 (0)