Save an unconditional clone of guest parameters for registered guest functions

[ludfjig]

vs main:

Only affects registered guest functions (not c api, nor when someone implements their own guest_dispatch_function).

Note that this is a breaking change for downstream users who manually register guest functions (hyperlight-wasm), but even more scary is that it will break silently, since the function signature is not type-checked😱. We should probably address this by exporting the type as pub, and add const time assertions in the downstream libraries. Or make it a macro that users use, which provides the function signature, and let the user provide the function body

[dblnz]

This is fine from my point of view. I've got burnt by this slight difference between the guest_dispatch_function definition and the other function definitions.

[syntactically]

The perf improvement looks great, and I don't think that the amount of breaking that you mention there is necessarily a barrier. If we wanted to be very proper I suppose we would introduce a new GuestFunctionDefinitionV2 structure and mark the original deprecated so that a misuse threw an error---but I don't know if it is worth it?

Two slight comments:

• Is there a reason why we have the function pointer on the GuestFunctionDefinition be a usize, rather than an appropriate fn() type, which might help catch any other misuses of this?
• The other direction that we could go here is perhaps to see if it is possible to get rid of the requirement of having an owned FunctionCall struct in order to build the parameter tuple to hand off to the registered guest function. I would be curious if you have any thoughts on doing that. Having this be owned the way it is seems a little questionable, because it means that the copy that is currently happening in try_pop_shared_input_data_into is impossible to soundly eliminate. Of course, the lifetime constraints get a little bit more hairy with the changes to buffer lifetime and support for async code in Add HIP 0001 ring buffer proposal for Hyperlight I/O #1112, but it's still not totally obvious to me that making this owned would not preclude some nice ability to make direct references into the I/O buffers during the lifetime of the call. @andreiltd would you care to chime in on this at all?

[syntactically]

If we wanted to be very proper I suppose we would introduce a new GuestFunctionDefinitionV2 structure and mark the original deprecated so that a misuse threw an error---but I don't know if it is worth it?

I know @jsturtevant has strong opinions on the permissibility of breaking changes that can cause silent failure in downstreams, so paging him on this as well.

[ludfjig]

Maybe we can rename guest_dispatch_function randomly between every version, and provide a macro that defines it, which would force users to use our macro :D We can then make changes in the future without breaking people (hopefully).

Regarding the owned FunctionCall indeed it does make things less flexible in the future, especially if we'd want to borrow directly from raw buffer with appropriate lifetimes. Maybe we can provide a method that users should call in order to get the parameters, to allow even more flexibility

[jprendes]

This LGTM.
I agree with Lucy's concerns about borrowing data from the buffer directly.
But borrowing from the buffer would imply a complete redesign of the parameters / return value, and will certainly introduce a breaking chage regardless of what we do here.
For example, a guest function that would take a String would not be able to borrow from the buffer, it would have to take a &str, same with Vec<u8>. And those are very common patterns that we have encouraged people to use.
Until then this gives us a significant performance win, so it gets a thumbs up from me :-)

[jprendes]

Maybe we can rename guest_dispatch_function randomly between every version, and provide a macro that defines it, which would force users to use our macro :D We can then make changes in the future without breaking people (hopefully).

Regarding the owned FunctionCall indeed it does make things less flexible in the future, especially if we'd want to borrow directly from raw buffer with appropriate lifetimes. Maybe we can provide a method that users should call in order to get the parameters, to allow even more flexibility

Or we can make the type private, and make people use the guest_function macro :-D (joking not joking)

[jsturtevant]

If we wanted to be very proper I suppose we would introduce a new GuestFunctionDefinitionV2 structure and mark the original deprecated so that a misuse threw an error---but I don't know if it is worth it?

I know @jsturtevant has strong opinions on the permissibility of breaking changes that can cause silent failure in downstreams, so paging him on this as well.

This seems like a pretty small and reasonable thing to do to avoid any downstream users having any issues with this. I believe building a habit of recognizing and coming up with patterns to avoid unexpected downstream issues is a good thing.

[ludfjig]

Updated @syntactically @jprendes @dblnz @jsturtevant

[jsturtevant]

does this still need to be public? could we just use the marco going forward?

[ludfjig]

oh yeah let's switch to the macro here too! Fixed

[jsturtevant]

Is this borrow right here?

[ludfjig]

good observation, but yes it is correct. The guest c-api has it's own function signature that needs to be hl_Vec *c_guest_dispatch_function(const hl_FunctionCall *function_call)