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

Moving towards fast evaluation (unwindProtect) by default #1227

Closed
7 tasks done
Enchufa2 opened this issue Jul 27, 2022 · 12 comments
Closed
7 tasks done

Moving towards fast evaluation (unwindProtect) by default #1227

Enchufa2 opened this issue Jul 27, 2022 · 12 comments

Comments

@Enchufa2
Copy link
Member

Enchufa2 commented Jul 27, 2022

Unwind protection for fast evaluation of R code at C/C++ level was introduced in R 3.5, and it has been supported as an opt-in since Rcpp 0.12.15 via a define (RCPP_USE_UNWIND_PROTECT) and a plugin (unwindProtect). Now #1225 brings this as a default for the next release:

  • RCPP_USE_UNWIND_PROTECT has no effect anymore.
  • Instead, it can be opted-out by defining RCPP_NO_UNWIND_PROTECT.
  • The associated plugin is deprecated. To avoid errors, it currently shows a warning and has no effect. It could be removed in a future release.

To plan ahead for this eventual removal (and avoid warnings as much as possible meanwhile), we'll try to identify all the packages using the plugin and propose a patch to switch to using RCPP_USE_UNWIND_PROTECT, which is harmless when the feature is enabled by default. This issue will keep track of the process.

  • PPRL (0.3.6) [no repo] [patch emailed 2022-07-27]
Done
@eddelbuettel
Copy link
Member

(Just curious here: should the checkmarks above not be open until the respective PRs have been merged?)

Thanks for filing all those PRs. Our machine is well oiled 😀

@Enchufa2
Copy link
Member Author

Enchufa2 commented Jul 27, 2022

In this case, I used the checkmarks to denote that I need to do something (that is, if they're not checked). When the PRs are merged, we'll see the links above change color. And when they hit CRAN, I'll add a note.

@eddelbuettel
Copy link
Member

Oh, wow, neat! I was unaware of that link trick. I did notice that GitHub started to expand them, I do notice now that they have the open pull request logo. (Still, to me the tick is for final completion. But details schmetails. You are running with this -- and thanks for that -- so your rules.

david-cortes pushed a commit to david-cortes/isotree that referenced this issue Jul 27, 2022
david-cortes added a commit to david-cortes/MatrixExtra that referenced this issue Jul 27, 2022
david-cortes added a commit to david-cortes/outliertree that referenced this issue Jul 27, 2022
david-cortes added a commit to david-cortes/readsparse that referenced this issue Jul 27, 2022
david-cortes added a commit to david-cortes/recometrics that referenced this issue Jul 27, 2022
ctoney added a commit to USDAForestService/FIESTAutils that referenced this issue Jul 27, 2022
@david-cortes
Copy link

Wanted to clarify: if I am using function Rcpp::unwindProtect, do I need the defined macro if I am already including the unwind header?

@Enchufa2
Copy link
Member Author

Enchufa2 commented Jul 27, 2022

If you are just using Rcpp::unwindProtect for your own purposes, then no, you don't need the define (nor the plugin). The define enables fast evaluation via unwindProtect (i.e., whenever Rcpp pokes back into R, this is done via Rcpp_fast_eval instead of Rcpp_eval), so it won't hurt if you add the define, and this will be the default in the next release anyway.

@kevinushey
Copy link
Contributor

If I recall, (and sorry for the lack of context; conference brain) there was one issue with jsonlite (or another package?) regarding how R tryCatch() handlers interacted with the UnwindProtect mechanism. Did we document this distinction anywhere, or do we have a clear path for users who need explicit tryCatch() handling with UnwindProtect? (Or am I misremembering some parts of this?)

@Enchufa2
Copy link
Member Author

It was Jeroen's V8. The code path that was failing required a tryCatch, which Rcpp has been artificially adding for years. The solution was to explicitly add that tryCatch in the proper place, and this is what Jeroen implemented. So we are good to go.

And packages can still call Rcpp_eval directly. That function is not going anywhere.

@kevinushey
Copy link
Contributor

Okay, great. Thanks!

@eddelbuettel
Copy link
Member

Yeah, as I recall Jeroen fixed that promptly and early and it already wasn't an issue on 1.0.8. All good 😸

@david-cortes
Copy link

david-cortes commented Aug 6, 2022

Updated versions now on CRAN for packages isotree, MatrixExtra, outliertree, readsparse, recometrics.

@ctoney
Copy link

ctoney commented Aug 10, 2022

Updated version now on CRAN for package FIESTAutils.

@eddelbuettel
Copy link
Member

This is now fully complete as Rcpp 1.0.10 is now on CRAN. 🎇

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

No branches or pull requests

5 participants