avoid heap allocs in transform.Inspect and transform.InspectExpr#3348
avoid heap allocs in transform.Inspect and transform.InspectExpr#3348
transform.Inspect and transform.InspectExpr#3348Conversation
jycor
force-pushed
the
james/transform
branch
from
e4ad5f3 to
8056b95
Compare
zachmu
left a comment
zachmu
left a comment
There was a problem hiding this comment.
LGTM
Another thing to think about: we very frequently want to iterate over Children and it doesn't matter that they're a slice for this use case. We could change the signature of Children to be a seq.Iter and avoid all the allocs in these cases. Could probably use cursor to do most of this.
| return false | ||
| }) | ||
| if stop { | ||
| return false |
There was a problem hiding this comment.
Isn't this backwards? Don't you want to stop as soon as you find a foundMatchAgainst expr?
There was a problem hiding this comment.
This does stop once foundMatchAgainst expr is found. The logic is backwards for transform.Inspect vs transform.InspectExpr. I was initially going to swap the logic in this PR, but the changes quickly got unwieldy, so I left the TODOs for now.
| // InspectExpr performs a post-order traversal of the sql.Expression tree; | ||
| // First, `f` is called on `expr.Children()` and if stop = false, then InspectExpr is recursively called on node's | ||
| // children. | ||
| // TODO: this conflicts with transform.Inspect which performs a pre-order traversal and stops when cont = false. |
There was a problem hiding this comment.
This is actually really annoying. All the callback boolean returns in this package should mean the same thing, either continue or stop. Continue is probably better to standardize on.
2 participants
Changes:
transform.Inspectswitches onUnaryNodeandBinaryNodeto avoid heap allocations fromnode.Children()transform.InspectExprswitches onUnaryExpressionandBinaryExpressionto avoid heap allocations fromexpression.Children()UnaryExpressionimplementation fromunaryAggBaseand some functions that can return multiple children.Benchmarks: dolthub/dolt#10222 (comment)