Remove very expensive unwind_protect() in string proxy assignment and push back (#378)

* Remove very expensive `unwind_protect()` in string proxy assignment and push back

* NEWS bullet

Author: DavisVaughan

NEWS.md

@@ -1,5 +1,11 @@
 # cpp11 (development version)
 
+* Repeated assignment to a `cpp11::writable::strings` vector through either
+  `x[i] = elt` or `x.push_back(elt)` is now more performant, at the tradeoff
+  of slightly less safety (as long as `elt` is actually a `CHARSXP` and `i` is
+  within bounds, there is no chance of failure, which are the same kind of
+  invariants placed on the other vector types) (#378).
+
 * Read only `r_vector`s now have a move constructor and move assignment
   operator (#365).
 

cpp11test/R/cpp11.R

@@ -160,6 +160,14 @@ cpp11_safe_ <- function(x_sxp) {
   .Call(`_cpp11test_cpp11_safe_`, x_sxp)
 }
 
+string_proxy_assignment_ <- function() {
+  .Call(`_cpp11test_string_proxy_assignment_`)
+}
+
+string_push_back_ <- function() {
+  .Call(`_cpp11test_string_push_back_`)
+}
+
 sum_dbl_for_ <- function(x) {
   .Call(`_cpp11test_sum_dbl_for_`, x)
 }

cpp11test/src/cpp11.cpp

@@ -310,6 +310,20 @@ extern "C" SEXP _cpp11test_cpp11_safe_(SEXP x_sxp) {
     return cpp11::as_sexp(cpp11_safe_(cpp11::as_cpp<cpp11::decay_t<SEXP>>(x_sxp)));
   END_CPP11
 }
+// strings.cpp
+cpp11::writable::strings string_proxy_assignment_();
+extern "C" SEXP _cpp11test_string_proxy_assignment_() {
+  BEGIN_CPP11
+    return cpp11::as_sexp(string_proxy_assignment_());
+  END_CPP11
+}
+// strings.cpp
+cpp11::writable::strings string_push_back_();
+extern "C" SEXP _cpp11test_string_push_back_() {
+  BEGIN_CPP11
+    return cpp11::as_sexp(string_push_back_());
+  END_CPP11
+}
 // sum.cpp
 double sum_dbl_for_(cpp11::doubles x);
 extern "C" SEXP _cpp11test_sum_dbl_for_(SEXP x) {
@@ -504,6 +518,8 @@ static const R_CallMethodDef CallEntries[] = {
     {"_cpp11test_rcpp_sum_int_for_",        (DL_FUNC) &_cpp11test_rcpp_sum_int_for_,        1},
     {"_cpp11test_remove_altrep",            (DL_FUNC) &_cpp11test_remove_altrep,            1},
     {"_cpp11test_row_sums",                 (DL_FUNC) &_cpp11test_row_sums,                 1},
+    {"_cpp11test_string_proxy_assignment_", (DL_FUNC) &_cpp11test_string_proxy_assignment_, 0},
+    {"_cpp11test_string_push_back_",        (DL_FUNC) &_cpp11test_string_push_back_,        0},
     {"_cpp11test_sum_dbl_accumulate2_",     (DL_FUNC) &_cpp11test_sum_dbl_accumulate2_,     1},
     {"_cpp11test_sum_dbl_accumulate_",      (DL_FUNC) &_cpp11test_sum_dbl_accumulate_,      1},
     {"_cpp11test_sum_dbl_for2_",            (DL_FUNC) &_cpp11test_sum_dbl_for2_,            1},

cpp11test/src/strings.cpp

@@ -0,0 +1,35 @@
+#include "cpp11/strings.hpp"
+
+// Test benchmark for string proxy assignment performance.
+// We don't unwind_protect() before each `SET_STRING_ELT()` call,
+// as that kills performance.
+[[cpp11::register]] cpp11::writable::strings string_proxy_assignment_() {
+  R_xlen_t size = 100000;
+
+  cpp11::writable::strings x(size);
+
+  cpp11::r_string elt(NA_STRING);
+
+  for (R_xlen_t i = 0; i < size; ++i) {
+    x[i] = elt;
+  }
+
+  return x;
+}
+
+// Test benchmark for string push back performance.
+// We don't unwind_protect() before each `SET_STRING_ELT()` call,
+// as that kills performance.
+[[cpp11::register]] cpp11::writable::strings string_push_back_() {
+  R_xlen_t size = 100000;
+
+  cpp11::writable::strings x;
+
+  cpp11::r_string elt(NA_STRING);
+
+  for (R_xlen_t i = 0; i < size; ++i) {
+    x.push_back(elt);
+  }
+
+  return x;
+}

inst/include/cpp11/strings.hpp

@@ -67,7 +67,8 @@ inline void r_vector<r_string>::set_elt(
 template <>
 inline typename r_vector<r_string>::proxy& r_vector<r_string>::proxy::operator=(
     const r_string& rhs) {
-  unwind_protect([&] { SET_STRING_ELT(data_, index_, rhs); });
+  // NOPROTECT: likely too costly to unwind protect every elt
+  SET_STRING_ELT(data_, index_, rhs);
   return *this;
 }
 
@@ -169,7 +170,8 @@ inline void r_vector<r_string>::push_back(r_string value) {
   while (length_ >= capacity_) {
     reserve(capacity_ == 0 ? 1 : capacity_ *= 2);
   }
-  unwind_protect([&] { SET_STRING_ELT(data_, length_, value); });
+  // NOPROTECT: likely too costly to unwind protect every elt
+  SET_STRING_ELT(data_, length_, value);
   ++length_;
 }