Skip to content

Commit

Permalink
Avoid allocations by not calling custom functions on hot path (#1289)
Browse files Browse the repository at this point in the history
This is absolutely wild 🙃

**Before**
```
BenchmarkRegalLintingItself-10	1	3245668375 ns/op	6689411656 B/op	127195706 allocs/op
```

**After**
```
BenchmarkRegalLintingItself-10  1	2777580459 ns/op	5235852824 B/op	107686435 allocs/op
```

And
```
hyperfine -i --warmup 1 'regal lint bundle' 'regal-new lint bundle'
Benchmark 1: regal lint bundle
  Time (mean ± σ):      3.232 s ±  0.046 s    [User: 22.535 s, System: 0.623 s]
  Range (min … max):    3.169 s …  3.296 s    10 runs

Benchmark 2: regal-new lint bundle
  Time (mean ± σ):      2.754 s ±  0.046 s    [User: 18.641 s, System: 0.472 s]
  Range (min … max):    2.689 s …  2.813 s    10 runs

Summary
  regal-new lint bundle ran
    1.17 ± 0.03 times faster than regal lint bundle
```

No @srenatus, I will not use benchstat! 🤣

Signed-off-by: Anders Eknert <[email protected]>
  • Loading branch information
anderseknert authored Dec 6, 2024
1 parent 8759f08 commit 0db5b5f
Show file tree
Hide file tree
Showing 3 changed files with 62 additions and 22 deletions.
7 changes: 5 additions & 2 deletions bundle/regal/ast/ast.rego
Original file line number Diff line number Diff line change
Expand Up @@ -347,8 +347,11 @@ all_function_names := object.keys(all_functions)

# METADATA
# description: set containing all negated expressions in input AST
negated_expressions[rule] contains value if {
some rule in input.rules
negated_expressions[rule_index] contains value if {
some i, rule in _rules

# converting to string until https://github.com/open-policy-agent/opa/issues/6736 is fixed
rule_index := sprintf("%d", [i])

walk(rule, [_, value])

Expand Down
71 changes: 53 additions & 18 deletions bundle/regal/ast/search.rego
Original file line number Diff line number Diff line change
Expand Up @@ -111,13 +111,6 @@ _find_vars(value, last) := {"term": find_term_vars(function_ret_args(fn_name, va
function_ret_in_args(fn_name, value)
}

_find_vars(value, last) := {"assign": _find_assign_vars(value)} if {
last == "terms"
value[0].type == "ref"
value[0].value[0].type == "var"
value[0].value[0].value == "assign"
}

# `=` isn't necessarily assignment, and only considering the variable on the
# left-hand side is equally dubious, but we'll treat `x = 1` as `x := 1` for
# the purpose of this function until we have a more robust way of dealing with
Expand All @@ -126,11 +119,9 @@ _find_vars(value, last) := {"assign": _find_assign_vars(value)} if {
last == "terms"
value[0].type == "ref"
value[0].value[0].type == "var"
value[0].value[0].value == "eq"
value[0].value[0].value in {"assign", "eq"}
}

_find_vars(value, _) := {"ref": find_ref_vars(value)} if value.type == "ref"

_find_vars(value, last) := {"somein": _find_some_in_decl_vars(value)} if {
last == "symbols"
value[0].type == "call"
Expand Down Expand Up @@ -167,11 +158,25 @@ _rule_index(rule) := sprintf("%d", [i]) if {
# traverses all nodes under provided node (using `walk`), and returns an array with
# all variables declared via assignment (:=), `some`, `every` and in comprehensions
# DEPRECATED: uses ast.found.vars instead
find_vars(node) := [var |
walk(node, [path, value])
find_vars(node) := array.concat(
[var |
walk(node, [path, value])

var := _find_vars(value, regal.last(path))[_][_]
]
last := regal.last(path)
last in {"terms", "symbols", "args"}

var := _find_vars(value, last)[_][_]
],
[var |
walk(node, [_, value])

value.type == "ref"

some x, var in value.value
x > 0
var.type == "var"
],
)

# hack to work around the different input models of linting vs. the lsp package.. we
# should probably consider something more robust
Expand All @@ -198,12 +203,31 @@ found.vars[rule_index][context] contains var if {

walk(rule, [path, value])

some context, vars in _find_vars(value, regal.last(path))
last := regal.last(path)
last in {"terms", "symbols", "args"}

some context, vars in _find_vars(value, last)
some var in vars
}

found.vars[rule_index].ref contains var if {
some i, rule in _rules

# converting to string until https://github.com/open-policy-agent/opa/issues/6736 is fixed
rule_index := sprintf("%d", [i])

walk(rule, [_, value])

value.type == "ref"

some x, var in value.value
x > 0
var.type == "var"
}

# METADATA
# description: all refs foundd in module
# description: all refs found in module
# scope: document
found.refs[rule_index] contains value if {
some i, rule in _rules

Expand All @@ -212,11 +236,22 @@ found.refs[rule_index] contains value if {

walk(rule, [_, value])

is_ref(value)
value.type == "ref"
}

found.refs[rule_index] contains value if {
some i, rule in _rules

# converting to string until https://github.com/open-policy-agent/opa/issues/6736 is fixed
rule_index := sprintf("%d", [i])

walk(rule, [_, value])

value[0].type == "ref"
}

# METADATA
# description: all symbols foundd in module
# description: all symbols found in module
found.symbols[rule_index] contains value.symbols if {
some i, rule in _rules

Expand Down
6 changes: 4 additions & 2 deletions bundle/regal/rules/bugs/impossible-not/impossible_not.rego
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ _multivalue_rules contains path if {
}

_negated_refs contains negated_ref if {
some rule, value
ast.negated_expressions[rule][value]
some rule_index, value
ast.negated_expressions[rule_index][value]

# if terms is an array, it's a function call, and most likely not "impossible"
is_object(value.terms)
Expand All @@ -40,6 +40,8 @@ _negated_refs contains negated_ref if {
path.type == "string"
}

rule := input.rules[to_number(rule_index)]

# ignore negated local vars
not ref[0].value in ast.function_arg_names(rule)
not ref[0].value in {var.value |
Expand Down

0 comments on commit 0db5b5f

Please sign in to comment.