-
-
Notifications
You must be signed in to change notification settings - Fork 211
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
Memory leak in Rcpp::sample #1074
Comments
Thanks for the report but ... I'm not sure. You are allocated 10^8 as R objects in the context of a memory-managed dynamic interpreted environment. These are not supposed to disappear at end of scope as in C/C++; it simply is different by design. |
Then again, something seems to stick around longer indeed. There are two calls to |
The behavior does not depend on |
That is not something I implied. Scope in C++ is curlies, but R is still reference counted. Different story. Either an R function gets created which gets called by Feel free to debug if it is an itch that needs scratching. Help is always welcome :) |
From a very casual first glance this seems to make a difference. Can you try it too, please? modified inst/include/Rcpp/sugar/functions/sample.h
@@ -348,7 +348,8 @@ inline Vector<INTSXP> EmpiricalSample(int n, int size, bool replace, bool one_ba
return ans;
}
- int* x = reinterpret_cast<int*>(R_alloc(n, sizeof(int)));
+ //int* x = reinterpret_cast<int*>(R_alloc(n, sizeof(int)));
+ IntegerVector x(n);
for (int i = 0; i < n; i++) {
x[i] = i;
}
@@ -378,7 +379,8 @@ inline Vector<RTYPE> EmpiricalSample(int size, bool replace, const Vector<RTYPE>
return ans;
}
- int* x = reinterpret_cast<int*>(R_alloc(n, sizeof(int)));
+ //int* x = reinterpret_cast<int*>(R_alloc(n, sizeof(int)));
+ IntegerVector x(n);
for (int i = 0; i < n; i++) {
x[i] = i;
} |
Already did. This resolves the issue. Thanks! Should I create a PR? |
No, I got as it as I have a branch up. Any idea why that goes belly up? Anyway, good catch! Thanks for that. |
The memory is also cleared up for me if I just manually call |
Still, makes sense to be deterministic here and just use an |
I tried the usual |
First of all, thanks for the great work, Rcpp is awesome!
I've noticed that memory usage blows up when repeatedly calling
Rcpp::sample()
.Running this minimal reproducible example allocates multiple GB of memory:
The expected memory footprint of
call_sample_n_times(10^8)
is pretty much zero. Note that the parameters toRcpp::sample
do not depend onn
.Rcpp::RcppArmadillo::sample
does not exhibit this behavior. The equivalentRcppArmadillo
code uses almost no memory, as expected:Tested on Windows 10 and Ubuntu 18.04.
Ubuntu 18.04:
Windows 10:
The text was updated successfully, but these errors were encountered: