Run CompiledFunction in VM

[misiek08]

Basing on #330 I created PoC of running compiled functions. Please tell me what you think about this.

[geseq]

I'm not sure what use case this solves. You should be able to do the same by just having your script do the computation, set a global for input and then get the value from it after the computation. It provides for a cleaner separation between in-vm and out-of-vm computation.

[geseq]

I will close this for now, but please feel free to re-open this if you have a use case for this that isn't handled by what I mentioned above.

[misiek08]

Two cases for now:

1. Basic one - user/client provide script with predefined function names as handlers for specific actions in system.
2. More complex - user provide single script for multiple actions and can specify which function should be called for predefined and custom events in system.

Reusability of code make script-per-action bad solution. Having a "router"-function inside every script sounds bad too.

uGo has this func (vm *VM) RunCompiledFunction(f *CompiledFunction, globals Object, args ...Object,) (Object, error), but it's 2.8k vs 73 stars on GH, so Tengo looks safer for longer future. We don't wonna run from fork/patched version too :)

[geseq]

Why do you think a router sounds bad? I don’t necessarily mean a router per script, but you could have a single entrypoint with a router, and load your scripts from modules and call the function there, similar similar to the case of how http is generally handled in php/js based “frameworks”.

That said, I do see your point as well. Your implementation itself looks fine, but I’m concerned about potential issues that might be introduced if these CompiledFunctions were called concurrently.

@d5 thoughts?

[bjoerndemeyer]

I could certainly also use this. I wanted to provide a Tengo API with callbacks but was unable to do so without this fix.

[misiek08]

The router approach is introducing another in-memory structure access and function callback. Other than just performance - I don't see any bigger disadvantages, because I can provide my own module loader on Go side to side-load "client" scripts.
It just make it more complex and slightly slower.

[misiek08]

Do I need to write integration tests or some manual tests with highly concurrent code will be enough?

[geseq]

I think it’d be better to have tests written in rather than manual testing.

[misiek08]

I've added some proposition. It's all open for discuss, maybe we should extend Compiled instead of touching internals.

[geseq]

Thanks for the changes. I won't be able to look into this for a couple of weeks but will get back to you as soon as I can.

[Snyssfx]

.

UPD: Sorry: missclicked, wrong repo

[geseq]

Oops sorry, this is still on my list of TODOs. Just haven’t had the time to get to it yet.

[NoF0rte]

Just wanted to voice a current use case for this although it might be an outlier.

Currently, I am working on a project that, for simplicity sake, basically runs Tengo scripts but adds custom modules that can be imported. I created a module that implements some of the functionality of the cobra library to add command/flag functionality to the scripts. For cobra, you can set what code runs when a command is passed as an argument by setting the command struct's Run field which is a func type. In order to implement this functionality in a Tengo module, I used this PR's code to be able to run a function that is passed in from the Tengo script. It has been working like a charm, with no issues that I have seen so far.

[geseq]

Sorry this took some time.

There is a way to break things with this implementation. It comes to incorrect usage, but the current implementation doesn't prevent this incorrect usage.

try this test:

func TestRunCompiled(t *testing.T) {
	s := tengo.NewScript([]byte(`fnMap := {"fun1": func(a) { return a * 2 }}`))
	c, err := s.Run()
	require.NoError(t, err)

	cFn := c.Get("fnMap").Map()["fun1"].(*tengo.CompiledFunction)

	var wg sync.WaitGroup
	globals := make([]tengo.Object, tengo.GlobalsSize)
	vm := tengo.NewVM(c.Bytecode(), globals, -1) // everything is fine if this is inside the goroutine
	for i := 0; i < 1000; i++ {
		wg.Add(1)
		go func() {
			require.NotNil(t, vm)
			res, err := vm.RunCompiled(cFn, &tengo.Int{Value: 12})
			require.NoError(t, err)
			require.NotNil(t, res)
			require.Equal(t, res, &tengo.Int{Value: 24})

			wg.Done()
		}()
	}

	wg.Wait()
}

One thing that is not prevented right now is that for a given VM that has been created you shouldn't be able to call RunCompiled concurrently.

A possible fix would be to add a mutex that will lock around the RunCompiled so that the state of a given VM is not concurrently reused.

As long as RunCompiled can't be run concurrently for the same VM, this looks safe.

[geseq]

s.Compile() should do here

[kcsampson]

Yes, unfortunately this won't work because essentially I need the VM to be re-entrant (not mult-threaded). The implementation completely wipes out the stack of the virtual machine, which is no good if it's being recursively called.

My use case very basic in data processing. I want to write some user functions like C# LINQ-SQL's to work on collections of data: select(),selectMany(), join(),groupBy(). All these functions take closures/lambdas as arguments. In a scripting language, less imperative and more declaritive. The functions return things that are so-called "queryable". You could have something like:

Assume to arrays containing myCollection []A and type B myOtherCollection []B

x := From( myCollection ).Join( myOtherCollection, func( a , b ) true { return b.AId == a.AId }, func( a , b ){
return { "a": a, "b": b } ).Sort( func( o1, o2 ) true { return o1.a.name < o2.a.name } )

Here we join two collections on matching field, map into a map containing both objects, then sort by the "a" object's name.

[misiek08]

I wasn't here for a while, but if this issue still persists and no other implementation exist - I can fix the requested changes.

@kcsampson are you able to write example code that breaks with this changes so I can work on real example? To be honest I used those changes for a while, rewrote most code to parametrized Go functions and currently almost no traffic goes through tengo. Those were simple cases for access control.