feat: Vectorize addition in map

[Marwes]

Closes #4585

Based on #4636

[jsternberg]

Just some comments about how the memory allocator is passed through the compiler. I'd like to understand more the challenges around passing it through as I share the same concern that passing it through the context is non-ideal. After learning a bit more, I'll likely just change to approve if we can't determine a better way.

[jsternberg]

I think this is what you were mentioning but I can't remember what you said. Do you think we can change the compiler to take an extra parameter for the memory allocator? I'd generally prefer to change the function signature than to inject the memory allocator through the context.

[Marwes]

I'd agree and I tried that, but that would force me to change every call into the evaluator to take that extra parameter (or rather, a new EvaluatorState struct which would make it easier to add new parameters/state later on). However even the interpreter ends up calling into the evaluator through some builtins so there are just far to much code that would be affected by such a change as part of this PR.

type EvaluatorState struct {
    ctx context.Context
   scope Scope
   mem memory.Allocator
}

[jsternberg]

I figured you could have the compiler initialize it as an attribute. So binaryVectorEvaluator would be like:

type binaryVectorEvaluator struct {
    // other attributes
    mem memory.Allocator
}

It would then use that during evaluation. The compiler would initialize the evaluator with the memory allocator.

[Marwes]

I tried that, but it doesn't really change anything as then the evaluator compiler needs access to the allocator and the only way to provide that is by storing it in the Context.

[jsternberg]

I think you can do one of these two things:

1. Add an extra parameter to the compile function to take in the allocator. Then change the call sites to include it. It looks like the JetBrains IDE can automate most of this.
2. Change the compile function to be a method on a type and have that type include a reference to the memory allocator such as this:

type compiler struct {
    mem memory.Allocator
}

func (c *compiler) compile(...) (...) {
    c.mem.Allocate(...)
}

I think either of these two would be better than the context. The context is very error prone for this type of thing.

[Marwes]

Yes, and I have tried that but then I ran into callsites like this

flux/stdlib/experimental/array/array.go

Line 152 in 01dcde3

f, err := compiler.Compile(compiler.ToScope(fn.Scope), fn.Fn, inputType)

where the only way to pass in the allocator is via the context again

flux/stdlib/experimental/array/array.go

Line 124 in 01dcde3

func(ctx context.Context, args values.Object) (values.Value, error) {

.

So it doesn't fundamentally change anything, we still need to store the allocator in the context.

I do think we should refactor this such that we can pass the allocator (and any other state we might need) in a type safe way, however it would force changes throughout almost the entire go project since most functions that take a Context needs to be altered to pass a bespoke struct instead (though that struct would have a field holding the Context).

If we do want that refactor I can do that before or after this PR depending on what seems best.

[jsternberg]

Ok, I didn't realize that some functions needed access. Let me take a quick look again and then I can hit approve.

[jsternberg]

I took a look and if we're going to inject this into the context I think this isn't the proper location.

First, the dependency interface isn't really meant for this. It's much more straightforward to use the context directly and not with a struct and a name like RuntimeDependencies.

Second, we'll need to add this to the context in lang.Program probably. We need to make sure that the allocator is available in the context for both the interpreter and the compiler. The compiler generally gets run inside of the execution context inside of the executor, but the interpreter runs outside of that. Fortunately for us, they're close enough that they seem to use the same allocator.

flux/lang/compiler.go

Line 474 in 4996f84

func (p *AstProgram) Start(ctx context.Context, alloc *memory.Allocator) (flux.Query, error) {

It seems to me like we should add the logic inside of the memory package instead so that the interpreter, the compiler, and any functions that use it have easy access.

Generally, the Go standard for using context is like this:

type key int

const allocatorKey key = iota

func WithAllocator(ctx context.Context, mem Allocator) context.Context {
    return context.WithValue(ctx, allocatorKey, mem)
}

func GetAllocator(ctx context.Context) Allocator {
    mem, ok := ctx.Value(allocatorKey).(Allocator)
    if !ok {
        return DefaultAllocator
    }
    return mem
}

[Marwes]

Done. I was sure I had added the allocator in Start but apparently I hadn't or I removed it for some reason...

[jsternberg]

Should this just be named "Vectorize"? Since the rust code is agnostic for where it's vectorized.

[Marwes]

Maybe, but the actual feature is called vectorizedMap so I felt it was better to be consistent with that.

[sanderson]

Looks good from the docs side 👍🏻