-
Notifications
You must be signed in to change notification settings - Fork 142
Optimize function call serializing #778
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
base: main
Are you sure you want to change the base?
Conversation
bf0445e
to
4196a2f
Compare
1ffa58d
to
0c83035
Compare
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.
LGTM, thanks for the extensive tests!
and a c+rust sample workload benchmark Signed-off-by: Ludvig Liljenberg <[email protected]>
Signed-off-by: Ludvig Liljenberg <[email protected]>
…ioncall Signed-off-by: Ludvig Liljenberg <[email protected]>
This should save memory allocations, but require that a FlatBufferBuilder is passed in. Signed-off-by: Ludvig Liljenberg <[email protected]>
Signed-off-by: Ludvig Liljenberg <[email protected]>
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.
Pull Request Overview
This PR optimizes FunctionCall serialization to improve performance by reducing memory allocations and improving buffer capacity estimation. The changes focus on making serialization more efficient by returning borrowed slices instead of owned vectors and pre-allocating FlatBufferBuilder capacity.
Key changes:
- Refactored FunctionCall serialization to use
encode()
method returning&[u8]
instead ofTryFrom<FunctionCall> for Vec<u8>
- Added capacity estimation function to pre-allocate FlatBufferBuilder capacity and avoid reallocations
- Updated function signatures to accept
&[u8]
instead ofVec<u8>
where possible
Reviewed Changes
Copilot reviewed 11 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
src/hyperlight_common/src/flatbuffer_wrappers/function_call.rs |
Refactored serialization from TryFrom trait to encode() method returning borrowed slice |
src/hyperlight_common/src/flatbuffer_wrappers/util.rs |
Added capacity estimation function with comprehensive tests |
src/hyperlight_common/src/flatbuffer_wrappers/function_types.rs |
Fixed VecBytes deserialization to use .bytes().to_vec() instead of iterator |
src/hyperlight_host/src/sandbox/initialized_multi_use.rs |
Updated to use new encode method with capacity estimation |
src/hyperlight_guest/src/guest_handle/io.rs |
Changed parameter from Vec<u8> to &[u8] |
src/hyperlight_guest/src/guest_handle/host_comm.rs |
Updated to use new encode method and pass references |
src/hyperlight_guest_bin/src/guest_function/call.rs |
Updated function call to pass reference instead of owned vector |
src/hyperlight_host/benches/benchmarks.rs |
Added benchmarks for serialization performance and sample workloads |
src/tests/rust_guests/simpleguest/src/main.rs |
Added benchmark test function for 24K input/8K output scenario |
src/tests/c_guests/c_simpleguest/main.c |
Added C version of benchmark test function |
src/hyperlight_guest/Cargo.toml |
Added flatbuffers dependency |
} | ||
|
||
// match how vec grows | ||
estimated_capacity.next_power_of_two() |
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.
The next_power_of_two()
call could potentially over-allocate significantly for large capacities. For example, if the estimated capacity is just over a power of 2, this will double the allocation. Consider adding a maximum threshold or using a more conservative growth strategy for very large estimates.
estimated_capacity.next_power_of_two() | |
// match how vec grows, but avoid excessive over-allocation for large buffers | |
const MAX_POWER_OF_TWO_THRESHOLD: usize = 1_048_576; // 1 MiB | |
if estimated_capacity < MAX_POWER_OF_TWO_THRESHOLD { | |
estimated_capacity.next_power_of_two() | |
} else { | |
estimated_capacity | |
} |
Copilot uses AI. Check for mistakes.
}) | ||
.transpose()?; | ||
/// Encodes self into the given builder and returns the encoded data. | ||
pub fn encode<'a>(&self, builder: &'a mut FlatBufferBuilder) -> &'a [u8] { |
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.
The encode
method mutates the passed FlatBufferBuilder and returns a reference to its internal data. This creates a tight coupling between the builder's lifetime and the returned slice. Consider documenting that the builder should not be reused after this call, or design the API to take ownership of the builder to make this constraint clearer.
pub fn encode<'a>(&self, builder: &'a mut FlatBufferBuilder) -> &'a [u8] { | |
/// Encodes self into the given builder and returns the encoded data as a Vec<u8>. | |
/// Consumes the builder, which must not be reused after this call. | |
pub fn encode(&self, mut builder: FlatBufferBuilder) -> Vec<u8> { |
Copilot uses AI. Check for mistakes.
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.
this seems like a reasonable suggestion
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.
Nice optimization! Mostly LGTM. Just a couple of questions/nits here and there 👍
let mut estimated_capacity = 20; | ||
|
||
// Function name overhead | ||
estimated_capacity += function_name.len() + 12; | ||
|
||
// Parameters vector overhead | ||
estimated_capacity += 12 + args.len() * 6; | ||
|
||
// Per-parameter overhead | ||
for arg in args { | ||
estimated_capacity += 16; // Base parameter structure | ||
estimated_capacity += match arg { | ||
ParameterValue::String(s) => s.len() + 20, | ||
ParameterValue::VecBytes(v) => v.len() + 20, | ||
ParameterValue::Int(_) | ParameterValue::UInt(_) => 16, | ||
ParameterValue::Long(_) | ParameterValue::ULong(_) => 20, | ||
ParameterValue::Float(_) => 16, | ||
ParameterValue::Double(_) => 20, | ||
ParameterValue::Bool(_) => 12, |
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.
Maybe consider making these values consts to avoid diff magic numbers here and there?
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.
Feel free to address the remaining comments but this LGTM
Closes #789
This PR optimizes serialization of
FunctionCall
s by:Future Todos:
Writer
on which they pass return values.Relevant benchmark results compared to main branch (but with c2e6cdd which adds the 2 first benchmarks):