Skip to content
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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

moisespsena
Copy link

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:

-------------------------------------
fibonacci(35)
-------------------------------------
Result:  9227465
Go:      50.13513ms
Parser:  14.368µs
Compile: 71.829µs
VM:      2.694701467s
-------------------------------------
fibonacci(35) (tail-call #1)
-------------------------------------
Result:  9227465
Go:      67.91345ms
Parser:  18.285µs
Compile: 68.48µs
VM:      2.776874189s
-------------------------------------
fibonacci(35) (tail-call #2)
-------------------------------------
Result:  9227465
Go:      236ns
Parser:  16.982µs
Compile: 55.363µs
VM:      18.95µs

Benchmark after my implementations:

-------------------------------------
fibonacci(35)
-------------------------------------
Result:  9227465
Go:      50.351214ms
Parser:  14.591µs
Compile: 71.252µs
VM:      14.926579001s
-------------------------------------
fibonacci(35) (tail-call #1)
-------------------------------------
Result:  9227465
Go:      74.248173ms
Parser:  19.069µs
Compile: 85.272µs
VM:      19.496196464s
-------------------------------------
fibonacci(35) (tail-call #2)
-------------------------------------
Result:  9227465
Go:      234ns
Parser:  17.122µs
Compile: 56.115µs
VM:      33.158µs

@moisespsena moisespsena changed the title Function kwargs feat: function/closure with keyword arguments Feb 18, 2022
Copy link
Collaborator

@geseq geseq left a 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.

Comment on lines +589 to +591
newArgs := make([]Object, numArgs+len(arr.Value))
copy(newArgs, args)
copy(newArgs[numArgs:], arr.Value)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

Copy link
Collaborator

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])
Copy link
Collaborator

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
Comment on lines 564 to 565
args []Object
kwargs, varKwargs map[string]Object
Copy link
Collaborator

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

Copy link
Collaborator

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
Comment on lines 631 to 637
calleeData := &ImmutableMap{
Value: map[string]Object{
"fn": callee,
"args": &ImmutableArray{Value: args},
"kwargs": &ImmutableMap{Value: kwargs},
},
}
Copy link
Collaborator

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

@geseq
Copy link
Collaborator

geseq commented Feb 19, 2022

Hopefully these comments will help with a good starting point to optimize the runtime

@moisespsena
Copy link
Author

Hello!
Current bachmark results after your comments:

-------------------------------------
fibonacci(35)
-------------------------------------
Result:  9227465
Go:      50.01228ms
Parser:  14.9µs
Compile: 69.707µs
VM:      4.527052515s
-------------------------------------
fibonacci(35) (tail-call #1)
-------------------------------------
Result:  9227465
Go:      68.14745ms
Parser:  21.051µs
Compile: 72.122µs
VM:      5.220183077s
-------------------------------------
fibonacci(35) (tail-call #2)
-------------------------------------
Result:  9227465
Go:      230ns
Parser:  16.992µs
Compile: 55.53µs
VM:      16.023µs

What do you think?

@geseq
Copy link
Collaborator

geseq commented Feb 24, 2022

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
Copy link
Author

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
)

Copy link
Collaborator

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

Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants