Closures Continued

[DuncanUszkay1]

Remaking the PR using the master branch that is being used to publish the fork. Plus the old PR was getting a little too long, with most of its contents being outdated.

The plan for closure implementation is currently as follows:

High level

Function objects are now represented as pointers, not indexes. These pointers point to in-memory structs with the following layout:

{
  u_int32:
  <closed local type>, //One per closed local
  ...,
  <closed local type>
}

To call a function object, we do the following:

• Extract the function index, which is always the first usize in the struct the function pointer points to
• Call that function index indirectly, with the function pointer as the first argument, followed by normal arguments

Issues with Interop

Previously, host interop was trivial. We would return the index over the boundary, and then you could call the function at that index from the host. It's not longer that simple- function objects have state, and are represented by pointers. We need to provide a way for hosts to call function pointers. My suggestion for this approach is to use a two-step method which mirrors how we call these function pointers interally:

• First you call a builtin, always exported function which loads the function index from the pointer for you (a trivial load), returning a function index
• Call that function index with the function pointer as the first argument, followed by your normal arguments

Details (summary of the code)

Anonymous functions are all given an extra argument to potentially hold closure context.

When compiling access to a closed over local:

• We assign that local an offset
• We add that local to the type.closedLocals field
• We return a module load from the closure context argument (always $0, or the first argument) at the generated offset

When we finish compiling an anonymous function:

• We allocate some memory to hold the closure context struct that will fit the function index and any closed over locals
• The function index we received as a result of compiling the anonymous function is always the first field in that struct
• We return a pointer to that struct with a special type as the result of the anonymous function statement, which I'm calling a function pointer (since it's a pointer to a function)

When a function pointer is called in its original scope:

• We copy in the values of the local scope into the closure's struct (Note that we currently require that it is called this way only within the local scope due to a bug, see limitations)
• We load the closure context argument, followed by the rest of the normal arguments
• We call the function index

When the closure leaves scope or is otherwise coerced into a function type (a type represented only by a signature):

• We copy in the values of the local scope into the struct one last time
• We return the pointer

When a function pointer is called out of scope:

• Same process as being called in scope, except we don't copy in local values first (since we don't have access to them), it'll just use whatever values were loaded into the function struct when we left scope.

Garbage Collection

Function pointers are no different from other kinds of pointers, so they also use retain and release calls.

Limitations

In the interest of time, I'm leaving in some limitations intentionally. They should be represented in the tests closure-limitations and closure-limitations-runtime. Here's a list for reference:

• Directization: We want to be able to change indirect calls to direct calls, but always having to load the function pointer from linear memory prevents that. The current proposed solution for this is to provide a table which maps pointers to function indexes, and have binaryen substitute loads to those pointers with their function indexes. This is possible since the function indexes living in memory are static.
• writing to a closed value
• creating a closure within an anonymous function
• returning a closure from an exported function
• Calling a closure in the same scope it was defined in within a function, i.e.:

class Foo {
  field: i32;
}
var compClosuresSameScope = (): void => {
  var foo: Foo = {
    field: 8
  }
  var withFoo1 = (): void => {
    foo.field += 1;
  }
  var withFoo2 = (): void => {
    withFoo1();
  }
}
- closing over a `const` variable

[dcodeIO]

Regarding the const local, does it have a relatively simple initializer, like an integer literal? Asking because in very simple cases, the compiler tries to inline the value right away, so there won't be an actual (local) attached to it. If that's what's happening there, that'd actually be a good thing, because it doesn't require space in the closure context but can be inlined within the closure as well. local.is(CommonFlags.INLINED) -> compiler.compileInlineConstant.

[DuncanUszkay1]

We should also try out a version of this PR in which we don't use a complex function pointer scheme, and instead make all anonymous functions pointers, to compare bloat/performance

Comparing benefits of the two approaches:

Function Pointer Scheme (bit shifted MSB):

• Only allocs if the anonymous function is a closure
• Can directize indirect calls in binaryen

Pointer scheme (all anon functions are pointers to contexts):

• Removes a lot of runtime if logic to differentiate between function indexes and bit shifted pointers
• We don't have to change retain/release, since they're just references
• PR becomes considerably simpler

[MaxGraey]

Or following our philosophy you could use current approach for speed (like -O2 -O3 flags) and second approach for size (shrinkLevel >= 1). But it will be even more complex.

[jtenner]

@MaxGraey what about approaches for software that uses neither?

[MaxGraey]

Another solution. Just leave all as is but for anonym functions which comes from host (exported method) always use (expect) closure object and add according helper method which wrap any callback to Closure obj for loader. wdyt?

[DuncanUszkay1]

Or following our philosophy you could use current approach for speed (like -O2 -O3 flags) and second approach for size (shrinkLevel >= 1). But it's will be even more complex.

Interesting, so we'd use the optimization levels before we start optimizing? That makes sense. I suppose it invalidates the PR simplicity benefit though since having both approaches living in the source is going to be a ton of code- may I suggest we ship one approach and then additively add other approaches attached to optimization flags?

[DuncanUszkay1]

Another solution. Just leave all as is but for anonym functions which comes from host (exported method) always use (expect) closure object and add according helper method which wrap any callback to Closure obj for loader. wdyt?

Not sure I understand what you mean here- what issue does this resolve?

[dcodeIO]

I'd also favor an approach that doesn't involve complicating this more than necessary. Mostly concerned that having multiple variants of deeply rooted language features like this may (and probably will) become a nightmare to maintain. From a simplicity standpoint, a dummy memory segment for every function reference makes sense (we discussed this earlier but then there was directize), and I guess we might be able to tackle the directize problem on the Binaryen side. For instance, we can guarantee that these memory segments never change (as long as a function is not a closure), so perhaps a custom directize pass specifically for AS recognizing the invoke pattern and translating non-closure function memory addresses to their respective function indexes can help.

[DuncanUszkay1]

Seems like next steps then are:

• Create a branch which removes the MSB function index scheme
• Assign dummy memory segments for anonymous, non-closure functions to put their context in
• Figure out how to directize these calls in binaryen using the fact that these are constant segments in memory which cannot be altered

[MaxGraey]

Binaryen can't inline reads from static segment unfortunately.

[DuncanUszkay1]

Binaryen can't inline reads from static segment unfortunately.

Is that a permanent limitation or just something that hasn't been implemented?

[MaxGraey]

I'm wondering could we analyse call sites statically (may in two phases) which sites never call with closures and generate simple inline calls for function calls? In other case fallback to Closure objects. So do pre-optimizations instead try delegate this to binaryen

[DuncanUszkay1]

I'm wondering could we analyse call sites statically (may in two phases) which sites never call with closures and generate simple inline calls for function calls? In other case fallback to Closure objects. So do pre-optimizations instead try delegate this to binaryen

Sounds like that'd be tricky, and then when anonymous functions are part of a library interface like as-pect it wouldn't help

[MaxGraey]

Is that a permanent limitation or just something that hasn't been implemented?

It really hard for binaryen due to we haven't immutable for mem segments like globals have

[MaxGraey]

Sounds like that'd be tricky, and then when anonymous functions are part of a library interface like as-pect it wouldn't help

Yes for cross module boundary functions we should always expect closures as more general scenario.

[dcodeIO]

Perhaps to elaborate on the idea of a custom directize pass a bit:

Currently, Binaryen optimizes function indexes like other values, potentially becoming constant. If it sees a call_indirect with a constant index operand, it looks up the function in the table and makes it a direct call.

In the suggested case, it would not optimize function indexes but pointers into memory, again like normal values, with the pointers to all function dummies being known on our end (only non-closures make sense, though). Now, if we give Binaryen a map of memory addresses to function indexes, when it sees our invoke pattern, it can directize the entire pattern by looking up the function index before looking up the function in the table.

[DuncanUszkay1]

In the suggested case, it would not optimize function indexes but pointers into memory, again like normal values, with the pointers to all function dummies being known on our end (only non-closures make sense, though). Now, if we give Binaryen a map of memory addresses to function indexes, when it sees our invoke pattern, it can directize the entire pattern by looking up the function index before looking up the function in the table.

So it's a pointer to memory, but what's at the memory address is irrelevant because we're actually just using it to perform a lookup in a table of anonymous, non-closure functions? Sounds like it could work.

If I understand correctly the process would then be like this:

• We're calling a function pointer
• We load the function reference from memory as though it were a closure
• Binaryen compares a provided table of pointers to function indexes, substituting the loads above for static indexes wherever the pointer is both known and present in the table(effectively implementing a sort of static memory in a table)
• We call the function index
• Binaryen can directize the above call when we know the pointer points to an anonymous function, since the load has been replaced by a static value from the pointer to function index table

Is that right?

[willemneal]

Finally got around to testing things out. Everything looks great so far!

I found one simple case that resulted in a memory leak.

function addX(x: i32): (_: i32) => i32 {
  return (y) => x + y;
}

let addOne = addX(1);

assert(addOne(41) == 42);

On Friday I hope to really dig into this. My initial thoughts with the design is with the copying in of the context into local vars, the local var reads could be become field reads on the struct.

[DuncanUszkay1]

Oh interesting- did you see what our next steps were in the discussion of the PR? Do you agree with the direction we're going in?

[DuncanUszkay1]

Also @dcodeIO looks like you're working on it now based on the project board? You guys should sync up if you're both working on it so you don't butt heads

[DuncanUszkay1]

Tried applying some of the planned changes- it causes some problems with interop.

In the current version, closures are prepended with an extra closure argument, and anonymous functions aren't. If we remove differentiation, anonymous functions and closures both get the extra closure argument. This means that there's no good way to call those anonymous functions in JS land. And to make matters worse we also have to get rid of our abort when trying to return a closure, since we no longer know if what we're returning is a closure.

However, I think that this just means that we need to pull the bandaid off here and implement something for calling closures from JS land. It was inevitable that we were going to have to get rid of passing function indexes over the module boundary anyway. I haven't put a ton of thought into this yet but my first guess for implementation would be generating a 'call' function for every anonymous function signature which takes a closure context pointer and returns whatever that function returns. Probably will want some sugar in the loader to make that a little more ergonomic as well.

[dcodeIO]

I remember we had a global holding the closure context initially, but there have been some problems with this iirc. However, perhaps that's still something that might work on the boundary? Let's say the actual closure function has a prepended context argument

function theClosure(ctx, ...args)

then we can export

function theClosure$export(...args) {
  return theClosure(__closureContext, ...args);
}

with __closureContext set externally pre-call. That should at least avoid the ugly internal codegen we had initially and limit it to external calls. Not sure if this has only benefits, but thought I mention.

[DuncanUszkay1]

Given that I think there are three streams of work right now for people to pick up if they want to help:

• Rip out all the special ID logic
• Design and then implement a contextual anonymous function call system which works from the JS side Just going to document a workaround for now
• Create Binaryen Pass which directizes loads of anonymous functions (see description above)

[DuncanUszkay1]

with __closureContext set externally pre-call.

This seems like a good solution to prevent being incompatible with just anonymous functions (though it'd still be a breaking change).

Once we look at closures though I don't think this works- if we're passing the index to theClosure$export over the boundary, then how are we also going to give them the closure context at the same time? They won't be able to inject context into the global because they won't have access to it. (unless I'm misunderstanding your suggestion)

Another possible idea:

• When returning an anonymous function, we return the closure context pointer
• We export a function in the module which returns the function index from the closure context(just loading the first 4 bytes at the pointer address)
• You then call that function index with the closure context as the first parameter, followed by normal parameters
• Most of this would be hidden in the loader ideally

Advantages:

• Works with closures
• Minimal internal code gen (just a single function consisting of a single load)
  Disadvantage:
• We have to make 2 calls into the module to call an anonymous function. I don't really know the cost of this though- it doesn't seem like there wouldn't be much overhead in an extra module call but it may be runtime specific

[DuncanUszkay1]

Looked into interop stuff and I think the best thing to do here is nothing- we should just document the fact that from now on we'll be passing back pointers instead of function indexes. Anybody who is just using that function as an argument to another function won't need to change anything (except possibly some function signatures).

For anybody who is reliant on calling indirectly from the host (something not supported in the Assemblyscript loader), we can document a new process for calling them:

• load the function index from memory
• call the function indirectly with the pointer as the first argument

thoughts @dcodeIO?

[dcodeIO]

Sounds good to me. The docs can state the layout next to String etc. then, hinting at Functions being builtin objects. Reminds me that there is an opportunity here to add a standard Function class (non-closure has exactly one instance, closures have multiple) and see what we can do on top of it, like .call and such.

[DuncanUszkay1]

Reminds me that there is an opportunity here to add a standard Function class (non-closure has exactly one instance, closures have multiple) and see what we can do on top of it, like .call and such.

Probably a good idea to look into that- but not sure if it's needed to merge

[DuncanUszkay1]

Closing old some old PRs