Optimisations On 10M Calls

[martindevans]

Optimisations based on profiling a simple program that loops and makes 10M calls back into C#. Runtime of this test program was 350ms initially. Test program is here: https://gist.github.com/martindevans/0e0f334bc095808b81c13dedde6c80c1

FromCallback has been modified to use the store initially used to create the callback, instead of retrieving it from the caller context. In cases where the Caller is not required this skips creating the Caller (in turn skipping a call to wasmtime_caller_context) and skips accessing the store from the caller (in turn skipping GCHandle.FromIntPtr to retrieve the Store object). Overall this reduced the total execution time down from 350ms to 272ms.

Store has been modified to fetch the contextHandle when first created. The docs (https://docs.wasmtime.dev/c-api/structwasmtime__context.html) state that the context handle lifetime is the same as the store lifetime, so it should be safe to keep it as long as the store is checked before accessing the context. This is achieved by checking handle.IsInvalid in the Context property. Doing all of this skips a call to wasmtime_store_context every time Context is accessed. Overall this reduced total execution from time down from 272ms to 208ms.

[martindevans]

I initially had this code guarding access to the context:

if (handle.IsInvalid)
{
    throw new ObjectDisposedException(typeof(Store).FullName);
}

return new StoreContext(contextHandle);

However, this failed in unit tests. Even after store.Dispose() is called IsInvalid was false but IsClosed was true.

I'm not sure if this indicates a potential issue in other places IsInvalid is used?

[kpreisser]

However, this failed in unit tests. Even after store.Dispose() is called IsInvalid was false but IsClosed was true.

I'm not sure if this indicates a potential issue in other places IsInvalid is used?

AFAIK, SafeHandle.IsInvalid determines whether the handle value is invalid according to the handle type it represents (e.g. SafeHandleZeroOrMinusOneIsInvalid would report 0 and -1 as being invalid); it doesn't necessarily report whether the handle has already been released/disposed, which is indicated by .IsClosed.

If SafeHandle.IsInvalid is used in other places where the intention was to check whether the handle has already been released (e.g. to throw an ObjectDisposedException), then I would agree this was probably a mistake and should be replaced with calling .IsClosed.

[martindevans]

@kpreisser in this case here's an example of some code that looks suspect to me. This same pattern is used everywhere, in fact the new code I just added is the only placed IsClosed is used! Test coverage shows that none of the IsInvalid branches are ever tested.

Edit: I just tried changing them all to IsClosed. Tests still pass, but also coverage shows that all of those branches are still never taken so confidence is low.

[martindevans]

Opened https://github.com/bytecodealliance/wasmtime-dotnet/pull/289 to resolve the IsInvalid thing.

[peterhuene]

Does this need to be rebased on #289 or good to merge?

[martindevans]

It should be good to merge as is :)