AST: Allow children of commutative nodes to be reordered cost-wise. #800
Conversation
apognu
force-pushed
the
feat/ast-reorder-weighted-ast-nodes
branch
from
aa29fa4 to
47f3847
Compare
| selfCost = attrs.Cost | ||
| } | ||
|
|
||
| for _, ch := range node.Children { |
There was a problem hiding this comment.
Also gotta do the same on name children (for keeping generality at least, not sure if the case can be seen currently)
There was a problem hiding this comment.
Aren't named children already order-agnostic? They're both stored in a map, which does not have a reproducible order, and I assume are used out-of-order?
Will look into it tomorrow.
There was a problem hiding this comment.
Yes they are, however for cost computation the named child nodes should also have their cost taken into account
| func (environment AstEvaluationEnvironment) WithoutCircuitBreaking() AstEvaluationEnvironment { | ||
| environment.disableCircuitBreaking = true | ||
|
|
||
| return environment | ||
| } | ||
|
|
||
| func (environment AstEvaluationEnvironment) WithoutCostOptimizations() AstEvaluationEnvironment { |
There was a problem hiding this comment.
Just checking my understanding here: this is necessary so we don't have to reorder error results that we return to the frontend, correct?
There was a problem hiding this comment.
The Without* methods are mostly for testing (when you're testing one of the two optimizations, so the other one does not bother the test), and for validation of scenario (so optimizations do not change the given scenario).
There was a problem hiding this comment.
Now that I think about it, was your comment on the right line?
If it was about the whole re-ordering / de-re-ordering instead, yes, it is done to ensure the tree results are sent in the order the frontend expects it to be, since it relies on that specific structure to match results with their node.
That would not be necessary if every node had a unique ID, but since that is not the case, we need this.
models/ast/ast_node_evaluation.go
Outdated
| @@ -7,6 +7,13 @@ import ( | |||
| ) | |||
|
|
|||
| type NodeEvaluation struct { | |||
| // Index of the initial node in the AST tree, used to reorder the results as they were. | |||
There was a problem hiding this comment.
just to clarify the comment: this is the index in the initial array of children, correct ? not a tree-global index of nodes
There was a problem hiding this comment.
Indeed, this is the original index at that level of the tree. I will clarify the comment.
|
This could benefit, before merging, of adding some new costs for other functions accessing the database. |
apognu
force-pushed
the
feat/ast-boolean-lazy-evaluation
branch
from
d0d3c04 to
d78abc6
Compare
This adds several things: * Commutative functions are marked as such. * AST functions now have an associated cost depending on their computational complexity. * Evaluation will now reorder the children of a commutative node to execute the cheapest first. This would have the benefit, thanks to the lazy evaluation of AST nodes (circuit breaking), to improve overall performance for those scenarii.
apognu
force-pushed
the
feat/ast-reorder-weighted-ast-nodes
branch
from
bad8dd4 to
2726621
Compare
This adds several things:
This would have the benefit, thanks to the lazy evaluation of AST nodes (circuit breaking), to improve overall performance for those scenarii.