ast+topdown: implement early exit semantics for complete document and function rules with one (ground) value #3898

srenatus · 2021-10-18T12:36:45Z

If the following conditions hold for a set of rules returned by the indexer,
it will set EarlyExit: true, and change how the complete virtual doc is
evaluated:

all rule head values are ground
all rule head values match

This implies that some cases where early exit would be possible will not be
covered:

p = x {
  x := true
  input.foo == "bar"
}

p = x {
  x := true
  input.baz == "quz"
}

To indicate that "early exit" is possible, the indexer result message is
amended. Also, the "Exit" trace event will have a message of "early" when
"early exit" actually happens in eval:

$ echo '{"x":"x", "y":"y"}' | opa eval -I -fpretty --explain=full -d r.rego data.r.r
query:1       Enter data.r.r = _
query:1       | Eval data.r.r = _
query:1       | Index data.r.r (matched 2 rules, early exit)
r.rego:11     | Enter data.r.r
r.rego:12     | | Eval input.y = "y"
r.rego:11     | | Exit data.r.r
query:1       | Exit data.r.r = _
query:1       Redo data.r.r = _
query:1       | Redo data.r.r = _
r.rego:11     | Redo data.r.r
r.rego:12     | | Redo input.y = "y"
r.rego:11     | Exit data.r.r early

With r.rego as

package r
r {
  input.x = "x"
}
r = 2 {
  input.z = "z"
}
r {
  input.y = "y"
}

srenatus · 2021-10-19T12:03:43Z

Currently, the failures in CI have to do with trace events not firing:

package test
p { trace("P1") }
p { false }
p { trace("P2") }

Fair enough -- P2 will be gone with this, since there's no need to evaluate the third p rule.

srenatus · 2021-10-19T13:24:39Z

Not yet dealing with sets and membership shortcuts at all btw

srenatus · 2021-10-19T19:10:47Z

TODO:

test eval code path via captured trace events + outputs
figure out if we should return "early-exit: true" if applicable in the (ruleIndex).AllRules(...) case (e.indexing == false in eval.go)
document this change in eval behaviour, if there's a good spot
early-exit in one rule body (iteration)

topdown/eval.go

docs/content/policy-performance.md

topdown/eval.go

topdown/topdown_test.go

topdown/eval.go

tsandall

LGTM. Feel free to merge after squashing.

tsandall · 2021-11-10T17:25:56Z

topdown/eval.go

@@ -2642,7 +2644,10 @@ func (e evalVirtualComplete) evalValueRule(iter unifyIterator, rule *ast.Rule, p

 		err := e.evalTerm(iter, term, termbindings)
 		if err != nil {
-			return err
+			ok := errors.Is(err, errEarlyExit)
+			if !ok || e.ir.EarlyExit {


For my own understanding... this ensures that topdown does not (incorrectly) early-exit on "q" when the early-exit signal from the closure of "p" is received here. In other words, the early-exit signal from "p" is caught here and dropped.

Again and again I'm getting unsure about this spoiling EE for valid cases, in more involved situations... My gut feeling is that "the last" eval of q should pass the EE error along. I'll have another look into this, maybe add another trace testcase.

srenatus · 2021-11-11T12:44:17Z

package test
p {
  q.a
}
xs = {1, 2}
q["a"] = xs[_]

☝️ we'll next extra cases for partials, too.

srenatus · 2021-11-11T19:54:42Z

🤔 back to draft mode. I think there must be some way to approach this in a more principled way than littering eval with if !errors.Is(err, errEarlyExit) { return err } for the virtuals that don't support EE...

srenatus · 2021-11-12T17:20:39Z

I think we'll need to rub some more of what's in the last commit over the evalVirtual cases. I'll approach the problem with more test cases next week.

ast/index_test.go

tsandall

One comment about use of errors.As. Otherwise, once we get the sort-on-insert change in for sets and squash these commits, this PR looks good to me.

topdown/eval.go

…tions If the following conditions hold for a set of rules returned by the indexer, it will set EarlyExit: true, and change how the complete virtual doc or function is evaluated: - all rule head values are ground - all rule head values match This implies that some cases where early exit would be possible will not be covered: p = x { x := true input.foo == "bar" } p = x { x := true input.baz == "quz" } To indicate that "early exit" is possible, the indexer result message is amended. Also, the "Exit" trace event will have a message of "early" when "early exit" actually happens in eval: $ echo '{"x":"x", "y":"y"}' | opa eval -I -fpretty --explain=full -d r.rego data.r.r query:1 Enter data.r.r = _ query:1 | Eval data.r.r = _ query:1 | Index data.r.r (matched 2 rules, early exit) r.rego:11 | Enter data.r.r r.rego:12 | | Eval input.y = "y" r.rego:11 | | Exit data.r.r query:1 | Exit data.r.r = _ query:1 Redo data.r.r = _ query:1 | Redo data.r.r = _ r.rego:11 | Redo data.r.r r.rego:12 | | Redo input.y = "y" r.rego:11 | Exit data.r.r early With `r.rego` as package r r { input.x = "x" } r = 2 { input.z = "z" } r { input.y = "y" } This is done in in a way such that early-exit will abort array/set/object iterations on data: r { data.i[_] = "one" data.j[_] = "four" } f(x, y) { data.i[_] = x data.j[_] = y } Complete rules (r) and functions (f) that iterate over sets, arrays, and objects from either data (evalTree) or a term that's returned by some other rule etc (evalTerm). The CLI and golang packages expose ways to disable 'early-exit': This is in line with how indexing can be disabled. It's supposed to be used as a debugging measure, so it's only exposed as a CLI flag to `opa eval`. Co-authored-by: Torin Sandall <[email protected]> Signed-off-by: Stephan Renatus <[email protected]>

srenatus · 2021-11-18T09:37:21Z

Will merge when green

…tions (open-policy-agent#3898) If the following conditions hold for a set of rules returned by the indexer, it will set EarlyExit: true, and change how the complete virtual doc or function is evaluated: - all rule head values are ground - all rule head values match This implies that some cases where early exit would be possible will not be covered: p = x { x := true input.foo == "bar" } p = x { x := true input.baz == "quz" } To indicate that "early exit" is possible, the indexer result message is amended. Also, the "Exit" trace event will have a message of "early" when "early exit" actually happens in eval: $ echo '{"x":"x", "y":"y"}' | opa eval -I -fpretty --explain=full -d r.rego data.r.r query:1 Enter data.r.r = _ query:1 | Eval data.r.r = _ query:1 | Index data.r.r (matched 2 rules, early exit) r.rego:11 | Enter data.r.r r.rego:12 | | Eval input.y = "y" r.rego:11 | | Exit data.r.r query:1 | Exit data.r.r = _ query:1 Redo data.r.r = _ query:1 | Redo data.r.r = _ r.rego:11 | Redo data.r.r r.rego:12 | | Redo input.y = "y" r.rego:11 | Exit data.r.r early With `r.rego` as package r r { input.x = "x" } r = 2 { input.z = "z" } r { input.y = "y" } This is done in in a way such that early-exit will abort array/set/object iterations on data: r { data.i[_] = "one" data.j[_] = "four" } f(x, y) { data.i[_] = x data.j[_] = y } Complete rules (r) and functions (f) that iterate over sets, arrays, and objects from either data (evalTree) or a term that's returned by some other rule etc (evalTerm). The CLI and golang packages expose ways to disable 'early-exit': This is in line with how indexing can be disabled. It's supposed to be used as a debugging measure, so it's only exposed as a CLI flag to `opa eval`. Co-authored-by: Torin Sandall <[email protected]> Signed-off-by: Stephan Renatus <[email protected]>

srenatus force-pushed the sr/ast/index-and-early-exit branch 2 times, most recently from c821ce9 to d1eab2f Compare October 19, 2021 09:26

srenatus mentioned this pull request Oct 19, 2021

Improve evaluator to detect and perform early-exit if possible #2092

Closed

srenatus force-pushed the sr/ast/index-and-early-exit branch from bab038a to 51e1cbc Compare October 19, 2021 19:05

srenatus commented Oct 20, 2021

View reviewed changes

topdown/eval.go Outdated Show resolved Hide resolved

srenatus force-pushed the sr/ast/index-and-early-exit branch 5 times, most recently from 8e180c5 to 0b9c165 Compare October 22, 2021 12:55

srenatus changed the title ~~ast/index: start on early-exit eval~~ ast+topdown: implement early exit semantics for some cases Oct 22, 2021

srenatus marked this pull request as ready for review October 22, 2021 12:56

srenatus force-pushed the sr/ast/index-and-early-exit branch from 0b9c165 to 81c7ec7 Compare October 22, 2021 16:27

srenatus commented Oct 22, 2021

View reviewed changes

topdown/eval.go Show resolved Hide resolved

srenatus force-pushed the sr/ast/index-and-early-exit branch 2 times, most recently from 1bc1dfa to bf87dd0 Compare October 22, 2021 19:57

srenatus marked this pull request as draft October 24, 2021 06:07

srenatus force-pushed the sr/ast/index-and-early-exit branch 5 times, most recently from 0d9beb9 to 5862a49 Compare October 25, 2021 09:17

srenatus changed the title ~~ast+topdown: implement early exit semantics for some cases~~ ast+topdown: implement early exit semantics for complete document and function rules with one (ground) value Oct 25, 2021

srenatus marked this pull request as ready for review October 25, 2021 09:20

tsandall reviewed Nov 2, 2021

View reviewed changes

docs/content/policy-performance.md Outdated Show resolved Hide resolved

topdown/eval.go Outdated Show resolved Hide resolved

topdown/topdown_test.go Outdated Show resolved Hide resolved

tsandall reviewed Nov 2, 2021

View reviewed changes

topdown/eval.go Outdated Show resolved Hide resolved

srenatus force-pushed the sr/ast/index-and-early-exit branch from e148dc5 to 71edef8 Compare November 3, 2021 10:19

srenatus force-pushed the sr/ast/index-and-early-exit branch 2 times, most recently from 2bd939c to 87243a8 Compare November 9, 2021 10:12

tsandall previously approved these changes Nov 9, 2021

View reviewed changes

srenatus dismissed tsandall’s stale review via 89dc4ef November 9, 2021 20:14

srenatus force-pushed the sr/ast/index-and-early-exit branch 3 times, most recently from b0ce662 to 5e488cf Compare November 10, 2021 16:54

tsandall reviewed Nov 10, 2021

View reviewed changes

srenatus marked this pull request as draft November 11, 2021 12:43

srenatus force-pushed the sr/ast/index-and-early-exit branch 2 times, most recently from ba33207 to 96a2696 Compare November 16, 2021 09:51

srenatus commented Nov 16, 2021

View reviewed changes

ast/index_test.go Show resolved Hide resolved

srenatus marked this pull request as ready for review November 16, 2021 13:32

tsandall reviewed Nov 17, 2021

View reviewed changes

topdown/eval.go Outdated Show resolved Hide resolved

srenatus force-pushed the sr/ast/index-and-early-exit branch from 077a009 to 6ea4732 Compare November 18, 2021 09:18

srenatus force-pushed the sr/ast/index-and-early-exit branch from 6ea4732 to fc0c94e Compare November 18, 2021 09:35

srenatus merged commit 2f6bf3f into open-policy-agent:main Nov 18, 2021

srenatus mentioned this pull request Nov 18, 2021

topdown/EE: early-exit in partial sets membership queries #4035

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ast+topdown: implement early exit semantics for complete document and function rules with one (ground) value #3898

ast+topdown: implement early exit semantics for complete document and function rules with one (ground) value #3898

srenatus commented Oct 18, 2021 •

edited

Loading

srenatus commented Oct 19, 2021

srenatus commented Oct 19, 2021

srenatus commented Oct 19, 2021 •

edited

Loading

tsandall left a comment

tsandall Nov 10, 2021

srenatus Nov 10, 2021

srenatus commented Nov 11, 2021

srenatus commented Nov 11, 2021

srenatus commented Nov 12, 2021

tsandall left a comment

srenatus commented Nov 18, 2021

ast+topdown: implement early exit semantics for complete document and function rules with one (ground) value #3898

ast+topdown: implement early exit semantics for complete document and function rules with one (ground) value #3898

Conversation

srenatus commented Oct 18, 2021 • edited Loading

srenatus commented Oct 19, 2021

srenatus commented Oct 19, 2021

srenatus commented Oct 19, 2021 • edited Loading

tsandall left a comment

Choose a reason for hiding this comment

tsandall Nov 10, 2021

Choose a reason for hiding this comment

srenatus Nov 10, 2021

Choose a reason for hiding this comment

srenatus commented Nov 11, 2021

srenatus commented Nov 11, 2021

srenatus commented Nov 12, 2021

tsandall left a comment

Choose a reason for hiding this comment

srenatus commented Nov 18, 2021

srenatus commented Oct 18, 2021 •

edited

Loading

srenatus commented Oct 19, 2021 •

edited

Loading