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

reduce interrupt frequency in ray_shade #18

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@brodieG

brodieG commented Oct 24, 2018

I'm working on a response to Wolf Vollprecht's Next Journal article, and I noticed when running benchmarks that ray_trace runs substantially slower than the Julia version. I didn't think this should be the case, so I dug into it and noticed that Rcpp::checkUserInterrupt() is called in the innermost for loop. This is a somewhat expensive operation and can be just as effective if we check every few thousand loops.

Making the change increases performance by 5x (see below). Presumably we could make a similar change to the multi core version, and to other places were interrupts are used, but I haven't checked that. I figure you might have opinions about how to do this, so I didn't want to spend too much time changing things before getting a sense for what you want.

I did also make a change to NEWS, but only because I've been asked to that in pull requests into other projects. Feel free to edit as you see fit (obviously).

Before changes:

library(rayshader)
elev <- seq(-90, 90, length=25)
microbenchmark::microbenchmark(times=10,
  ray_shade(volcano, elev, 45, lambert=FALSE, progbar=FALSE)
)
## Unit: milliseconds
##                                                            expr      min
##  ray_shade(volcano, elev, 45, lambert = FALSE, progbar = FALSE) 134.2335
##        lq     mean   median       uq      max neval
##  135.0545 162.9438 143.9956 177.8761 242.3845    10

After changes:

library(rayshader)
elev <- seq(-90, 90, length=25)
microbenchmark::microbenchmark(times=10,
  ray_shade(volcano, elev, 45, lambert=FALSE, progbar=FALSE)
)
## Unit: milliseconds
##                                                            expr     min
##  ray_shade(volcano, elev, 45, lambert = FALSE, progbar = FALSE) 25.9512
##        lq     mean   median       uq      max neval
##  26.52456 28.63295 27.44805 27.87292 41.60245    10
@tylermorganwall

This comment has been minimized.

Owner

tylermorganwall commented Oct 25, 2018

Thanks for the bug report! I just pushed an update that moves the check to the outer loop.

@tylermorganwall

This comment has been minimized.

Owner

tylermorganwall commented Oct 25, 2018

I didn't pull your request (moving the call to the inner loop was all that was required), but I did include you on the NEWS for the update.

@brodieG

This comment has been minimized.

brodieG commented Oct 25, 2018

No problem. I started off with that change first, but then wanted something that you could use for both this and the multicore version which doesn't loop through rows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment