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

Rethink storage of the RNG #63

Closed
rstub opened this issue Sep 20, 2023 · 2 comments · Fixed by #70
Closed

Rethink storage of the RNG #63

rstub opened this issue Sep 20, 2023 · 2 comments · Fixed by #70

Comments

@rstub
Copy link
Member

rstub commented Sep 20, 2023

In PR #58 it was discussed if it makes sense to change the way the RNG is stored within dqrng.

  • Currently the RNG is stored as dqrng::rng64_t == std::shared_pointer<dqrng::random_64bit_generator> where dqrng::random_64bit_wrapper<RNG> provides the actual storage.
  • Keeping with the standard C++ smart pointers std:unique_ptr might make more sense.
  • Switching to Rcpp::XPtr would simplify the code for sharing the RNG externally. It would make sense the assert the life cycles of XPtr objects are copy-construction on rcpp-devel, c.f. feat: global RNG access #58 (comment).
@rstub
Copy link
Member Author

rstub commented Sep 28, 2023

According to the discussion on rcpp-devel, a Rcpp:XPtr would indeed work quite well. It behaves like a smart pointer that releases the content only when the last XPtr object gets out of scope. So this would be sort of ideal for this use case. Changing dqrng::rng64_t would be a breaking change, but we are doing this in this release anyway. And at least on CRAN I have not seen anybody actually programming against the C++ API or using the header only libraries beyond just including one of the RNGs and and maybe seeding from R.

@LTLA
Copy link
Contributor

LTLA commented Sep 28, 2023

FWIW I use Rcpp::XPtr quite a lot, for example here; which is then passed around all of my C++ functions in the same package; and even across shared library boundaries in different packages, e.g., here. Seems to work pretty well, and at 10k downloads a month, I haven't seen any catastrophic segfaults/bus errors/other weirdness... yet.

The only thing to note is that anyone saving their R session will invalidate the external pointer (obviously). It seems that invalidation yields a NULL pointer so you can check for that and re-init the pointer when necessary, e.g., here.

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 a pull request may close this issue.

2 participants