From b2ba1316609abc59265d6616639c5213da117126 Mon Sep 17 00:00:00 2001
From: Davis Vaughan <davis@posit.co>
Date: Fri, 9 Aug 2024 14:13:48 -0400
Subject: [PATCH 1/2] Remove very expensive `unwind_protect()` in string proxy
 assignment and push back

---
 cpp11test/R/cpp11.R            |  8 ++++++++
 cpp11test/src/cpp11.cpp        | 16 ++++++++++++++++
 cpp11test/src/strings.cpp      | 35 ++++++++++++++++++++++++++++++++++
 inst/include/cpp11/strings.hpp |  6 ++++--
 4 files changed, 63 insertions(+), 2 deletions(-)
 create mode 100644 cpp11test/src/strings.cpp

diff --git a/cpp11test/R/cpp11.R b/cpp11test/R/cpp11.R
index bb3153e3..038e7b76 100644
--- a/cpp11test/R/cpp11.R
+++ b/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)
 }
diff --git a/cpp11test/src/cpp11.cpp b/cpp11test/src/cpp11.cpp
index 7cfa36c3..421de637 100644
--- a/cpp11test/src/cpp11.cpp
+++ b/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},
diff --git a/cpp11test/src/strings.cpp b/cpp11test/src/strings.cpp
new file mode 100644
index 00000000..1db7736b
--- /dev/null
+++ b/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;
+}
diff --git a/inst/include/cpp11/strings.hpp b/inst/include/cpp11/strings.hpp
index 64cee045..3a574e08 100644
--- a/inst/include/cpp11/strings.hpp
+++ b/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_;
 }
 

From eec9b13ff905af7a7f08da3efd0b445b1eee951b Mon Sep 17 00:00:00 2001
From: Davis Vaughan <davis@posit.co>
Date: Fri, 9 Aug 2024 14:14:23 -0400
Subject: [PATCH 2/2] NEWS bullet

---
 NEWS.md | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/NEWS.md b/NEWS.md
index 2c486c35..fcb31e1b 100644
--- a/NEWS.md
+++ b/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).