Support more efficient nesting of unwind_protect #207

jimhester · 2021-07-19T16:09:06Z

The internal protection can be skipped if we are already in an
unwind_protect call.

Fixes #141

The internal protection can be skipped if we are already in an unwind_protect call. Fixes #141

jimhester · 2021-07-19T16:10:15Z

inst/include/cpp11/protect.hpp

+    }
+#endif
+  }
+
+  void release(SEXP token) {
+    if (token == R_NilValue) {
+      return;
+    }
+
+#ifdef CPP11_USE_PRESERVE_OBJECT
+    R_ReleaseObject(token);
+    return;
+#endif
+
+    SEXP before = CAR(token);
+
+    SEXP after = CDR(token);
+
+    if (before == R_NilValue && after == R_NilValue) {
+      Rf_error("should never happen");
+    }
+
+    SETCDR(before, after);
+
+    if (after != R_NilValue) {
+      SETCAR(after, before);
+    }
+  }
+
+ private:
+  // We deliberately avoid using safe[] in the below code, as this code runs
+  // when the shared library is loaded and will not be wrapped by
+  // `CPP11_UNWIND`, so if an error occurs we will not catch the C++ exception
+  // that safe emits.
+  static void set_option(SEXP name, SEXP value) {
+    static SEXP opt = SYMVALUE(Rf_install(".Options"));
+    SEXP t = opt;
+    while (CDR(t) != R_NilValue) {
+      if (TAG(CDR(t)) == name) {
+        opt = CDR(t);
+        SET_TAG(opt, name);
+        SETCAR(opt, value);
+        return;
+      }
+      t = CDR(t);
+    }
+    SETCDR(t, Rf_allocList(1));
+    opt = CDR(t);
+    SET_TAG(opt, name);
+    SETCAR(opt, value);
+  }
+
+  // The list_ singleton is stored in a XPtr within an R global option.
+  //
+  // It is not constructed as a static variable directly since many
+  // translation units may be compiled, resulting in unrelated instances of each
+  // static variable.
+  //
+  // We cannot store it in the cpp11 namespace, as cpp11 likely will not be loaded by
+  // packages.
+  // We cannot store it in R's global environment, as that is against CRAN
+  // policies.
+  // We instead store it as an XPtr in the global options, which avoids issues
+  // both copying and serializing.
+  static SEXP get_preserve_xptr_addr() {
+    static SEXP preserve_xptr_sym = Rf_install("cpp11_preserve_xptr");
+    SEXP preserve_xptr = Rf_GetOption1(preserve_xptr_sym);
+
+    if (TYPEOF(preserve_xptr) != EXTPTRSXP) {
+      return R_NilValue;
+    }
+    auto addr = R_ExternalPtrAddr(preserve_xptr);
+    if (addr == nullptr) {
+      return R_NilValue;
+    }
+    return static_cast<SEXP>(addr);
+  }
+
+  static void set_preserve_xptr(SEXP value) {
+    static SEXP preserve_xptr_sym = Rf_install("cpp11_preserve_xptr");
+
+    SEXP xptr = PROTECT(R_MakeExternalPtr(value, R_NilValue, R_NilValue));
+    set_option(preserve_xptr_sym, xptr);
+    UNPROTECT(1);
+  }
+
+  static SEXP get_preserve_list() {
+    static SEXP preserve_list = R_NilValue;
+
+    if (TYPEOF(preserve_list) != LISTSXP) {
+      preserve_list = get_preserve_xptr_addr();
+      if (TYPEOF(preserve_list) != LISTSXP) {
+        preserve_list = Rf_cons(R_NilValue, R_NilValue);
+        R_PreserveObject(preserve_list);
+        set_preserve_xptr(preserve_list);
+      }
+    }
+
+    return preserve_list;
+  }


All of this code was just moved here from lower down in the file, because it needs to be defined before the unwind_protect() calls below

jimhester · 2021-07-19T16:19:53Z

inst/include/cpp11/protect.hpp

+    return preserve_list;
+  }
+
+  static int* get_should_unwind_protect() {


Here we define a global logical sexp to track if we are inside an unwind protect call.

We need to use a singleton managed by R rather than a global in the header file so we don't have multiple global variables in each compilation unit.

It is possible we should move this code outside of this class, I put it here for convenience, since we were already using this same strategy for the preserve list.

jimhester · 2021-07-19T16:21:09Z

inst/include/cpp11/protect.hpp

@@ -42,6 +221,7 @@ SEXP unwind_protect(Fun&& code) {

  std::jmp_buf jmpbuf;
  if (setjmp(jmpbuf)) {
+    *preserved.should_unwind_protect_ = TRUE;


This one is important, otherwise we don't correctly set the flag after there is an error.

jimhester · 2021-07-19T20:23:30Z

inst/include/cpp11/protect.hpp

+  SEXP list_ = get_preserve_list();
+
+ public:
+  int* should_unwind_protect_ = get_should_unwind_protect();


Suggested change

int* should_unwind_protect_ = get_should_unwind_protect();

R_Boolean* should_unwind_protect_ = get_should_unwind_protect();

inst/include/cpp11/protect.hpp

jimhester · 2021-07-19T20:30:35Z

inst/include/cpp11/protect.hpp

@@ -34,6 +211,13 @@ class unwind_exception : public std::exception {
 template <typename Fun, typename = typename std::enable_if<std::is_same<
                            decltype(std::declval<Fun&&>()()), SEXP>::value>::type>
 SEXP unwind_protect(Fun&& code) {
+  REprintf("%s\n", *preserved.should_unwind_protect_ == TRUE ? "TRUE" : "FALSE");


Cleans up the user code a bit

jimhester · 2021-07-20T13:15:29Z

@sbearrows could you look this over, it should be a bit simpler than what we reviewed together yesterday.

I decided not to write an additional test for this, as it doesn't seem obvious to me how you would test it.

sbearrows

LGTM!

jimhester · 2021-07-21T14:13:20Z

Thanks for reviewing!

Support more efficient nesting of unwind_protect

40ec0a7

The internal protection can be skipped if we are already in an unwind_protect call. Fixes #141

jimhester commented Jul 19, 2021

View reviewed changes

Lookup the should_protect option from a global option

620bd17

jimhester commented Jul 19, 2021

View reviewed changes

inst/include/cpp11/protect.hpp Show resolved Hide resolved

jimhester commented Jul 19, 2021

View reviewed changes

jimhester added 4 commits July 20, 2021 08:43

Do not put the should_unwind_protect flag in the protected struct

b50bb37

Remove debugging code

a451c1a

Use a reference rather than a pointer

9b9be8c

Cleans up the user code a bit

Whitespace

5a38c26

jimhester requested a review from sbearrows July 20, 2021 13:14

sbearrows reviewed Jul 20, 2021

View reviewed changes

jimhester merged commit 46ee0bb into master Jul 21, 2021

jimhester deleted the nested_unwind_protect branch July 21, 2021 14:13

DavisVaughan mentioned this pull request Nov 11, 2022

Nested unwind_protect() optimization isn't working correctly #298

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support more efficient nesting of unwind_protect #207

Support more efficient nesting of unwind_protect #207

jimhester commented Jul 19, 2021

jimhester Jul 19, 2021

jimhester Jul 19, 2021

jimhester Jul 19, 2021

jimhester Jul 19, 2021

jimhester Jul 19, 2021 •

edited

Loading

jimhester Jul 19, 2021

jimhester commented Jul 20, 2021

sbearrows left a comment

jimhester commented Jul 21, 2021

	int* should_unwind_protect_ = get_should_unwind_protect();
	R_Boolean* should_unwind_protect_ = get_should_unwind_protect();

Support more efficient nesting of unwind_protect #207

Support more efficient nesting of unwind_protect #207

Conversation

jimhester commented Jul 19, 2021

jimhester Jul 19, 2021

Choose a reason for hiding this comment

jimhester Jul 19, 2021

Choose a reason for hiding this comment

jimhester Jul 19, 2021

Choose a reason for hiding this comment

jimhester Jul 19, 2021

Choose a reason for hiding this comment

jimhester Jul 19, 2021 • edited Loading

Choose a reason for hiding this comment

jimhester Jul 19, 2021

Choose a reason for hiding this comment

jimhester commented Jul 20, 2021

sbearrows left a comment

Choose a reason for hiding this comment

jimhester commented Jul 21, 2021

jimhester Jul 19, 2021 •

edited

Loading