Labeled as experimental as we'll want some time to adjust this
based on real editor integrations. Initial testing looks very
good though, and dramatically reduces the time taken to test
e.g. a package compared to the whole workspace and filtering
the result afterwards.
Fixes#6696
Signed-off-by: Anders Eknert <anders.eknert@apple.com>
The OPA parser places a single boolean true term inside an expression
to populate empty rule bodies, so that e.g. `x := input.x` parses as:
```rego
x := input.x if {
true
}
```
When the compiler rewrites rules in its various stages, it would
previously ignore such "true expressions" and simply append what
it rewrote as the next expression:
```
x := __local1__ if {
true
__local1__ = input.x
}
```
This is of course redundant, as the only purpose of the `true` expr
was to mark the body as empty — which it no longer is! While fairly
harmless, this contributes noise to the compiler's output, which is
then passed onwards to topdown evaluation. Not a huge deal, but
measurable!
**BenchmarkRegalLintingItselfPrepareOnce**
```
502062750 ns/op 1900946509 B/op 45426050 allocs/op
496509250 ns/op 1893094357 B/op 45112660 allocs/op
```
With this change, the compiler now replaces an empty true expression with
a new one whenever we append to a body. Meaning `true` remains to mark
empty bodies, but not in non-empty ones.
This is more about elegance than performance though :)
Signed-off-by: Anders Eknert <anders.eknert@apple.com>
Some work I did during the holidays as part of improving the performance
of interpolated strings. This change is however not isolated to those, but
updates the `String()` implementation of all AST node types (term values
and policy components). This change also lays the groundwork for migrating
OPA to the `json/v2` package once that's stable. The `json/v2` package
provides low-level functions for zero alloc marshalling via appenders — and
well, here they are. The appenders here should be usable for that purpose with
only a few tweaks needed for the few cases where our `String()` implementations
aren't also valid JSON.
Creating perfectly sized buffers requires knowing the expected length beforehand.
In order to do this, each component now implements not only `encoding.AppendText`
but a new custom `StringLengther` interface, which allows asking any AST node about
its `StringLength()` before `make`ing a buffer of that length.
We could definitely consider adding these to e.g. the `Value` or `Node` interfaces,
but I've left that out of this PR as it's an easy thing to do later should we want
to, and I guess there's always some concerns about changing public interfaces even
when they're not meant to be implemented by external code.
While no `Value` appenders allocate and almost none of the policy appenders do either,
one notable exception is `Module` when there are annotations present, as they are
a bit of a (YAML) special case. It's doable, but as serializing full modules isn't
on a hot path anywhere, I have chosen to defer that work to the future.
Signed-off-by: Anders Eknert <anders.eknert@apple.com>
Replace blind trust of gzip trailer size with io.LimitReader to
prevent memory exhaustion from forged payloads. Add a regression
test, and refactor to use test cases.
Signed-off-by: Ville Vesilehto <ville@vesilehto.fi>
As @srenatus pointed out in a previous review, these were a little
messy in some places. This moves the generic SyncPool type to the
util package for reuse across the codebase. Also some various related
improvements.
Signed-off-by: Anders Eknert <anders.eknert@apple.com>
Parsing is generally fast, so this mainly improves performance
of creating big bundles with many Rego files in them. For Regal's
embedded bundle, loading it from memory would previously take 16
ms on my laptop, and now it takes 9 ms. There are other things
in this process that could be concurrent too, like JSON unmarshalling
of multiple data files. But starting with parsing modules.
This PR adds `errgroup` as a direct dependency (previously indirect)
as it is a nicer way to work with wait groups, and one that can be
useful elsewhere in the codebase (like in the compiler).
Also, and as usual, went off on a bit of a tangent refactoring code
related to the bundle build process, and made sure to use some common
helpers in code where available.
Signed-off-by: Anders Eknert <anders.eknert@apple.com>
- Bump golangci-lint -> 2.6.2
- Fix all `deprecatedComment` "notices should be in a dedicated paragraph, separated from the rest" reports
- Enable `appendCombine` and fix all "appendCombine: can combine chain of X appends into one" notices
- Enable `preferFprint` and fix the few reported issues
- Fix various issues reported only once or twice, like `zeroByteRepeat`
Signed-off-by: Anders Eknert <anders.eknert@apple.com>
This was something I originally intended to use in another project,
but since this turned out to be a better implementation (subject of
course to review!) in terms of both performance and simplicity, I
figured we might as well use it here too. Some changes include:
- `MustParse` to parse known valid versions
- Allocates nothing in any operations other than 1 alloc in `String()`
- Ignores empty `PreRelease` and `Metadata` fields in serialization
- `encoding.TextAppender` implementation to serialize without allocating
- A whole bunch of benchmarks
Signed-off-by: Anders Eknert <anders@eknert.com>
Have done this some time in the past, but there was a few
new issues this would highlight now that we're on Go 1.24.
Mostly:
- Use `b.Loop()` in benchmarks
- Use `strings.SplitSeq` where possible
- Remove `omitempty` tag for types that can't be empty
Signed-off-by: Anders Eknert <anders@eknert.com>
- Use new `util.SlicePool` to avoid cost of temporary slices in `formatTerm`
- Add `SkipDefensiveCopying` option and enable it for all `Source`* functions
```
676179 ns/op 995204 B/op 8850 allocs/op // regression in 0fb7526
513570 ns/op 378787 B/op 8775 allocs/op // addressed regression with sync.Pool
481681 ns/op 352130 B/op 7954 allocs/op // new util.NewSlicePool using only pointers
365116 ns/op 160528 B/op 2098 allocs/op // new SkipDefensiveCopying option
```
Signed-off-by: Anders Eknert <anders@eknert.com>
The way the gzip reader was returned to the sync.Pool meant
there were cases where it could either be nil, or not returned
to the pool.
Also simplified the function a bit.
Signed-off-by: Anders Eknert <anders@eknert.com>
Save 1 alloc per invocation of the `split` built-in function by
using `strings.Cut` instead of `strings.Split`, thus avoiding
creating an intermediate string slice. Added a `SplitMap` helper
for doing this elsewhere in our code where `strings.Split` could
be replaced.
Signed-off-by: Anders Eknert <anders@eknert.com>
* v1/plugins: Address race in config access
I ran into this race condition on another PR:
https://github.com/open-policy-agent/opa/actions/runs/16655603110/job/47139789057
I have tried to make all manager.Config access thread-safe by adding new
getters for used values. GetConfig is regrettably based on a JSON
roundtrip deep copy of the config. This us used in tests (fine) but also
in the discovery plugin:
https://github.com/open-policy-agent/opa/blob/2d014a89bbbc307d7204817220146ffae992e838/v1/plugins/discovery/discovery.go#L122
getPluginSet is very tightly coupled to the manager.Config and because
of it's dependencies on status and the other plugins packages, it's hard
to break out.
So, for now, I think this is an improvement and worth getting a second
opinion on before more refactoring.
Signed-off-by: Charlie Egan <charlie@styra.com>
* v1/config: Use add Clone to config
This makes the use of the manager's config more thread-safe and
consistent without more API changes.
Signed-off-by: Charlie Egan <charlie@styra.com>
* topdown: Add clone() funcs for config structs
NamedValueCacheConfig.Clone, InterQueryBuiltinValueCacheConfig.Clone and
InterQueryBuiltinCacheConfig.Clone have been added.
All Clone methods return a deep copy of the struct. This is tested for
missed new fields using PopulateAllFields, a generic function that
stuffs structs with values for all fields.
Signed-off-by: Charlie Egan <charlie@styra.com>
* plugins: Clone new config
Signed-off-by: Charlie Egan <charlie@styra.com>
---------
Signed-off-by: Charlie Egan <charlie@styra.com>
* plugin/bundle: Correct bundle delay behavior
I ran into an issue when testing an earlier change:
https://github.com/open-policy-agent/opa/actions/runs/16646981900/job/47110035165
I found that this test generated around 100,000 lines of errors showing
the bundle downloader running.
This can be be tested using:
```
go test -v ./v1/plugins/bundle -count=1 2>&1 | grep -c "request failed"
```
This commit closes managers and plugins correctly.
Signed-off-by: Charlie Egan <charlie@styra.com>
* download: Update stop to be idempotent
I had some race detector issues with TestStartStopWithLongPollNotSupported
https://github.com/open-policy-agent/opa/actions/runs/16722869930/job/47334690407?pr=7812
I think this is a deadlock around multiple calls to Stop dead locking
updating the stopped var.
Signed-off-by: Charlie Egan <charlie@styra.com>
---------
Signed-off-by: Charlie Egan <charlie@styra.com>
Funnily, this started out as an attempt to look into issues reported
with compiling large policy sets... before I realized that it isn't
likely *this* compiler that has perf issues, but the one that "compiles"
bundles as part of activation. So while these fixes likely does little
to address that, there are still some rather nice improvements here, where
the big ones as ususal are mostly just wins from avoiding work where it's
possible.
For benchmarking I've used Regal's embedded bundle, which isn't great to
use over time, as it's a moving target. But since it's a pretty extensive
bundle and one that covers most features of OPA, it's at least good for
1:1 comparisons when testing perf improvements.
```
// 66555594 ns/op 50239492 B/op 1083664 allocs/op - main
// 62569440 ns/op 38723015 B/op 944277 allocs/op - compiler-optimizations pr
```
The B/op / alloc_space improvement is particularly nice here. What's noteworthy
is how relatively little impact that has on performance in this case. That may
be surprising but aligns pretty well with my previous experience of Go code where
a lot of time is spend in recursive walks — that simply takes time, no matter how
much you optimize. Oh well, less memory allocated for this is more memory to spend
elsewhere.
(I'm adding the benchmark used below to Regal in a parallel PR)
Signed-off-by: Anders Eknert <anders@styra.com>
Following up on #7566, and now applying the more exciting
modernizations. fmt.Appendf was new to me! But especially
the contains checks are so much better IMHO. I have reviewed
all changes myself and did a few manual changes where it
became obvious that things could be improved a little further.
(the modernize analyzer still has some issues running against
OPA, and I have manually worked around those for the time being)
Signed-off-by: Anders Eknert <anders@styra.com>
If a status API is slow to respond it can cause OPA to be blocked writing to an unbuffered channel. This fixes it by using a buffered channel that never blocks but drops the oldest status update if full.
Signed-off-by: Sebastian Spaink <3441183+sspaink@users.noreply.github.com>
`TestControlPlaneSpans` could case a race condition, where the discovery plugin is manually triggered before/during server initialization, resulting in the manager config being changed while actively consumed.
Replacing `Runtime.serverInitialized` boolean field with more granular enum type state, to allow test-runtime to hold off on triggering plugins until runtime is actively waiting for plugin ready state.
Currently, manager config writes are guarded by an internal mutex, while config reads are largely unguarded. A broader fix here might be to deprecate the public `plugins.Manager.Context` field, replacing it with a getter that guards the config with an r/w-lock.
Also fixing:
* Possible race condition in telemetry reporter by using r/w-mutex guarded compiler getter instead of direct field access
* AWS signing tests where signing randomly failed because of too small mock random value used in test
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Brace yourselves! For there are many touched files here. No changes
in semantics however.
Spent a long time trying out the various optional rules gocritic
provides, and settled for a few of them. There are more I really
like, but that would take many hours to address across the codebase.
Perhaps others find gocritic too pedantic? If so, we can merge the
fixes without enabling the rule.
Signed-off-by: Anders Eknert <anders@styra.com>
This is a simpler version of util.TypedHashMap where the keys
implement a `.Hash()` method and as such won't need one to be passed
in, and where the values are largely ignored by the map. These maps
are smaller / more performant, but most importantly, they are nicer
to work with.
Perf wise, this saves about 600k+ allocs and 40 MB allocated memory
in `regal lint bundle`:
```
1207614875 ns/op 3293454016 B/op 64802095 allocs/op
1197978125 ns/op 3256960504 B/op 64164871 allocs/op
```
Also:
- Use `strings.Builder` instead of `fmt.Sprintf` in one location
- Remove `ValueMap.Copy` as it was only used in a test
Signed-off-by: Anders Eknert <anders@styra.com>
And update code to conform to the rule.
- Replace unnecessary fmt.Sprintf with string concatenation
- Replace fmt.Sprint with more efficient strconv.Itoa
- Replace static fmt.Errorf calls with more efficient errors.New
Thanks @srenatus for pushing me down this rabbit hole!
Signed-off-by: Anders Eknert <anders@styra.com>
Another PR to reduce allocations, split up from my rather
huge local changeset. Here are a few tricks to avoid heap
allocations in the eval hotpath, mainly by not letting
variables escape their scope, and to make sure we don't
evaluate code for partial evaluation when that's not
the mode we're running with.
About 3.3 million allocations less reported by the
`regal lint bundle` benchmark.
**BenchmarkRegalLintingItself-10 before / after**
```
1822641834 ns/op 3464916080 B/op 66300871 allocs/op
1762596000 ns/op 3380345056 B/op 62983984 allocs/op
```
Signed-off-by: Anders Eknert <anders@styra.com>
And a few other small fixes in tests. This i not so much
about performance but about choosing the best tool for a
given task :) But that the alternatives are also faster
doesn't hurt either.
Signed-off-by: Anders Eknert <anders@styra.com>
And use them to reduce imperative boilerplate throughout
the codebase.
Additionally, replace use of sort.Slice with slices.SortFunc
which is more efficient since it is generic and as such avoids
allocations related to `interface{}` casts.
Also a few performance-related minor fixes, but not the main
theme of this PR.
```
BenchmarkRegalLintingItself-10 before / after
1832684458 ns/op 3453470360 B/op 66125422 allocs/op
1826601250 ns/op 3449619024 B/op 65999164 allocs/op
````
Signed-off-by: Anders Eknert <anders@styra.com>
Having worked on performance improvements in OPA on the side for almost
a month now, there's a lot of code piling up 😅 So much that a single PR
would be way too much to review. Instead, I'm splitting the work into
chunks, and will submit the next PR as soon as this one is merged. Using
the same benchmark as before — Regal linting itself, these new changes
in total reduce the number of allocations by ~13 million, and quite a
substantial amount of evaluation time saved as well.
This first PR is isolated to improvements to terms, values and
built-ins, and saves ~3M allocations. The details can be found below for
each change, and of course in the code :)
**BenchmarkRegalLintingItself-10 Before**
```
1885978209 ns/op 3497157312 B/op 69064779 allocs/op
```
**BenchmarkRegalLintingItself-10 After**
```
1796255084 ns/op 3452379408 B/op 66126623 allocs/op
```
**Terms**
- Use pointer receivers consistently for object and set types. This allows
changing the sortGuard once lock from a pointer to a non-pointer type, which
is really the biggest win performance-wise in this PR.
- Comparisons happen all the time, so make sure these take the shortest path
possible whenever, possible, such as when one type is compared to another
value of the same type.
Built-in functions:
**Arrays**
- Both `array.concat` and `array.slice` will now return the operand on operations
where the result isn't different from the input operand (like when concatenating
an empty array) instead of allocating a new term/value.
**Strings**
- Return operand on unchanged result rather than allocating new term/value.
- Where applicable, have functions take a cheaper path when string is ASCII
and we can avoid the cost of rune conversion.
**Crypto**
- Hashing functions now optimized, spending less than half the time compared to
previously.
**Objects**
- Avoid heap allocating result boolean escaping its scope, and instead use the
return value of the `Until` function.
**HTTP**
- Use interned terms for keys in configuration object, avoding allocating these
each time `http.send` is invoked.
**Globs**
- Use read/write lock to avoid contention. Use package level vars for "constant"
values, avoiding them to escape to the heap each invocation.
**Not directly/only related to built-in functions**
- Add `ValueName` function replacing the previous `TypeName` functions for
getting the name of Value's without paying for `any` interface allocations.
- Add a few more interned terms.
Signed-off-by: Anders Eknert <anders@styra.com>
My last PR for a while in the ongoing "reduce allocations in eval" quest.
Motivated initially mostly to speed up `regal lint`, but most of the changes
here positively impacts evaluation performance for most policies.
The changes with the highest impact in this PR:
* Use `sync.Pool`s to avoid the most costly allocations, includuing heavy `*eval`
pointers created each time a child or closure scope is evaluated.
* When tracing is disabled, avoid variable escaping to heap in `evalStep` function
whose value is only read when tracing is enabled.
* Save one allocation per iteration in `walkNoPath` by reusing an AST array instead
of creating a new one for each call.
Also a few minor fixes here and there which either fixed some correctness issue, or
had a measurable (although minor) positive impact on performance.
**regal lint bundle (main)**
```
BenchmarkRegalLintingItself-10 1 2015560750 ns/op 4335625360 B/op 83728460 allocs/op
```
**regal lint bundle (now)**
```
BenchmarkRegalLintingItself-10 1 1828754125 ns/op 3541027496 B/op 70080568 allocs/op
```
About 10% faster eval, with almost a gigabyte less memory allocated, and 13 million+ allocations
less performed.
Another topic discussed recently has been the cost of calling custom functions in hot paths.
While this PR doesn't address that problem fully, the benefits of the change is still quite
noticeable. A benchmark for that case specifically is also included in the PR, and the change
compared to main as noted below:
**main**
```
BenchmarkCustomFunctionInHotPath-10 55 18543908 ns/op 20821043 B/op 284611 allocs/op
```
**pr**
```
BenchmarkCustomFunctionInHotPath-10 73 16247587 ns/op 13048108 B/op 228406 allocs/op
```
It's worth noting however that this benchmark benefits "unfairly" by the improvements made
in the `walkNoPath` function, and perhaps more so than custom function evaluation getting
that much more efficient.
Signed-off-by: Anders Eknert <anders@styra.com>
Stephan Renatus