Skip to content
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

Feature: Add support for publishing multiple buffers concatenated together #1736

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

sidhant007
Copy link

@sidhant007 sidhant007 commented Jul 2, 2020

As discussed in my previous PR (which I broke accidentally while force-pushing an unintended commit and subsequently closed that PR)

Motivation: If you want to make a single publish to consist of multiple buffers concatenated together, then instead of concatenating the buffers on the user's end and then passing it to mosquitto_publish the user can call mosquitto_publish_bufs, which will reduce memcpy calls on the user's end.
Example - If the user has 10 buffers each 20 bytes big, then the user will save cost (in terms of both, time and memory) of doing memcpy for a total of 200 bytes.

Signature of the function: int mosquitto_publish_bufs(mosq, mid, topic, buffers, buffers_cnt, qos, retain), where buffers is an array of struct buf, where

struct buf {
   const void *base;
   uint32_t len;
};

^Taking into account the feedback provided on the last PR.

The difference in implementations for users will look like following:

With publish_bufs:

buffer_count = sensor_count + 2;
buffers = malloc(buffer_count * sizeof(struct buf));
buffer[0]->iov_base = header;
buffer[0]->iov_len = strlen(header);
for(i=0; i<sensor_count; i++){
    buffer[i+1]->base = sensor[i];
    buffer[i+1]->len = strlen(sensor[i]);
}
for(i=0;i<4;i++){
	buffers[i].base = message[i];
	buffers[i].len = strlen(message[i]);
}
mosquitto_publish_bufs(..., buffers, buffer_count, ...);
free(buffers);

Without publish_bufs:

payload_len = strlen(header)+strlen(footer);
for(i=0; i<sensor_count, i++){
	payload_len += strlen(sensor[i]);
}
payload = malloc(payloadlen + 1);
memcpy(payload, header, strlen(header));
offset += strlen(header);
for(i=0; i<sensor_count; i++){
	len = strlen(sensor[i]);
	memcpy(&payload[offset], sensor[i], len);
	offset += len;
}
memcpy(&payload[offset], footer, strlen(footer));
mosquitto_publish(..., payload_len, payload, ...);
free(payload);
  • I have defined a new struct buf instead of using struct iovec (present in <sys/uio.h>) because of the feedback provided here of keeping the mosquitto API as consistent as possible across platforms.
  • The PR is big in size because of the tests added, if more convenient to review, then let me know and I can break this into multiple PRs (4 = 1 for feature + 3 for new tests added).
  • The implementation makes mosquitto_publish a special case of mosquitto_publish_bufs to reduce code-duplication. I can make the two separate in-case the maintainer prefers so.
  • The three new tests added (03-publish_bufs-qos0, 03-publish_bufs-qos0-no-payload, 03-publish_bufs-c2b-qos1-disconnect) extend the code-coverage to the new functionality added (primarily when buffers_cnt != 1) and the currently present (03-publish-*) tests still pass confirming that mosquitto_publish_bufs works fine when buffers_cnt == 1.

  • Have you signed the Eclipse Contributor Agreement, using the same email address as you used in your commits?
  • Do each of your commits have a "Signed-off-by" line, with the correct email address? Use "git commit -s" to generate this line for you.
  • If you are contributing a new feature, is your work based off the develop branch?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you successfully run make test with your changes locally?

Allows a message payload to be constructed from multiple independent
buffers by concatenating them together.

Signed-off-by: sidhant007 <[email protected]>
Signed-off-by: sidhant007 <[email protected]>
@sidhant007
Copy link
Author

sidhant007 commented Jul 16, 2020

@ralight, please let me know if you have some feedback for this PR.

@zadewg
Copy link

zadewg commented Jul 31, 2023

I am interested in this PR too. Can we have a review?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants