Feature reuse result of common expr #393
Conversation
|
Benchmark results not very promising: https://github.com/antonmedv/expr/actions/runs/5658005304/job/15332440349?pr=393 |
| n.SetSubExpr(strconv.Quote(n.Value)) | ||
| } | ||
|
|
||
| func (c *compiler) analyzeCommonConstantNode(n *ast.ConstantNode) { |
There was a problem hiding this comment.
It will be better to just implement a toString() method for nodes.
There was a problem hiding this comment.
emm, you meaning String() func in golang?
There was a problem hiding this comment.
Yes
At the beginning, I also wanted to use the interface of String, but when I used it later, I found that I still needed an interface to set the expression for checking common sub-expr. because of the expression optimzation in package optimizer, I can't set it at parse expr. meanwhile, I found that there are function to set/get file.Location in ast.Node. so I added GetSubExpr/SetSubExpr similarly.
| @@ -21,6 +21,8 @@ type Config struct { | |||
| ConstFns map[string]reflect.Value | |||
| Visitors []ast.Visitor | |||
| Functions map[string]*builtin.Function | |||
|
|
|||
| AllowReuseCommon bool // allow cache common sub-expr computed result, aimed to reuse already computed result | |||
There was a problem hiding this comment.
For functions will make more sense a config like this:
expr.PureFunc("fib")There was a problem hiding this comment.
emm, you mean to add Option function like expr.Function?
There was a problem hiding this comment.
Similar to expr.ConstExpr
Yes, I already have added this functionexpr.AllowReuseCommon()in expr.go.
https://github.com/zhuliquan/expr/blob/f84c5a98f555a187a861bcc94218d80d0fa2c7ed/expr.go#L127
| @@ -21,6 +22,14 @@ func root(l *lexer) stateFn { | |||
| l.error("%v", err) | |||
| } | |||
| l.emitValue(String, str) | |||
| case r == '`': // raw string case | |||
There was a problem hiding this comment.
Better to split into two PRs.
There was a problem hiding this comment.
sorry, I can't to split into two prs, I have some projects in my company also uses raw_string and resue common two features in my fork branch. whether I propose another PR does not contain the raw string feature, or you merge the previous PR first and then deal with this PR
| if vm.stack == nil { | ||
| vm.stack = make([]interface{}, 0, 2) | ||
| vm.stack = make([]interface{}, 0, 64) |
There was a problem hiding this comment.
Why to increate default stack size?
There was a problem hiding this comment.
Why to increate default stack size?
which make it bigger can reduce times of extend capacity of stack in running very long expression (i.g. very long array expr), and I like number of 64, if you don't like it, I will recover to default 2
vm/vm.go
Outdated
|
|
||
| if vm.scopes != nil { | ||
| if vm.scopes == nil { | ||
| vm.scopes = make([]*Scope, 0, 64) |
There was a problem hiding this comment.
Not every expression uses scopes.
vm/vm.go
Outdated
|
|
||
| case OpNot: | ||
| v := vm.pop().(bool) | ||
| vm.push(!v) | ||
| vm.push(!(vm.pop().(bool))) |
There was a problem hiding this comment.
Additional variable makes code more readable.
|
This optimization is actually very interesting to do, but I think it should be done on the compile spec with AST transformation. But the first step will be to add the variables support in the language. So for example this expression: Can be changed into: |
fix: extract commonCache to temp variable in vm.go
it have been improvement about 25% according to benchmark report( case |
|
Setting AllowReuseCommon on or off is not meaningful. As both of them perform much worse for all expressions. This PR, in its current form, significantly slows downs expression executions. |
I don't agree it, the fact that |
I also agree this plan. but it means that expr must support variable assignment and variable declarations，variable type inference, whether it make expr more complex. |
|
Variable in expr already in plan #101, and I'm actually want to implement it soon. |
nice |
I find a tiny improvement for compiler, which use save result of common sub-expr and used it after.
for example:
we can reuse result of
veryHeavyFunc()I define a new parameter of Config called
AllowReuseCommon, if it's set true, we will reused result of common expr which has been evaluated before.I define three new opcodes for program compiler.