Speed up Rcpp_eval()

[krlmlr]

I wonder if it's possible to speed up Rcpp_eval(), e.g., by making use of R_ErrorHook. The current implementation seems to wrap an evalq call in a tryCatch at the R level. I understand that we can't simply use Rf_eval() because our destructors won't be called.

Motivation: Introducing Rcpp_eval() instead of Rf_eval() has made dplyr one order of magnitude slower for some operations:

Here's the offending commit: tidyverse/dplyr@7ea10. The plot shows only an excerpt of the whole history, but the weak performance is still present in dplyr's current master.

Related: tidyverse/dplyr#2220.

CC @hadley @kevinushey @romainfrancois.

[romainfrancois]

There are definitely ways to make it faster. @kevinushey found this; https://twitter.com/kevin_ushey/status/776129880161263616

this is where I'd look. The current implementation of Rcpp_eval has been silly from the beginning, and has become more and more convoluted over the years, because of the lack of support from R for that kind of stuff.

[eddelbuettel]

See https://github.com/wch/r-source/blob/trunk/src/main/errors.c#L1985-L1989 :

/* This implementation uses R's tryCatch via calls from C to R to
invoke R's tryCatch, and then back to C to infoke the C
body/handler functions via a .Internal helper. This makes the
implementation fairly simple but not fast. If performance becomes
an issue we can look into a pure C implementation. LT */

We all looked that when the patch was added. Likely no silver bullet yet.

[krlmlr]

I just saw that R_ErrorHook is about 15 years old and has never been part of the API.

We could start by conditionally enabling the new C-level try-catch on R 3.4.0. But I can evaluate performance impact locally in dplyr.

Keeping this open as a reminder.

[eddelbuettel]

I think you are still misreading. There is no C-level try-catch in R 3.4.0. I quoted a part of that patch where Luke Tierney pretty plainly states that it is C to R back to C, and not fast.

[krlmlr]

I was referring to the entry point which is at C-level, if I got it right. Making that fast would be R core's job, but we probably still need to use that new API.

[eddelbuettel]

There is at present nothing actionable here. We have a promised of work conditional on an as-of-yet unreleased R version.

My suggestion is to close this.

[jjallaire]

In all cases we would need to ensure that Rcpp doesn't take on a new
version requirement (as we are dragging ~ 2/3 of CRAN with us every time we
bump our required R version). Enabling conditionally as you said would be
the right approach.

On Mon, Nov 7, 2016 at 8:40 AM, Kirill Müller notifications@github.com
wrote:

I was referring to the entry point which is at C-level, if I got it right.
Making that fast would be R core's job, but we probably still need to use
that new API.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#578 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAGXx-7IbNwoHc7pI0Zox67sO9zekBO_ks5q7ypWgaJpZM4KrJKO
.

[eddelbuettel]

Also of note is that in the last few months we have accepted two PRs which were of less than ideal quality and both needed subsequent PRs for cleanup. So I'd rather not "jump" into messing with something critical. This sounds like a topic which could be explored first in a pure client-side package, and bubbled up if and when we are sure it is ready.

[kevinushey]

Agree with the general consensus that there's nothing much we can do at this point. :/