-
Notifications
You must be signed in to change notification settings - Fork 195
SHM API improvement: ProtocolID and others... #1975
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
- simplify ShmProvider builder APIs
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1975 +/- ##
=======================================
Coverage ? 70.86%
=======================================
Files ? 365
Lines ? 61617
Branches ? 0
=======================================
Hits ? 43666
Misses ? 17951
Partials ? 0 ☔ View full report in Codecov by Sentry. |
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.
I'm happy that the ID disappeared and made the usage simpler 👍
I think we can polish the examples for better understanding.
examples/examples/z_queryable_shm.rs
Outdated
.protocol_id::<POSIX_PROTOCOL_ID>() | ||
.backend(backend) | ||
.wait(); | ||
let provider = ShmProviderBuilder::backend(backend).wait(); |
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.
Why can't we use let provider = ShmProviderBuilder::default_backend().wait().unwrap();
in this example?
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.
...and provide the size.
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.
done
examples/examples/z_alloc_shm.rs
Outdated
run().await.unwrap(); | ||
} | ||
|
||
async fn run_minimal() -> zenoh::Result<()> { |
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.
In fact, I'm not pretty sure if we want this. The basic usage should be already in other examples.
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.
removed minimal
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.
It took me some time to understand what the example was trying to convey. I suggest adding the description in the example README. (z_alloc_shm.rs
is missing 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.
added readme
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.
I'm new to the Zenoh shared memory usage. While we provide two ways to allocate SHM (direct allocation and layout allocation), I would suggest using a block to isolate each way to avoid confusion.
// Option 1:
let _direct_allocation = {
...
};
// Option 2:
let _layout_allocation = {
let buffer_layout = {...};
let sbuf = {...};
sbuf
};
I'm just taking an example. You could choose a better variable name.
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.
Reorganized options in this example
}; | ||
|
||
/// Client factory implementation for particular shared memory protocol | ||
#[zenoh_macros::unstable_doc] | ||
#[derive(Debug)] | ||
pub struct PosixShmClient; | ||
|
||
impl WithProtocolID for PosixShmClient { | ||
fn id(&self) -> ProtocolID { | ||
POSIX_PROTOCOL_ID |
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.
Just out of curiosity, do we plan to provide more ways to allocate SHM? I only see POSIX_PROTOCOL_ID
now, so I'm not sure why we need to specify the ID.
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.
There are reasons to keep the ID:
- Some day we can implement some specific shared-memory backends like ones backed by SystemV shared memory or some device vendor things like CUDA shared-memory.
ShmProviderBackend
trait is made public so that any user can implement specific backend.- IDs + file access mode flags can be used to artificially split SHM domains.
SHM is architectured to be extremely flexible. You might find some things excessive, for example all that stuff with Layout and Alignment - and it was made to smoothly support allocator API (like C++ STL-style allocators) and more specific stuff for GPU. Now I'm polishing it to make things easier for basic usage and hide too much specific things yet leaving them accessible for advanced use case.
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.
I believe CUDA shared-memory is something useful. Make sense in this case.
.unwrap(); | ||
// ...and an SHM provider | ||
ShmProviderBuilder::backend(comprehencive).wait() | ||
}; |
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.
I think we should define the alignment for the provider and completely remove the layout type.
examples/examples/z_alloc_shm.rs
Outdated
// OPTION: comprehensive ShmProvider creation | ||
// NOTE: For extended PosixShmProviderBackend API please check z_posix_shm_provider.rs | ||
// create backend... | ||
let comprehencive = PosixShmProviderBackend::builder() |
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.
s/comprehencive/comprehensive
examples/examples/z_bytes_shm.rs
Outdated
.protocol_id::<POSIX_PROTOCOL_ID>() | ||
.backend(backend) | ||
.wait(); | ||
let provider = ShmProviderBuilder::backend(backend).wait(); |
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.
For POSIX SHM it would be nice to be able to combine the two steps into one.
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.
Yes, it is possible to do like this:
let provider = ShmProviderBuilder::default_backend(16 * 1024 * 1024).wait()?;
let mut shm_buffer = provider.alloc(512).wait()?;
No layouts by default, nothing, just pure sizes.
However, I want to keep the layout API both for provider and allocations - just hide it behind streamlined defaults. The rationale to do that is:
- allocator APIs
- the integration of GPU and FPGA devices into our shared-memory - there you might easily have 1024 byte alignment for certain cases and sometimes operate with different differently-aligned arrays of data within the same project - that's where the layout API will have it's use.
- custom provider backends for specific needs
So yes, I already take measures to hide layouts away from general use-case, and I vote to keep it for advanced scenario.
examples/examples/z_queryable_shm.rs
Outdated
.protocol_id::<POSIX_PROTOCOL_ID>() | ||
.backend(backend) | ||
.wait(); | ||
let provider = ShmProviderBuilder::backend(backend).wait(); |
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.
...and provide the size.
.protocol_id::<POSIX_PROTOCOL_ID>() | ||
.backend(backend) | ||
.wait(); | ||
let provider = ShmProviderBuilder::backend(backend).wait(); | ||
|
||
// Prepare a layout for allocations | ||
let layout = provider.alloc(1024).into_layout().unwrap(); |
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 layout has confused many users. I suggest we fix the layout at the creation of the provider and then only expose a simple alloc. This would make it more user friendly and will streamline the implementation of the SHM. I don't think we should allow to allocate with different alignment within the same provider.
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.
Pls check my answer above
Conflicts: commons/zenoh-shm/src/api/client_storage/mod.rs commons/zenoh-shm/src/api/protocol_implementations/posix/posix_shm_provider_backend.rs commons/zenoh-shm/src/api/provider/shm_provider.rs
Draft because it requires API alignment for C and C++ (to be done after we agree on this)