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.

Sign up for GitHub

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

Jump to bottom

Move static local variable out of templated function #299

Merged
merged 4 commits into from May 17, 2023
Merged

Move static local variable out of templated function #299

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5a3b53c
Move `static` local variable out of templated function
DavisVaughan Nov 11, 2022
6950e51
NEWS bullet
DavisVaughan Nov 11, 2022
ca9fbe4
Make `format_check` happy
DavisVaughan Nov 11, 2022
626963e
Merge branch 'main' into fix/nested-unwind-protect
romainfrancois May 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions NEWS.md
View file
Open in desktop
@@ -1,5 +1,7 @@
# cpp11 (development version)

* Fixed a performance issue related to nested `unwind_protect()` calls (#298).

* Minor performance improvements to the cpp11 protect code. (@kevinushey)

* Silenced an unknown attribute warning specific to the Intel compiler
Expand Down
33 changes: 23 additions & 10 deletions inst/include/cpp11/protect.hpp
View file
Open in desktop
Expand Up @@ -54,20 +54,34 @@ inline void set_option(SEXP name, SEXP value) {
SETCAR(opt, value);
}

inline Rboolean& get_should_unwind_protect() {
inline Rboolean* setup_should_unwind_protect() {
romainfrancois marked this conversation as resolved.
Show resolved Hide resolved
SEXP should_unwind_protect_sym = Rf_install("cpp11_should_unwind_protect");
SEXP should_unwind_protect_sexp = Rf_GetOption1(should_unwind_protect_sym);

if (should_unwind_protect_sexp == R_NilValue) {
// Allocate and initialize once, then let R manage it.
// That makes this a shared global across all compilation units.
should_unwind_protect_sexp = PROTECT(Rf_allocVector(LGLSXP, 1));
SET_LOGICAL_ELT(should_unwind_protect_sexp, 0, TRUE);
detail::set_option(should_unwind_protect_sym, should_unwind_protect_sexp);
UNPROTECT(1);
}

Rboolean* should_unwind_protect =
reinterpret_cast<Rboolean*>(LOGICAL(should_unwind_protect_sexp));
should_unwind_protect[0] = TRUE;
Comment on lines -66 to -68
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was previously overwriting the global option to TRUE every time a new compilation unit needed to initialize its static local variable. I don't think we want to do that. I think we only want to initialize to TRUE the very first time this is created, i.e. in the if branch above.

return reinterpret_cast<Rboolean*>(LOGICAL(should_unwind_protect_sexp));
}

inline Rboolean* access_should_unwind_protect() {
// Setup is run once per compilation unit, but all compilation units
// share the same global option, so each compilation unit's static pointer
// will point to the same object.
static Rboolean* p_should_unwind_protect = setup_should_unwind_protect();
Comment on lines +73 to +77
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function holding the static local variable is no longer templated, so each compilation unit will only have to run setup_should_unwind_protect() 1 time

return p_should_unwind_protect;
}

inline Rboolean get_should_unwind_protect() { return *access_should_unwind_protect(); }

return should_unwind_protect[0];
inline void set_should_unwind_protect(Rboolean should_unwind_protect) {
*access_should_unwind_protect() = should_unwind_protect;
}

} // namespace detail
Expand All @@ -80,12 +94,11 @@ inline Rboolean& get_should_unwind_protect() {
template <typename Fun, typename = typename std::enable_if<std::is_same<
decltype(std::declval<Fun&&>()()), SEXP>::value>::type>
SEXP unwind_protect(Fun&& code) {
static auto should_unwind_protect = detail::get_should_unwind_protect();
if (should_unwind_protect == FALSE) {
if (detail::get_should_unwind_protect() == FALSE) {
return std::forward<Fun>(code)();
}

should_unwind_protect = FALSE;
detail::set_should_unwind_protect(FALSE);

static SEXP token = [] {
SEXP res = R_MakeUnwindCont();
Expand All @@ -95,7 +108,7 @@ SEXP unwind_protect(Fun&& code) {

std::jmp_buf jmpbuf;
if (setjmp(jmpbuf)) {
should_unwind_protect = TRUE;
detail::set_should_unwind_protect(TRUE);
throw unwind_exception(token);
}

Expand All @@ -120,7 +133,7 @@ SEXP unwind_protect(Fun&& code) {
// unset it here before returning the value ourselves.
SETCAR(token, R_NilValue);

should_unwind_protect = TRUE;
detail::set_should_unwind_protect(TRUE);

return res;
}
Expand Down