Skip to content

Commit

Permalink
Remove very expensive unwind_protect() in string proxy assignment a…
Browse files Browse the repository at this point in the history
…nd push back (#378)

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

* NEWS bullet
  • Loading branch information
DavisVaughan authored Aug 9, 2024
1 parent dd78834 commit ba8ea8a
Show file tree
Hide file tree
Showing 5 changed files with 69 additions and 2 deletions.
6 changes: 6 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
@@ -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).

Expand Down
8 changes: 8 additions & 0 deletions cpp11test/R/cpp11.R
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
16 changes: 16 additions & 0 deletions cpp11test/src/cpp11.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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},
Expand Down
35 changes: 35 additions & 0 deletions cpp11test/src/strings.cpp
Original file line number Diff line number Diff line change
@@ -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;
}
6 changes: 4 additions & 2 deletions inst/include/cpp11/strings.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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_;
}

Expand Down

0 comments on commit ba8ea8a

Please sign in to comment.