Tweaks to reduce number of allocations in regal lint hot path

[anderseknert]

Concluding my quest to reduce the number of allocations in the hot path for regal lint for this time around. This PR mainly does so by reusing pointers to boolean and integer terms where these are determined not to be mutated later.

The result is another ~4 million allocations reduced when linting Regal against its own bundle. These improvements should however help reduce allocations in pretty much any evaluation.

opa main

BenchmarkRegalLintingItself-10    1	3195257584 ns/op	6496097784 B/op	120108808 allocs/op

PR branch

BenchmarkRegalLintingItself-10    1	3132126333 ns/op	6376318224 B/op	116163318 allocs/op

[srenatus]

These are all great changes, thank you. I suppose I'd rather change the existing ast methods BooleanTerm and IntNumberTerm than add new functions, but it's not a strong opinion, I'd be curious whah @ash-styra and @johanfylling think about that.

[srenatus]

Ran with benchstat, just for completeness...

goos: darwin
goarch: amd64
pkg: github.com/styrainc/regal/pkg/linter
cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
                      │ main.bench  │      less-allocs.bench      │
                      │   sec/op    │   sec/op    vs base         │
RegalLintingItself-16   3.797 ± 10%   3.557 ± 8%  ~ (p=0.240 n=6)

                      │  main.bench  │         less-allocs.bench          │
                      │     B/op     │     B/op      vs base              │
RegalLintingItself-16   6.053Gi ± 0%   5.943Gi ± 0%  -1.83% (p=0.002 n=6)

                      │ main.bench  │         less-allocs.bench         │
                      │  allocs/op  │  allocs/op   vs base              │
RegalLintingItself-16   120.1M ± 0%   116.2M ± 0%  -3.28% (p=0.002 n=6)

[anderseknert]

@srenatus existing functions are sometimes used like this:

opa/ast/parser_ext.go

Line 286 in c47b8c1

body := NewBody(NewExpr(BooleanTerm(true).SetLocation(rhs.Location)).SetLocation(rhs.Location))

i.e mutating the pointer returned. If we want to have a single function to create bool/int terms, we'd have to at least identify all uses where mutation can happen after the pointer is returned. Having separate functions for cached values seemed like at least some way to try and communicate intent.

[srenatus]

Ah right, these are terms! Yeah that makes sense, many of them will have a location. OK I'm convinced. I'm little on the fence about the "cached" terminology... a cache somehow carries the connotation that you can have a hit or a miss, or that something is cached for a while and then isn't. Would "Static" seem like a valid alternative? "Interned"? 🤔

[anderseknert]

Interned sounds good to me! I'll make that change.

[srenatus]

@ash-styra @johanfylling could you have a look, please? 😎

[ashutosh-narkar]

These changes look good. Few comments 👇

[srenatus]

Let's get this merged! 🥳