ropensci / rzmq

R package for ZMQ
https://docs.ropensci.org/rzmq
84 stars 30 forks source link

Reuse messages in rzmq #39

Closed mschubert closed 7 years ago

mschubert commented 7 years ago

This is a pull request for https://github.com/ropensci/rzmq/issues/38.

It contains the two new functions (and their Rcpp equivalents):

It further makes the following (backwards-compatible) API changes:

These changes allow the user to send messages multiple times without copying their memory after the message is initialized. Further, they are required to fix https://github.com/mschubert/clustermq/issues/19 and #47 (in short: big messages that are sent repeatedly are not collected quickly enough by the gc, which may cause R to take up the combined memory - in my case sometimes over 30 GB for 300 MB data).

For the message object itself, I have checked appropriate memory release with valgrind, with no reported leaks.

codecov-io commented 7 years ago

Codecov Report

Merging #39 into master will decrease coverage by 2.35%. The diff coverage is 8.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #39      +/-   ##
==========================================
- Coverage   36.17%   33.82%   -2.36%     
==========================================
  Files           4        4              
  Lines         633      680      +47     
==========================================
+ Hits          229      230       +1     
- Misses        404      450      +46
Impacted Files Coverage Δ
R/rzmq.R 20.96% <20%> (-1.45%) :arrow_down:
src/interface.cpp 28.79% <7.5%> (-2.13%) :arrow_down:
inst/zmq.hpp 62.61% <0%> (-3.07%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 4d2e6d9...96046cd. Read the comment docs.

mschubert commented 7 years ago

@armstrtw @jeroen would be great if you could take a look!

jeroen commented 7 years ago

I'm going to leave this up to @armstrtw to review, he is more familiar with this code.

armstrtw commented 7 years ago

looks good. you've tested on your use cases?

@jeroen what's the deal w/ the codecov tests? I'm not really familiar w/ those.

mschubert commented 7 years ago

Yes, I've tested for my package and the memory issue is gone. I can add a unit test for usage if required.

Codecov I think is just the decreased test coverage because I provided code without a unit test (but most of the API doesn't have them).

armstrtw commented 7 years ago

great if you can add a unit test later.

thanks for the contribution.

mschubert commented 7 years ago

Great, thank you!

@jeroen, can you push to CRAN so I can release my updated pkg?

jeroen commented 7 years ago

I cannot release because your commit https://github.com/ropensci/rzmq/commit/23b639511f5785d83234a4ea17c537533d56ceee has broken the build for windows. See install log. Can you fix this?

jeroen commented 7 years ago

I think you can simply revert that commit? The default SIGWINCH is always to ignore.

mschubert commented 7 years ago

When I sent the patch SIGWINCH was not ignored on Linux (and I'm pretty sure things haven't changed). I think a better option would be to #ifdef it, but I'll test this and get back to you tomorrow.

I'm surprised that it worked on Appveyor and failed on CRAN now.

jeroen commented 7 years ago

I'm surprised about that too. I think it might have to do with the fact that CRAN runs a relatively old version of windows (server 2008) and travis uses a more recent version.

jeroen commented 7 years ago

Can you make sure the package passes on the CRAN win builder ?

jeroen commented 7 years ago

On cran now.

mschubert commented 7 years ago

Great, thank you!