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

Copy constructor for writable vectors is duplicating the SEXP too many times #369

Closed
DavisVaughan opened this issue Aug 7, 2024 · 0 comments · Fixed by #383
Closed

Copy constructor for writable vectors is duplicating the SEXP too many times #369

DavisVaughan opened this issue Aug 7, 2024 · 0 comments · Fixed by #383

Comments

@DavisVaughan
Copy link
Member

As of #362, we always have to do an allocation to truncate before returning a SEXP from a writable we've pushed to with push_back(). That results in one extra allocation in this copy constructor some of the time:

inline r_vector<T>::r_vector(const r_vector<T>& rhs)
: cpp11::r_vector<T>(safe[Rf_shallow_duplicate](rhs)),
protect_(detail::store::insert(data_)),
capacity_(rhs.capacity_) {}

It's subtle, but the operator SEXP() operator is called here before doing the shallow duplicate. So we end up with 2 duplications:

  • One from the truncation in the operator SEXP() (which previously just set a growable bit / truelength and was free)
  • One from the Rf_shallow_duplicate()

The right thing to do here is to take full control over this copy constructor rather than trying to push responsibility up to the read only variant. I think we are going to need to call Rf_xlengthgets() in here ourselves. And I think the resulting copy should end up with capacity_ = rhs.length_ rather than capacity_ = rhs.capacity_, i.e. the copy should not assume that it's capacity needs to be any larger than the elements it needs to hold.

#include "cpp11/integers.hpp"

[[cpp11::register]] cpp11::writable::integers test_() {
  cpp11::writable::integers x(100000);

  // Doubles the capacity to 200000
  x.push_back(1);

  // Calls writable copy constructor, currently this:
  // - Truncates when converting to SEXP through an allocation
  // - `Rf_shallow_duplicate()`s that SEXP before passing on to the read only ctor
  cpp11::writable::integers y(x);

  return y;
}
> profmem::profmem(test_())
Rprofmem memory profiling of:
test_()

Memory allocations:
       what   bytes              calls
1     alloc  400048 test_() -> .Call()
2     alloc  800048 test_() -> .Call()
3     alloc  400056 test_() -> .Call()
4     alloc  400056 test_() -> .Call() # <- this one is extra
total       2000208   
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.

1 participant