risor-io / risor

Fast and flexible scripting for Go developers and DevOps.
https://risor.io
Apache License 2.0
581 stars 24 forks source link

feat: add threads module #155

Closed luisdavim closed 6 months ago

luisdavim commented 6 months ago

relates to #131 but it's inspired by Python's threading module.

luisdavim commented 6 months ago

This needs more work as I'm seeing some data races in the VM....

myzie commented 6 months ago

Thanks for prototyping and throwing out some ideas.

A key consideration in all this is that Risor's VM supports exactly one call stack, which means it can't be used as-is to run parallel execution paths.

Regarding how to move forward, I think we can explore both smaller solutions and big new capabilities. One small approach could be to have a builtin that supports concurrently calling other builtins with given args. The advantage here is that the concurrency is all in Go code.

While in a bigger new capability I suspect it may involve one or more of these:

If we could leverage an event bus like approach to communication, it could even lend itself to being agnostic of whether the separate executions are in the same OS process or distributed remotely.

luisdavim commented 6 months ago

One small approach could be to have a builtin that supports concurrently calling other builtins with given args.

That's what I was going for here but I guess this needs to be moved down the sack a bit and can't be done at the module level. At least if we want to support calling user defined functions, I think that's mostly where the issues I'm seeing are coming from, I'm wrapping them in a new builtin but that still needs to be done JIT and relies on a closure as I don't know what functions there are or if they are going to be called in a thread.

If I modify the test script to only use builtins in the threads, like this:

#!/usr/bin/env risor --

ts := [threads.new(time.sleep, 1)]
ts.append(threads.new(fetch, "http://ipinfo.io"))
ts.append(threads.new(print, "test"))

ts.each(func(t){
    t.start()
})

res := []
ts.each(func(t){
    res.append(t.wait())
})

res

I have no issues running:

go run -race -tags="aws k8s vault" ./cmd/risor examples/scripts/threads.risor

maybe another approach could be to, on a first pass, maybe in the compiler, convert all user defined functions into builtins but I'm not very familiar with either the compiler or the VM code so I don't know if that would work. It's also possible that adding some mutexes to the VM around the function invocation parts can be enough to get over these issues at least for an early implementation.

I may experiment a bit more with this in the weekend.

luisdavim commented 6 months ago

Did a quick test and a mutex on the VM could solve most problems, the main remaining issue I think is the vm.tmp array that holds a copy of the args, we could move that out of the VM struct and pass a slice around but that would probably have an impact on performance and memory usage so I'm not sure if it's worth it.

applejag commented 6 months ago

If adding mutexes in on function calls, how much gain do we actually get from running things in different goroutines?

Won't IO function calls also trigger the mutexes to lock?

So you're only gaining parallel execution on plain Risor code then.

luisdavim commented 6 months ago

If adding mutexes in on function calls, how much gain do we actually get from running things in different goroutines?

Won't IO function calls also trigger the mutexes to lock?

So you're only gaining parallel execution on plain Risor code then.

It depends on where the locks are if we lock the whole function call yeah, there's not much gain, you can still run stuff in the background but won't gain much in parallelization and calling a function from another function could lead to a lock.

luisdavim commented 6 months ago

This works in some use cases, like the one on the example script, but has too many caveats:

Some of these issues may be solvable with some refactoring in the VM and more carefully placed mutex locks instead of a blanket one on vm.callFuntion... I may run some more experiments around this in the future but I'm closing this one for now.