Skip to content

Update recv.c#32

Open
dddvision wants to merge 3 commits intofagg:masterfrom
dddvision:patch-1
Open

Update recv.c#32
dddvision wants to merge 3 commits intofagg:masterfrom
dddvision:patch-1

Conversation

@dddvision
Copy link
Contributor

Fixed a performance problem when receiving large messages of unknown size. Also, changed the recv() inputs to more closely match the zmq API (without breaking any unit tests). The zmq API requires a buffer allocation for the largest anticipated message (if the buffer is too small, then the message will be truncated). The previous implementation of matlab-zmq recv() triggered allocation/deallocation of the largest anticipated buffer during each call from MATLAB, even if the actual message received was trivially small. With the submitted change, the buffer is declared as a static variable and the memory is (re)allocated using realloc. Finally, mexAtExit is used to register a function to free memory on program termination.

Fixed a performance problem when receiving large messages of unknown size. Also, changed the recv() inputs to more closely match the zmq API (without breaking any unit tests). The zmq API requires a buffer allocation for the largest anticipated message (if the buffer is too small, then the message will be truncated). The previous implementation of matlab-zmq recv() triggered allocation/deallocation of the largest anticipated buffer during each call from MATLAB, even if the actual message received was trivially small. With the submitted change, the buffer is declared as a static variable and the memory is (re)allocated using realloc. Finally, mexAtExit is used to register a function to free memory on program termination.
@abravalheri
Copy link
Collaborator

Hi @dddvision, it seems very good to me.

I am still confused with a point, would you mind to clarify it?

Is there any special reason to use the standard C realloc, free instead of the MathWorks specific (mxRealloc, mxFree), despite of documentation discouraging ANSI C memory management functions?

@dddvision
Copy link
Contributor Author

Hi Anderson,

According to MATLAB documentation...

"mxRealloc registers the allocated memory with the MATLAB memory manager.
When control returns to the MATLAB prompt, the memory manager then
automatically frees, or deallocates, this memory."

If the memory is freed when returning to the MATLAB prompt, then it will
need mxRealloc/mxFree during the next call. Instead, we want the buffer to
persist, so we...

"Use mexAtExit to register a function to call just before clearing the MEX
function or terminating MATLAB."

My reason for using realloc is to offer the caller efficiency in the case
that zmq.core.recv is called repeatedly with the same large maximum buffer
size, even if the message sizes are tiny. Efficiency is not guaranteed by
the C standard, but most compilers will return the same pointer that was
passed in if the new size is equal to the previous size. For example, the
GNU libc documentation says...

"If the new size you specify is the same as the old size, realloc is
guaranteed to change nothing and return the same address that you gave."

I've used C/C++ memory allocation within MEX code for a long time without
problems, mostly because I prefer to write as little MEX-specific code as
possible. However, there is a way to accomplish the same effect with
mxRealloc/mexMakeMemoryPersistent/mxFree. Do you prefer that option?

David

On Fri, Nov 6, 2015 at 1:37 PM, Anderson Bravalheri <
[email protected]> wrote:

Hi @dddvision https://github.com/dddvision, it seems very good to me.

I am still confused with a point, would you mind to clarify it?

Is there any special reason to use the standard C realloc, free instead
of the MathWorks specific (mxRealloc, mxFree), despite of documentation
discouraging ANSI C memory management functions?


Reply to this email directly or view it on GitHub
#32 (comment).

@dddvision
Copy link
Contributor Author

I should also mention that I did not attempt to document the change to the API. The function zmq.core.recv no longer accepts the second argument as a string. This means that the shorthand syntax zmq.core.recv(socket) works the same as before, but you can't call zmq.core.recv(socket, flags). You have to call zmq.core.recv(socket, len) or zmq.core.recv(socket, len, flags).

@abravalheri
Copy link
Collaborator

Thank you for the explanation David, it seems a good reason for me. I've fetched the pull request and played a little bit, it seems to not have a side effect in the overall architecture. I'm in favor of merging, but I prefer let Ashton take care of it, it's been a while since my last contribution...

I'm not that sure about the change in the API. Despite of been more compliant with original C API, it is less expressive. But we have a higher level OO interface, so I think this change will not change the common usage...

-----Original Message-----
From: "David Diel" [email protected]
Sent: ‎06/‎11/‎2015 19:28
To: "fagg/matlab-zmq" [email protected]
Cc: "Anderson Bravalheri" [email protected]
Subject: Re: [matlab-zmq] Update recv.c (#32)

I should also mention that I did not attempt to document the change to the API. The function zmq.core.recv no longer accepts the second argument as a string. This means that the shorthand syntax zmq.core.recv(socket) works the same as before, but you can't call zmq.core.recv(socket, flags). You have to call zmq.core.recv(socket, len) or zmq.core.recv(socket, len, flags).

Reply to this email directly or view it on GitHub.

@dddvision
Copy link
Contributor Author

Hi Anderson, where can I find the higher level API?

On Nov 6, 2015, at 5:56 PM, Anderson Bravalheri [email protected] wrote:

Thank you for the explanation David, it seems a good reason for me. I've fetched the pull request and played a little bit, it seems to not have a side effect in the overall architecture. I'm in favor of merging, but I prefer let Ashton take care of it, it's been a while since my last contribution...

I'm not that sure about the change in the API. Despite of been more compliant with original C API, it is less expressive. But we have a higher level OO interface, so I think this change will not change the common usage...

-----Original Message-----
From: "David Diel" [email protected]
Sent: ‎06/‎11/‎2015 19:28
To: "fagg/matlab-zmq" [email protected]
Cc: "Anderson Bravalheri" [email protected]
Subject: Re: [matlab-zmq] Update recv.c (#32)

I should also mention that I did not attempt to document the change to the API. The function zmq.core.recv no longer accepts the second argument as a string. This means that the shorthand syntax zmq.core.recv(socket) works the same as before, but you can't call zmq.core.recv(socket, flags). You have to call zmq.core.recv(socket, len) or zmq.core.recv(socket, len, flags).

Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHub.

@abravalheri
Copy link
Collaborator

Hi David, sorry for the lack of documentation. The higher level API is composed by Context and Socket classes. This two classes are loosely inspired by the python API design.

@fagg
Copy link
Owner

fagg commented Feb 18, 2016

I need to have a more in depth look at this and test it to make a final decision here. Sorry for the delay.

@dddvision
Copy link
Contributor Author

Ashton,

Thanks for looking into this. I have been using the modified version for
some time now. It would be great to see this fix included in the master.

David

On Wed, Feb 17, 2016 at 8:56 PM, Ashton Fagg [email protected]
wrote:

I need to have a more in depth look at this and test it to make a final
decision here. Sorry for the delay.


Reply to this email directly or view it on GitHub
#32 (comment).

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.

3 participants