-
Notifications
You must be signed in to change notification settings - Fork 303
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: function/closure with keyword arguments #371
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general you'll have to reduce the amount of type inference in your implementation. I suspect it's also playing a big part in the performance hit you're seeing.
newArgs := make([]Object, numArgs+len(arr.Value)) | ||
copy(newArgs, args) | ||
copy(newArgs[numArgs:], arr.Value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(below)
vm.go
Outdated
v.sp-- | ||
switch arr := v.stack[v.sp].(type) { | ||
for i := 0; i < numArgs; i++ { | ||
args = append(args, v.stack[start+i]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably playing a major part. Don't allocate new memory if you can work with existing. If you absolutely need this one create it with the size you need.
vm.go
Outdated
args []Object | ||
kwargs, varKwargs map[string]Object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try to refactor to get rid of these
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or allocate with the required sizes
vm.go
Outdated
calleeData := &ImmutableMap{ | ||
Value: map[string]Object{ | ||
"fn": callee, | ||
"args": &ImmutableArray{Value: args}, | ||
"kwargs": &ImmutableMap{Value: kwargs}, | ||
}, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also try to see if you can avoid recreating this and update the existing one instead
Hopefully these comments will help with a good starting point to optimize the runtime |
Hello!
What do you think? |
Much better but still quite slow. From a cursory glance not sure if anything else could be improved. Perhaps try profiling the benchmark code to see what the pain points are? |
vm.go
Outdated
hasVarArgs = int(v.curInsts[v.ip+2]) | ||
numKws = int(v.curInsts[v.ip+3]) | ||
hasVarKw = int(v.curInsts[v.ip+4]) | ||
kwargs, varKwargs map[string]Object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that the declarations of the variables influence the performance, but are needed. I do not know how to improve this.
var ( // ...
kwargs, varKwargs map[string]Object
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you able to allocate it with the required capacity and then manipulate the contents as necessary? that might help
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the variable "Varkwargs" from the main scope, but I had no change in the benchmark.
I defined a capacity for the "kwargs" variable, but it worsened.
Still, I can not find a solution.
Hello!
I implement kwargs/keyword arguments for functions and closures. But I'm not very satisfied with the VM performance difference.
Could anyone help me with that?
Original benchmark:
Benchmark after my implementations: