Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ref struct caller #214

Merged
merged 1 commit into from
Jan 25, 2023
Merged

Conversation

martindevans
Copy link
Contributor

@martindevans martindevans commented Jan 25, 2023

This builds on #213, merge that first

Third time lucky?

Replaced Caller with a ref struct. With the reflection path stripped out and the removal of IStore this was a fairly simple change.

A few notable changes:

  • Can't leak Caller, any breakage here is probably uncovering a bug.
  • Caller is no longer disposable (we could add an empty Dispose() method and mark it [Obsolete]?).

Also this test:

[Fact]
public void ItCannotBeConstructedFromNullPointer()
{
    var act = () => new Caller(IntPtr.Zero);
    act.Should().Throw<InvalidOperationException>();
}

Did not compile because act is now a CallerAction instead of an Action, which is surprising change of var inference. Fixed by changing var to Action. Never mind, see comment by kpreisser below.

Benchmark

Unlike my previous epoch based approach in #212 (which was 90ns slower) this benchmarks at exactly the same speed as the class Caller approach.

@martindevans
Copy link
Contributor Author

I thought I'd let the time zone differential work in my favour today ;)

@martindevans martindevans marked this pull request as ready for review January 25, 2023 15:28
@kpreisser
Copy link
Contributor

Did not compile because act is now a CallerAction instead of an Action

I think this is unrelated to the new CallerAction<...> and CallerFunc<...> types; it's actually an anonymous delegate type that the C# compiler creates when it can't match an existing delegate type (Action/Func):
grafik

This is because it would normally infer a Func<Caller> (as the expression new Caller(...) can also be used as return expression), which can't be matched now due to the ref struct Caller, so it generates a new delegate type like delegate Caller X().

@peterhuene
Copy link
Member

This looks good; I guess the explicitly defined delegates weren't as bad as I thought they'd be.

Let's merge #213, rebase this PR, and then I'll review it for inclusion in the next release.

@peterhuene
Copy link
Member

We should be able to rebase this now.

@martindevans
Copy link
Contributor Author

Rebased onto main.

Copy link
Member

@peterhuene peterhuene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thanks for this and I'm glad we can now leave the escaping of Caller from the current frame up to the C# compiler 👍.

@peterhuene peterhuene merged commit 64ea937 into bytecodealliance:main Jan 25, 2023
@peterhuene
Copy link
Member

Okay, with everything merged, I'm going to cut releases and call out any potential API incompatibilities.

@peterhuene peterhuene mentioned this pull request Jan 25, 2023
@martindevans martindevans deleted the ref-struct-caller branch January 25, 2023 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants