memory leak via use of static

[corybrunson]

First, thanks very much for creating and maintaining the library.

I'm working with some students and colleague to upgrade the R package ripserr with the current version of Ripser. While debugging an issue raised by a student, the colleague determined that a memory leak due to use of the static keyword caused the draft R package to crash, as explained here. Because the issue only arose through the attempted port to R, we thought it would be appropriate to bring to your attention.

If this is desired, i'd be very interested to understand why—while i may not understand it at a low level (having little knowledge of C++), it'll be valuable to refer back to while updating the R package now and in future.

Cheers!

[ubauer]

Thanks for your message. The static variables in Ripser should not cause memory leaks. These variables are instantiated globally only once. They are declared static to speed up instantiation in some methods which are called very frequently. The only possible issue I see is with multithreaded use of the code.

If there is a memory leak, I suppose it must result from some other problem, maybe related to the static variables, but I don't know how ripserr interfaces with the code. It is certainly OK to remove the static qualifier at the cost of speed.

[astamm]

Thanks for looking into it @ubauer.

I see for instance

static simplex_boundary_enumerator facets(0, *this);

which is declared within the implementation of method get_zero_pivot_facet() of class simplex_boundary_enumerator.

Shouldn't facets be a private attribute of the simplex_boundary_enumerator class so that it is properly destroyed when the class is out of scope? Otherwise I think it is not destroyed, especially given the nested calls.

If I am right, the same would apply for

static simplex_coboundary_enumerator cofacets(*this);

Maybe we should declare cofacets in the private field of the simplex_boundary_enumerator class.

Does that make sense?

[ubauer]

Yes, this is intentional. The variable is instantiated once and only once globally, and never destroyed. There are no nested calls of the methods in which these declarations appear. Like I said, this is quite a bit faster than having the variables as private members or as local variables because of the overhead of instantiating them, which is why I declared them as static.

I am not aware of any issues caused by this, at least in the pure C++ version, but I'd be grateful if you could explain how to replicate the issue in this version.

Also, I'd be happy to get rid of the use of static (mostly in order to make the code thread safe) if there is a way that doesn't sacrifice speed.

[astamm]

I believe using the thread_local keyword instead of the static keyword does the same job as static in a thread-safe manner (i.e. each thread gets its own instance which is not repeatedly re-created within each thread).

That being said, the fact that facets and cofacets are never destroyed does provoke a memory leak per design I would say. It might be that it does not make the C++ code crash but the leak is there per design.

Then, R has garbage collection mechanism that might mess with that leak in a way that causes it to crash. Or something else triggering the crash on the R side.

You can reproduce the issue from this branch: https://github.com/resetdisconnect/ripser/tree/ripserq.

Basically, if you put back the static or thread_local keyword, and then run this with R:

setwd("ripserq")
Rcpp::compileAttributes()
devtools::install()
.rs.restartR()

library(ripserq)
dist_vec <- c(4, 3, 5, 5, 3, 4)
result <- ripser_vec(dist_vec, dim = 1, thresh = 6.0, ratio = 1.0, p = 2)
result

# FIXME: Once output is handled one way (assignment to name versus print to
# console), attempting the other way crashes R.
ripser_vec(dist_vec, dim = 1, thresh = 6.0, ratio = 1.0, p = 2)

Then the last line should cause the crash.

Do you have a benchmark on the speed gain achieved only by the use of static/thread_local ?

[ubauer]

It is not a memory leak in the C++ version as there should be only a single instance of the variable for the entire life span of the process. In my code the process runs exactly for one persistence computation and then quits. A memory leak would mean that some memory is allocated but not referenced anymore by the code, but this is never the case for Ripser. I don't know however about the R package, but it seems likely that the crash is triggered by something else, especially if the crash occurs already on the second call.

[ubauer]

In fact, I wouldn't be able to come up with an example to trigger a memory leak by using static variables. A memory leak would require repeated allocation of objects that become unreferenced. Using static variables is in some sense the opposite, the variable is instantiated only once when the executable is initialized.

[ubauer]

I don't have a specific benchmark, but it's easy to compare the speed with vs without the static qualifier; just remove the qualifiers and compare the run time for both versions using time. That's what I did when I implemented the code, and the speedup was significant, so I decided to stick with static until I have a better solution.

[astamm]

Thanks!

In any case, we still have to figure out what's wrong on the R side, but you can use thread_local qualifier instead of static to make your code thread safe.

[astamm]

I think the reason is that, in pure C++, you have a main() function which acts as your program. The exit(0) corresponds to end of program, which triggers deletion of the static or thread-static variables.

In R, we do not have the main() function. Rather, we define (here using the {cpp11} package, but using {Rcpp} leads to the same issue):

[[cpp11::register]]
cpp11::list ripser_vec(const cpp11::doubles& dataset, int dim, double thresh, float ratio, int p) {
   std::vector<value_t> distances(dataset.begin(), dataset.end());

   // compare move with original vector
   compressed_lower_distance_matrix dist(std::move(distances));
   index_t idx_dim = static_cast<index_t>(dim);
   value_t val_thresh = static_cast<value_t>(thresh);
   coefficient_t coeff_p = static_cast<coefficient_t>(p);

   ripser<compressed_lower_distance_matrix> ripser_obj(std::move(dist), idx_dim, val_thresh, ratio, coeff_p);

   try {
     ripser_obj.compute_barcodes();
   } catch (const std::exception& e) {
     cpp11::stop("Error during computation: %s", e.what());
   } catch (...) {
     cpp11::stop("Unknown error during computation");
   }

   std::vector<std::vector<std::pair<value_t, value_t>>> result = ripser_obj.persistence_pairs;

   cpp11::writable::list output(result.size());
   // same for d, we can rename if desired but stands for dimension
   for (size_t d = 0; d < result.size(); ++d) {
     const auto& pairs = result[d];
     cpp11::writable::doubles_matrix<> mat(pairs.size(), 2);
     for (size_t i = 0; i < pairs.size(); ++i) {
       mat(i, 0) = pairs[i].first;
       mat(i, 1) = pairs[i].second;
     }
     output[d] = mat;
   }

   return output;
 }

This generates binding code to use ripser_vec() as if it was an R function.

So there is no program termination here, hence no freeing of static variables I guess.

[ubauer]

But this still doesn't explain why the code would crash, or even suggest that there is an actual memory leak. I don't see why ghat should be the case. The point of a memory leak is not that the object is not freed (for static objects it should simply be reused), but that it is not referenced and thus becomes inaccessible.

Did you confirm that there is an actual memory leak?

[astamm]

I found the issue.
This is related to the bindings via Rcpp or cpp11 and ultimately to the fact that end of program in our case happens on the R side, so R needs to be aware that there are things to be removed when the program ends.

It's been discussed here: https://lists.r-forge.r-project.org/pipermail/rcpp-devel/2010-May/000664.html.

The correct solution is to instantiate the ripser class object using:

Rcpp::XPtr<ripser<compressed_lower_distance_matrix>> ripser_obj(new ripser<compressed_lower_distance_matrix>(std::move(dist), idx_dim, val_thresh, ratio, coeff_p));
ripser_obj->compute_barcodes();
std::vector<std::vector<std::pair<value_t, value_t>>> result = ripser_obj->persistence_pairs;

That way, we use R external pointers, which properly handle memory deallocation at the right time.
I updated my PR with the changes, and while I was at it, I replaced static with thread_local to be thread-safe.

[astamm]

Thanks for this insightful discussion. I can file a PR or let you decide what you want to do with thread_local vs static.