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

add xdr argument #6

Merged
merged 5 commits into from
May 11, 2024
Merged

add xdr argument #6

merged 5 commits into from
May 11, 2024

Conversation

traversc
Copy link
Contributor

@traversc traversc commented May 11, 2024

Hi Dirk, what do you think about adding an argument for choosing between XDR and binary formats? From my testing serialization using binary fromat is about 3.5x faster.

Comparison

library(Rcpp)
sourceCpp(code="#include <Rcpp.h>
#include "RApiSerializeAPI.h"
// [[Rcpp::depends(RApiSerialize)]]
// [[Rcpp::export(rng=false)]]
SEXP rapi_serializeToRaw(SEXP x, SEXP use_xdrSexp) {
  return serializeToRaw(x, R_NilValue, use_xdrSexp);
}
// [[Rcpp::export(rng=false)]]
SEXP rapi_unserializeFromRaw(SEXP x) {
  return unserializeFromRaw(x);
}")

x <- 1:1e8/2
system.time(out1 <- rapi_serializeToRaw(x, TRUE)) # use XDR
# user  system elapsed
# 0.608   0.069   0.676
system.time(out2 <- rapi_serializeToRaw(x, FALSE)) # use Binary
# user  system elapsed
# 0.104   0.081   0.186

system.time(y1 <- rapi_unserializeFromRaw(out1)) # use XDR
# user  system elapsed
# 0.508   0.051   0.558
system.time(y2 <- rapi_unserializeFromRaw(out2)) # use Binary
# user  system elapsed
# 0.507   0.050   0.557

identical(y1, y2) # TRUE

@eddelbuettel
Copy link
Owner

Love it. Saw the discussion on the list too.

Copy link
Owner

@eddelbuettel eddelbuettel left a comment

Choose a reason for hiding this comment

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

This looks good. Left one minor comment.

Thinking it could do with a test. Should we add one? Maybe just extending tests/simpleTests.R (and its reference output) ?

DESCRIPTION Outdated
@@ -1,8 +1,8 @@
Package: RApiSerialize
Type: Package
Title: R API Serialization
Version: 0.1.2
Date: 2022-08-25
Version: 0.1.3
Copy link
Owner

Choose a reason for hiding this comment

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

Let's make this 0.1.2.1, please. I reserve 'a.b.c' versions for actual uploads I trigger.

@@ -210,13 +211,23 @@ extern "C" SEXP serializeToRaw(SEXP object, SEXP versionSexp = R_NilValue) {
version = Rf_asInteger(versionSexp);
}
if (version == NA_INTEGER || version <= 0) {
Rf_error("bad version value");
Rf_error("bad version value");
Copy link
Owner

Choose a reason for hiding this comment

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

Actually the rest of the file uses four spaces indentation.

@eddelbuettel
Copy link
Owner

I will just make the changes. Also needs a new argument at the R caller.

@eddelbuettel
Copy link
Owner

Ok, there were one or two little niggles missing I took care of (along with the one nag at CRAN about R_NO_REMAP).

This should be ready for a new CRAN version now. Let me know your thoughts.

@traversc
Copy link
Contributor Author

Looks good! :)

@eddelbuettel eddelbuettel merged commit b5c15cd into eddelbuettel:master May 11, 2024
1 check passed
@eddelbuettel
Copy link
Owner

I shipped it to CRAN a few hours ago, but I suspect they may have an issue with their test runners as we should long have heard back from the few reverse depends. I guess we will hear by tomorrow.

@eddelbuettel
Copy link
Owner

Booo. Was out for a nice bike ride and as I get back I am greeted by Uwe with a 'change to worse' for.... wait for it..... qs.

@eddelbuettel
Copy link
Owner

Fixed the blunder and re-uploaded. Because the existing package is tagged 'NoRemap' it requires a manual check which may happen early tomorrow during European morning hours.

@eddelbuettel
Copy link
Owner

And as expected the package is now at CRAN. Yay.

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