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

Use unchecked function callbacks for better performance #186

Merged

Conversation

kpreisser
Copy link
Contributor

@kpreisser kpreisser commented Nov 12, 2022

Hi,

this PR implements unchecked function variants (bytecodealliance/wasmtime#3350) for the generic callback overloads added by #163, which improves performance.

This adds a ValueRaw struct that maps to wasmtime_val_raw_t and an IValueRawConverter<T> interface similar to the existing ones, which are then used by the unchecked callback functions.

Since the .NET side knows the exact parameter and result types used by the callback, this should be safe. (However I'm not very familiar with wasmtime's resource management, so I would appreciate if you can review especially the handling in the FuncRefValueRawConverter and GenericValueRawConverter.)

This also improves the NRT annotations for callback delegates (note that for <auto-generated> files, we must explicitely add #nullable enable even if the project already enables it), and fixes a regression that prevented to define callbacks using an interface as generic type parameter (because that is currently not allowed by ValueBox.Converter<T>(), but it is allowed by Value.TryGetKind()).

Additionally, callbacks that use neither Caller nor Function as parameters are now allocation-free when being called, since the Caller instance will no longer be created as it isn't needed in these cases.

When trying to run the benchmarks from #163 (but using 6 instead of 5 iterations) with .NET 6.0.11 on Windows 10 (Build 19044) x64, I get the following results:

Benchmark 1 (Action<int>):

Without this PR:

Elapsed: 00:00:00.1257743
Elapsed: 00:00:00.1165437
Elapsed: 00:00:00.1098291
Elapsed: 00:00:00.1045918
Elapsed: 00:00:00.1042576
Elapsed: 00:00:00.1047506

With this PR:

Elapsed: 00:00:00.0524017
Elapsed: 00:00:00.0489112
Elapsed: 00:00:00.0494636
Elapsed: 00:00:00.0482166
Elapsed: 00:00:00.0423121
Elapsed: 00:00:00.0429405

Benchmark 2 (Action<int, float, long>):

Without this PR:

Elapsed: 00:00:00.2216621
Elapsed: 00:00:00.1976731
Elapsed: 00:00:00.1968243
Elapsed: 00:00:00.1969110
Elapsed: 00:00:00.1974007
Elapsed: 00:00:00.1961490

With this PR:

Elapsed: 00:00:00.0652740
Elapsed: 00:00:00.0631706
Elapsed: 00:00:00.0615255
Elapsed: 00:00:00.0497510
Elapsed: 00:00:00.0505233
Elapsed: 00:00:00.0497147

Benchmark 3 (Func<int, float, long, ValueTuple<int, int, long>>):

Without this PR:

Elapsed: 00:00:00.4356896
Elapsed: 00:00:00.3905873
Elapsed: 00:00:00.3896729
Elapsed: 00:00:00.3901601
Elapsed: 00:00:00.3902660
Elapsed: 00:00:00.3917198

With this PR :

Elapsed: 00:00:00.0881153
Elapsed: 00:00:00.0869675
Elapsed: 00:00:00.0849886
Elapsed: 00:00:00.0742805
Elapsed: 00:00:00.0611120
Elapsed: 00:00:00.0619580

With the last benchmark, this seems to be roughly a 6x improvement comparing to the current state (without this PR).

What do you think?

Thanks!

This is a follow-up to PR bytecodealliance#163.

This also improves the NRT annotations for callback delegates, and fixes an regression that prevented to define callbacks taking or returning an interface.
@kpreisser kpreisser changed the title Use unchecked functions with raw values for better performance Use unchecked functions for callbacks for better performance Nov 12, 2022
src/ValueRaw.cs Outdated Show resolved Hide resolved
…behavior of the Rust implementation (and this doesn't seem to affect performance).
This allows callbacks that don't use a Caller, Function or object parameter to be allocation-free.
…so that allocating a Caller is now only necessary when unboxing Functions.
@kpreisser kpreisser changed the title Use unchecked functions for callbacks for better performance Use unchecked function callbacks for better performance Nov 21, 2022
@peterhuene peterhuene self-requested a review November 28, 2022 18:33
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.

@kpreisser thanks for all this work! I paid close attention to the externref/funcref raw value conversion and it seems correct to me.

@peterhuene peterhuene merged commit 1160b5e into bytecodealliance:main Nov 28, 2022
@kpreisser kpreisser deleted the generic-define-function-unchecked branch November 28, 2022 19:21
kpreisser added a commit to kpreisser/wasmtime-dotnet that referenced this pull request Dec 3, 2022
…rmance.

This applies the changes to function callbacks from bytecodealliance#186 also to the other side, where wasm functions are wrapped with Function.Wrap().

Note that Function.Invoke() still uses the regular Value.
peterhuene pushed a commit that referenced this pull request Dec 8, 2022
…rmance (#194)

* Use unchecked function calls (for wrapped functions) for better performance.

This applies the changes to function callbacks from #186 also to the other side, where wasm functions are wrapped with Function.Wrap().

Note that Function.Invoke() still uses the regular Value.

* Follow-Up: Add the IsNull check to WrapAction/WrapFunc, and remove an unnecessary check in Invoke().

* Add a comment explaining the differences in return type handling compared to e.g. Function.FromCallback().

Also, add a missing CultureInfo.InvariantCulture, and fix formatting.

* Follow-Up: Make calling the function safer by always using the maximum of the parameter count and Results.Count for the stackalloc, regardless of what overload was used.

* Follow-Up: Simplify.
@kpreisser kpreisser mentioned this pull request Jan 13, 2023
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.

2 participants