-
Notifications
You must be signed in to change notification settings - Fork 48
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
Optimisations On 10M Calls #288
Optimisations On 10M Calls #288
Conversation
…s 10M calls back into C#. Runtime of this test program was 350ms initially. `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.
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 I'm not sure if this indicates a potential issue in other places |
AFAIK, If |
@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 Edit: I just tried changing them all to |
Opened #289 to resolve the IsInvalid thing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
Does this need to be rebased on #289 or good to merge? |
It should be good to merge as is :) |
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 theCaller
is not required this skips creating theCaller
(in turn skipping a call towasmtime_caller_context
) and skips accessing the store from the caller (in turn skippingGCHandle.FromIntPtr
to retrieve theStore
object). Overall this reduced the total execution time down from 350ms to 272ms.Store
has been modified to fetch thecontextHandle
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 checkinghandle.IsInvalid
in theContext
property. Doing all of this skips a call towasmtime_store_context
every timeContext
is accessed. Overall this reduced total execution from time down from 272ms to 208ms.