Proposal: Sample API rework #251

p-avital · 2024-02-26T14:28:25Z

This PR proposes to obfuscate z_sample_t, allowing future API additions and layout changes

eclipse-zenoh-bot · 2024-02-26T14:28:39Z

@p-avital If this pull request contains a bugfix or a new feature, then please consider using Closes #ISSUE-NUMBER syntax to link it to an issue.

milyin · 2024-03-01T14:58:33Z

Cargo.toml

@@ -61,7 +61,7 @@ serde_yaml = "0.9.19"

 [lib]
 path="src/lib.rs"
-name = "zenohc"
+name = "zenohcd"


undo this automatic change

Mallets · 2024-03-07T15:10:08Z

include/zenoh_commons.h

@@ -623,7 +625,7 @@ typedef struct z_delete_options_t {
 * To check if `val` is still valid, you may use `z_X_check(&val)` (or `z_check(val)` if your compiler supports `_Generic`), which will return `true` if `val` is valid.
 */
 typedef struct z_owned_encoding_t {
-  enum z_encoding_prefix_t prefix;
+  uint64_t prefix;


This change should come in a separate PR.

milyin · 2024-03-07T18:01:13Z

src/commons.rs

-    pub fn new(sample: &'a Sample, owner: &'a ZBuf) -> Self {
-        let std::borrow::Cow::Borrowed(payload) = owner.contiguous() else {
+    pub fn new(sample: &'a Sample) -> Self {
+        if !sample.value.payload.zslices().count() <= 1 {


if ! zslices.count <= 1 is exactly if zslices.count >1, but less readable
And should we also check for emply payload: zslices.count == 0 ?

With the exposure of ZBuf, we actually don't want to check it at all anymore, because samples with non-contiguous payloads are now allowed

src/commons.rs

milyin · 2024-03-07T18:06:18Z

src/commons.rs

@@ -436,21 +428,24 @@ impl From<zenoh_protocol::core::KnownEncoding> for z_encoding_prefix_t {
 #[repr(C)]
 #[derive(Clone, Copy, Debug)]
 pub struct z_encoding_t {
-    pub prefix: z_encoding_prefix_t,
+    pub prefix: u64,


As @Mallets mentioned, this change and changes below are for separate PR

src/lib.rs

yellowhatter · 2024-03-21T14:43:25Z

include/zenoh_commons.h

+ * This is achieved by increasing the reference count if the buffer is already contiguous, and by copying its data in a new contiguous buffer if it wasn't.
+ */
+ZENOHC_API
+struct z_owned_buffer_t z_buffer_contiguous(struct z_buffer_t buffer);


This is similar situation to mutable API discussed previously with @Mallets . Implicit copy API is bad for the following reasons:

masks the potential operation overhead (for user code reviewers)

isolates user code from having knowledge on the side effects

forces user code to have only one behavior (what if user wants to drop non-contiguous buffers instead of copying them? or user wants to process non-contiguous buffers in separate code branch? or user doesn't need to create z_owned_buffer_t wrapper around contiguous data?)

Instead, I just suggest adding bool is_contiguous(struct z_buffer_t); convenience method and give user all the freedom.

As discussed at the meeting today, I support the idea of removing z_buffer_contiguous both here and in rust.
For simple cases (number, string, etc., which IMHO will cover most of the needs of ordinary users), simple operations for obtaining them should be provided, for complex ones (working directly with buffers, which will most likely be used by experienced users) we should not hide the internal structure to avoid side effects.

DenisBiryukov91 · 2024-03-21T16:08:08Z

src/collections.rs

+///
+/// If the payload was not contiguous in memory, `z_bytes_null` will be returned instead.
+#[no_mangle]
+pub extern "C" fn z_buffer_payload(buffer: z_buffer_t) -> z_bytes_t {


I believe the behavior of this method is counterintuitive for most users, since it might be unexpected that z_buffer_contiguous needs to be called in certain cases (which ones btw? i.e. can we reliable predict when the buffer contents is actually guaranteed to be contagious ?) to receive non-null z_bytes_t. I would rather suggest to modify the output of this method into vector of z_bytes_t, or return a struct containing a union z_bytes_t/vector of z_bytes_t to more closely resemble what we have in rust.

According to the logic I suggest here, this method won't be needed anymore

DenisBiryukov91 · 2024-03-21T17:58:10Z

src/collections.rs

+    };
+    match buffer.contiguous() {
+        std::borrow::Cow::Borrowed(buffer) => buffer.into(),
+        std::borrow::Cow::Owned(_) => z_bytes_null(),


if we get the Owned enum member, it means we already allocated and copied element into a vector. It seems to be a pity not to return the data in this case.

The core problem here is that z_bytes_t is a borrowed type according to our convention: it may never be owned by the user. Hence, the user must never call its destructor, so giving the vector tp the user would cause a memory leak, or break the ownership conventions.

DenisBiryukov91 · 2024-03-21T18:03:46Z

src/querying_subscriber.rs

-                    };
-                    let sample = z_sample_t::new(&sample, &owner);
+                .callback(move |mut sample| {
+                    if let std::borrow::Cow::Owned(v) = sample.payload.contiguous() {


I suppose since we have api for working with non-contiguous ZBuf, we no longer need to convert payload to contiguous.

yellowhatter · 2024-03-26T11:28:05Z

src/commons.rs

+pub struct zc_owned_sample_t {
+    _0: z_owned_keyexpr_t,
+    _1: z_owned_buffer_t,
+    _2: z_owned_buffer_t,


Todo: investigate if this size padding should be made with _3

yellowhatter · 2024-03-26T11:32:20Z

include/zenoh_commons.h

+ *
+ * If the payload was not contiguous in memory, `z_bytes_null` will be returned instead.
+ */
+ZENOHC_API struct z_bytes_t z_buffer_payload(struct z_buffer_t buffer);


Todo: let's remove this

yellowhatter · 2024-03-26T11:37:29Z

include/zenoh_commons.h

+ZENOHC_API struct z_owned_buffer_t z_sample_owned_payload(const struct z_sample_t *sample);
+/**
+ * The sample's data, the return value aliases the sample.
+ *
+ * If you need ownership of the buffer, you may use `z_sample_owned_payload`.
+ */
+ZENOHC_API struct z_bytes_t z_sample_payload(const struct z_sample_t *sample);


Todo: replace this with one method
ZENOHC_API struct z_payload_t z_sample_payload(const struct z_sample_t *sample);

yellowhatter

Changes requested: @sashacmc @DenisBiryukov91 @milyin @yellowhatter
Also we would like to implement read method in separate PR
Also we have to be able to send z_owned_buffer_t instead of (or with) z_bytes_t

yellowhatter · 2024-03-26T11:58:10Z

src/collections.rs

@@ -21,8 +27,8 @@ use zenoh::prelude::ZenohId;
 #[repr(C)]
 #[derive(Clone, Copy, Debug)]
 pub struct z_bytes_t {


todo: replace with ZSlice API everywhere

yellowhatter · 2024-03-26T12:08:47Z

src/collections.rs

+            std::borrow::Cow::Borrowed(_) => value,
+            std::borrow::Cow::Owned(value) => value.into(),
+        };
+        unsafe { core::mem::transmute(Some(value)) }


core::mem::transmute should be used ONLY in impl_guarded_transmute macro, so no such code should present

p-avital added 4 commits February 29, 2024 14:44

introduce owned buffers

a52ef72

privatize sample

8dbbf37

add support for owned samples

3551d38

document the owned sample type itself

7148d5b

p-avital force-pushed the api-rework branch from 14002e0 to 7148d5b Compare February 29, 2024 13:44

fix macro usages

9c9f1ac

p-avital requested a review from milyin March 1, 2024 10:34

milyin approved these changes Mar 1, 2024

View reviewed changes

p-avital added 2 commits March 1, 2024 16:07

fix auto-generated zenohcd

3eb7236

introduce split buffers in zenoh-c

769d824

Mallets reviewed Mar 7, 2024

View reviewed changes

milyin requested changes Mar 7, 2024

View reviewed changes

address pr comments

1b09b30

milyin linked an issue Mar 12, 2024 that may be closed by this pull request

zc_payload_rcinc and zc_attachment_rcinc #238

Open

milyin mentioned this pull request Mar 12, 2024

zc_payload_rcinc and zc_attachment_rcinc #238

Open

Mallets added the api sync Synchronize API with other bindings label Mar 14, 2024

milyin mentioned this pull request Mar 15, 2024

Sample rework eclipse-zenoh/zenoh#837

Closed

yellowhatter reviewed Mar 21, 2024

View reviewed changes

DenisBiryukov91 reviewed Mar 21, 2024

View reviewed changes

yellowhatter reviewed Mar 26, 2024

View reviewed changes

yellowhatter suggested changes Mar 26, 2024

View reviewed changes

yellowhatter reviewed Mar 26, 2024

View reviewed changes

DenisBiryukov91 mentioned this pull request Mar 28, 2024

implementation_guarded_transmute with from and deref #303

Merged

DenisBiryukov91 mentioned this pull request Apr 8, 2024

Api rework #314

Closed

DenisBiryukov91 mentioned this pull request Apr 8, 2024

Sample/Payload api rework #315

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposal: Sample API rework #251

Proposal: Sample API rework #251

p-avital commented Feb 26, 2024

eclipse-zenoh-bot commented Feb 26, 2024

milyin Mar 1, 2024

Mallets Mar 7, 2024

milyin Mar 7, 2024

p-avital Mar 8, 2024

milyin Mar 7, 2024

yellowhatter Mar 21, 2024 •

edited

sashacmc Mar 22, 2024

DenisBiryukov91 Mar 21, 2024

yellowhatter Mar 22, 2024

DenisBiryukov91 Mar 21, 2024

p-avital Mar 21, 2024 •

edited

DenisBiryukov91 Mar 21, 2024

p-avital Mar 21, 2024

yellowhatter Mar 26, 2024

yellowhatter Mar 26, 2024

yellowhatter Mar 26, 2024

yellowhatter left a comment

yellowhatter Mar 26, 2024

yellowhatter Mar 26, 2024

Proposal: Sample API rework #251

Are you sure you want to change the base?

Proposal: Sample API rework #251

Conversation

p-avital commented Feb 26, 2024

eclipse-zenoh-bot commented Feb 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yellowhatter Mar 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-avital Mar 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yellowhatter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yellowhatter Mar 21, 2024 •

edited

p-avital Mar 21, 2024 •

edited