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

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

Closed
DavisVaughan opened this issue Nov 11, 2022 · 1 comment · Fixed by #299
Closed

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

DavisVaughan opened this issue Nov 11, 2022 · 1 comment · Fixed by #299

Comments

@DavisVaughan
Copy link
Member

DavisVaughan commented Nov 11, 2022

I believe that the unwind_protect() optimization originally added in #207 isn't quite right. I think it was broken after that PR was merged in 9a62c3a with the move to a local static object. I can prove that nested calls to unwind_protect() aren't being skipped as they should be.

First, here is a benchmark to show that the nested unwind_protect() optimization isn't working, and the performance loss is pretty brutal. The unwind_protect() calls happen on every iteration of the loop, because assigning an r_string to a writable::strings proxy element is wrapped in unwind_protect(), even though I wrapped the whole loop in unwind_protect() too.

unwind_protect([&] { SET_STRING_ELT(data_, index_, rhs); });

set.seed(123)
x <- sample(letters, 1e6, replace = TRUE)

cpp11::cpp_function("

cpp11::writable::strings test(cpp11::strings x) {
  R_xlen_t size = x.size();
  
  cpp11::writable::strings out(size);
  
  cpp11::unwind_protect([&] {
    for (R_xlen_t i = 0; i < size; ++i) {
      out[i] = x[i];
    }
  });
  
  return out;
}

")

cpp11::cpp_function("

SEXP test2(SEXP x) {
  R_xlen_t size = Rf_xlength(x);
  
  SEXP out = PROTECT(Rf_allocVector(STRSXP, size));
  
  for (R_xlen_t i = 0; i < size; ++i) {
    SET_STRING_ELT(out, i, STRING_ELT(x, i));
  }
  
  UNPROTECT(1);
  return out;
}

")

bench::mark(test(x), test2(x), iterations = 20)
#> Warning: Some expressions had a GC in every iteration; so filtering is disabled.
#> # A tibble: 2 × 6
#>   expression      min   median `itr/sec` mem_alloc `gc/sec`
#>   <bch:expr> <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl>
#> 1 test(x)    715.35ms 751.55ms      1.31    7.63MB     1.96
#> 2 test2(x)     7.65ms   9.11ms    100.      7.63MB    35.1

Next, here is a video that shows how inner calls to unwind_protect() aren't exiting early. It uses VS Code to step through the example from above. The sequence of steps are:

  • We step through the outer unwind_protect() call to see how it sets up the static should_unwind_protect local object
  • Then we step through the first iteration of the out[i] = x[i] copy. That puts us back in unwind_protect(), but that static local object is not yet initialized (weird right?), so we end up going back through the process of setting it up. That means unwind_protect() doesn't early exit here when it should.
  • Then on the second iteration of out[i] = x[i], the static local should_unwind_protect is set up, but is set to TRUE so we again can't early exit. This continues on for the rest of the loop.

https://vimeo.com/769942052

I'm fairly certain the main problem here is that SEXP unwind_protect(Fun&& code) is a template function, meaning that each new instantiation of it gets its own copy of should_unwind_protect, and it has to be set up every time.

I've fixed this in a branch I'll send in, and I'll explain the fix there.

jimhester referenced this issue Nov 16, 2022
It seems relying on non-local static objects can cause memory issues,
that are resolved by moving to use a local static objects instead.

Fixes #244
@jimhester
Copy link
Member

Made a comment in 9a62c3a#r90124412

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 a pull request may close this issue.

2 participants