Replaced class Caller with struct Caller

[martindevans]

Discovered a potential bug in the current Caller implementation around disposal. Opening up a draft PR to test with CI.

[peterhuene]

Indeed, this is because we're copying handle into the copied Caller, meaning the disposed check succeeds for stash when it should not.

If we don't implement IDisposable on Caller, is it possible for it to be a ref struct? I don't think we'd need to have a dispose implementation in that case since it's not actually freeing the handle with Wasmtime. It shouldn't be able to be captured either since it's only valid for the current callback frame.

[martindevans]

Indeed, this is because we're copying handle into the copied Caller, meaning the disposed check succeeds for stash when it should not.

Yep, when I was writing my original comment I'd somehow forgotten the change to struct was my change. This is why I don't usually do PRs late in the evening 😆

If we don't implement IDisposable on Caller, is it possible for it to be a ref struct?

This was actually my original intention (see discussion in #208). I did a lot of experimentation with it and found that it mostly worked but fell over in the reflection based cases (calling through reflection boxes parameters and you can't box a ref struct). Even if we could work around that one particular case ref struct is just a bit awkward to work with, so I thought it wouldn't necessarily be a better API (even if it does communicate the lifetimes better).

[martindevans]

This re-introduces the CallerContext mechanism to track disposal (using epochs), which correctly handles the case where a Caller is leaked.

[peterhuene]

With the evolution of the API, I wonder if it would be worth simply eliminating the reflection-based fallbacks entirely in favor of requiring users to use an untyped callback now that they're implemented.

That would be quite similar to how the Rust API works (can wrap signatures up to a certain limit, with the type checks performed statically; otherwise, must accept the parameters/returns as slices of "values").

[martindevans]

That would get my vote. I've been wondering if we should do away with the reflection based callbacks for a while - silently falling back into a significantly slower path felt a bit iffy, I just didn't have a good alternative before the untyped callbacks.

[peterhuene]

I think we should take that approach and simplify this PR to a ref struct caller.

I'm happy to take that change on before the next release if you'd like.

[martindevans]

Sounds good to me 👍

As you can probably tell from my scatterbrained comments it's too late in the day for me to manage it 😄 😴

[peterhuene]

@martindevans so removing the reflection fallback is proving to be a large enough change as to make me uncomfortable for including it in an already-overdue release without it sitting in main for a little while.

How important is fixing the Caller allocation for these immediate releases? Is it important enough to move forward with this PR with the knowledge that moving to ref struct in the near future will remove the epoch solution prescribed here?

[peterhuene]

Also, it appears we can't migrate fully to ref struct Caller as ref structs can't be used as generic type parameters, meaning we wouldn't be able to use it for all of the "has caller" Action/Func overloads of FromCallback/DefineFunction that aren't using reflection.

Defining our own delegate types for each of those overloads is possible, but it makes the API unwieldy.

I propose we move forward with this fix (or something similar) and I'll rip out the reflection-based fallback with a future commit past this release window.

[peterhuene]

I think we'll need to properly benchmark this change as I'm not convinced that saving on the alloc of a reference type caller outweighs the epoch management to guarantee we can dispose a value type Caller properly.

[kpreisser]

Regarding the immediate release, if you want to create it without including the Caller allocation change, IMHO it might make sense to not base it on current main but on commit 18020d2, as the next commit (#211) will be a breaking API change (due to the removal of IStore which impacts the Caller); so that the breaking API changes can be combined together in the next release after that. What do you think?

By the way, in order to quickly identify public API changes between releases, the package Microsoft.CodeAnalysis.PublicApiAnalyzers (documentation) can be used which analyzes the public API and records them in two text files, PublicAPI.Shipped.txt and PublicAPI.Unhipped.txt.
When the API doesn't match with what is in the text files, the analyzer will generate a warning, and Visual Studio (and probably other IDEs) provides a codefix to apply the changes to PublicAPI.Unhipped.txt. See: https://www.natmarchand.fr/prevent-breaking-changes-apis-roslyn/

When creating a release, you would apply the changes from PublicAPI.Unshipped.txt into PublicAPI.Shipped.txt (e.g. using this script), so that PublicAPI.Unshipped.txt will be empty again. That way you can quickly identiy how the public API would change in the next release (as indicated by the entries in PublicAPI.Unshipped.txt), and how it changed between existing releases (by viewing the commit history on PublicAPI.Shipped.txt).
For example, .NET runtime projects like use WinForms use this analyzer.

What do you think about adding it to wasmtime-dotnet?

Defining our own delegate types for each of those overloads is possible, but it makes the API unwieldy.

Personally, I would actually be OK with defining our own CallerAction and CallerFunc delegates (#208 (comment)), so that we can use a ref struct Caller as delegate parameter.
For example, there are already a huge number of Linker.DefineFunction and Callback.FromCallback overloads, and I think adding such function delegates would be in a similar category.

What do you think?
Thanks!

[martindevans]

How important is fixing the Caller allocation for these immediate releases?

We're going to be releasing our Unity package soon (3-4 weeks, mostly feature freeze next week), likely using whatever the next version of wasmtime-dotnet is. So I'd prefer that it's in the next release. That said, even this PR isn't the whole solution (Memory/Function still get allocated) so it's not really critical either, since we know we won't be zero-alloc until future updates anyway.

On balance I think what kpreisser suggested re. releasing 18020d2 is probably the best approach. We can delay all the Caller based churn to the next update and then we're not rushed deciding exactly which mechanism we want to adopt.

Defining our own delegate types for each of those overloads is possible, but it makes the API unwieldy.

I prototyped this approach (messy prototype work here) and didn't find it too bad. We could easily generate those delegates up to whatever number of parameters we need. As far as I could see this change to the public API seems to be source compatible which is a nice bonus. In the end I abandoned this approach due to the ref struct still needing some kind of IStore implementation to pass into Memory/Function objects retrieved from the caller, obviously that's no longer a problem with the new IStore changes.

I'm happy to restart this work with an aim to get it into the next release, if that's what we want?

properly benchmark this change

I just threw together a quick benchmark by modifying the simple benchmark to add a Caller argument to hello and to only call run.Invoke(); in the benchmarked section.

Current approach:

Method	Mean	Error	StdDev
SayHello	57.54 ns	0.245 ns	0.217 ns

Struct+Epoch with CallerContext pool (this PR):

Method	Mean	Error	StdDev
SayHello	151.3 ns	1.28 ns	1.20 ns

However it's very difficult to properly benchmark this kind of thing and I'm not convinced these numbers are very meaningful. With such a small+simple heap containing only short lived objects the GC is basically guaranteed to win! I also don't know if benchmark-dotnet will even be fully counting the GC time in the numbers. We can probably take these benchmarks as a best possible case for the current approach.

[martindevans]

Superseded by #214.