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

Internal usage of Rcpp_fast_eval in Rcpp #866

Closed
thirdwing opened this Issue Jun 10, 2018 · 4 comments

Comments

Projects
None yet
3 participants
@thirdwing
Copy link
Member

thirdwing commented Jun 10, 2018

This has been discussed on the Rcpp mailing list: http://lists.r-forge.r-project.org/pipermail/rcpp-devel/2018-June/010029.html

Hi all,

I've followed with interest the development of the new evaluation API.
Now that it's finally merged, I was testing it. Perhaps I'm mistaken,
but shouldn't we expect a performance improvement in code such as the
following?

Rcpp::sourceCpp(code='
  #include <Rcpp.h>
  using namespace Rcpp;

  // [[Rcpp::export]]
  void old_api(Function func, int n) {
    for (int i=0; i<n; i++) func();
  }'
)

Rcpp::sourceCpp(code='
  #define RCPP_PROTECTED_EVAL
  #include <Rcpp.h>
  using namespace Rcpp;

  // [[Rcpp::export]]
  void new_api(Function func, int n) {
    for (int i=0; i<n; i++) func();
  }'
)

func <- function() 1
system.time(old_api(func, 1e5))
system.time(new_api(func, 1e5))

Regards,
Iñaki
@thirdwing

This comment has been minimized.

Copy link
Member Author

thirdwing commented Jun 10, 2018

This is because the Rcpp Function is still using the old Rcpp_eval: https://github.com/RcppCore/Rcpp/blob/master/inst/include/Rcpp/Function.h#L82

After changing to Rcpp_fast_eval in Rcpp Function, the timing result on my laptop is as follow:

> system.time(old_api(func, 1e5))
   user  system elapsed
  1.025   0.000   1.025
> system.time(new_api(func, 1e5))
   user  system elapsed
  0.025   0.000   0.025 

Should we port all the internal usage of Rcpp_eval in Rcpp to Rcpp_fast_eval for better performance? @kevinushey

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Jun 10, 2018

We could try in a branch and run a reverse depends over it.

@Enchufa2

This comment has been minimized.

Copy link
Contributor

Enchufa2 commented Jun 10, 2018

I just tested this with simmer and I get an impressive x4 performance improvement in a reference test I have (!), so I'm very interested in these changes. Therefore, if no one is leading this already, I'd be more than glad to start a PR and check it against different R versions via rocker. I don't have the means to run such a heavy revdep check in a reasonable time though.

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Jun 10, 2018

A PR would be great! I think we all would be happy to review / chip in.

I can do the rev.dep across all (first-level) dependencies; that is set up. It would be nice if you could help with a simpler R CMD check across older R releases; that is not something I have scripted yet here. And for what it is worth my main machine is still on R 3.4.4 so I can cover 'oldrel' ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.