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.

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

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

Conversation

DavisVaughan
Copy link
Member

@DavisVaughan DavisVaughan commented Aug 9, 2024

I have determined that the unwind_protect()s being utilized in the string proxy assignment operator and the string push_back() operator are doing much more damage than they are worth.

  • No other proxy assignment operator or push_back() operator for other vector types is performing this protection. Not even for lists, which has near identical implementations between SET_VECTOR_ELT() and SET_STRING_ELT().
  • Outside of SET_STRING_ELT() itself, the only thing to look at for "weirdness" would be the implicit conversion operator from r_string to SEXP. But that's a no-op, it just returns the underlying data_, so there is no need for unwind_protect() for that.

Because of the above 2 points, I've determined that removing the unwind_protect()s in the string impl would not make it any less safe than using any other vector type, which seem to have been working fine so far.

[[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;
}
bench::mark(string_proxy_assignment_(), iterations = 100)
#> # A tibble: 1 × 6
#>   expression                      min   median `itr/sec` mem_alloc `gc/sec`
#>   <bch:expr>                 <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl>
#> 1 string_proxy_assignment_()   14.9ms     15ms      66.6     781KB    0.673
bench::mark(string_proxy_assignment_(), iterations = 100)
#> # A tibble: 1 × 6
#>   expression                      min   median `itr/sec` mem_alloc `gc/sec`
#>   <bch:expr>                 <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl>
#> 1 string_proxy_assignment_()    344µs    360µs     2769.     781KB     28.0

[[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;
}
bench::mark(string_push_back_(), iterations = 100)
#> # A tibble: 1 × 6
#>   expression               min   median `itr/sec` mem_alloc `gc/sec`
#>   <bch:expr>          <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl>
#> 1 string_push_back_()   20.5ms   21.8ms      46.1    2.76MB     2.43
bench::mark(string_push_back_(), iterations = 100)
#> # A tibble: 1 × 6
#>   expression               min   median `itr/sec` mem_alloc `gc/sec`
#>   <bch:expr>          <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl>
#> 1 string_push_back_()   5.17ms   5.33ms      185.    2.76MB     9.73

@DavisVaughan DavisVaughan merged commit ba8ea8a into r-lib:main Aug 9, 2024
16 checks passed
@DavisVaughan DavisVaughan deleted the feature/proxy-string-assignment-protection branch August 9, 2024 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant