Skip to content

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

yellowhatter
Copy link
Contributor

@yellowhatter yellowhatter commented Jun 2, 2025

Draft because it requires API alignment for C and C++ (to be done after we agree on this)

  • hide ProtocolID
  • simplify ShmProvider builder APIs
  • add minimal example for SHM
  • simplify some SHM examples using simplified SHM API

- simplify ShmProvider builder APIs
@yellowhatter yellowhatter requested a review from evshary June 2, 2025 15:18
@yellowhatter yellowhatter self-assigned this Jun 2, 2025
@yellowhatter yellowhatter added enhancement Existing things could work better new feature Something new is needed breaking-change Indicates that the issue implies a breaking change (be it at compile time or at runtime) api fix Correct API labels Jun 2, 2025
Copy link

codecov bot commented Jun 2, 2025

Codecov Report

Attention: Patch coverage is 49.29577% with 36 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@e6579fe). Learn more about missing BASE report.

Files with missing lines Patch % Lines
commons/zenoh-shm/src/api/provider/types.rs 0.00% 22 Missing ⚠️
commons/zenoh-shm/src/api/provider/shm_provider.rs 65.85% 14 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@evshary evshary left a 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.

.protocol_id::<POSIX_PROTOCOL_ID>()
.backend(backend)
.wait();
let provider = ShmProviderBuilder::backend(backend).wait();
Copy link
Contributor

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...and provide the size.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

run().await.unwrap();
}

async fn run_minimal() -> zenoh::Result<()> {
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed minimal

Copy link
Contributor

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added readme

Copy link
Contributor

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.

Copy link
Contributor Author

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
Copy link
Contributor

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.

Copy link
Contributor Author

@yellowhatter yellowhatter Jun 5, 2025

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:

  1. 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.
  2. ShmProviderBackend trait is made public so that any user can implement specific backend.
  3. 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.

Copy link
Contributor

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()
};
Copy link
Member

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.

// OPTION: comprehensive ShmProvider creation
// NOTE: For extended PosixShmProviderBackend API please check z_posix_shm_provider.rs
// create backend...
let comprehencive = PosixShmProviderBackend::builder()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/comprehencive/comprehensive

.protocol_id::<POSIX_PROTOCOL_ID>()
.backend(backend)
.wait();
let provider = ShmProviderBuilder::backend(backend).wait();
Copy link
Member

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.

Copy link
Contributor Author

@yellowhatter yellowhatter Jun 6, 2025

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.

.protocol_id::<POSIX_PROTOCOL_ID>()
.backend(backend)
.wait();
let provider = ShmProviderBuilder::backend(backend).wait();
Copy link
Member

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();
Copy link
Member

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.

Copy link
Contributor Author

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

@yellowhatter yellowhatter marked this pull request as ready for review June 10, 2025 12:24
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
@yellowhatter yellowhatter requested review from evshary and kydos June 10, 2025 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api fix Correct API breaking-change Indicates that the issue implies a breaking change (be it at compile time or at runtime) enhancement Existing things could work better new feature Something new is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants