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

Does `free()` actually get called? #20

Closed
stephematician opened this issue Oct 13, 2019 · 12 comments
Closed

Does `free()` actually get called? #20

stephematician opened this issue Oct 13, 2019 · 12 comments

Comments

@stephematician
Copy link

@stephematician stephematician commented Oct 13, 2019

I can't figure out if the free member function or gsl_vector_free() is ever called in the destructors of the Vector or vector types - it looks to me like no? The quickest-ugliest check on this I could manage is to use a static that takes the first gsl_vector.data passed to the function - and then prints the first element of the static data out each time the function is called.

    Rcpp::cppFunction("
    void modify_vec_rcppgsl_frees(RcppGSL::Vector vec) {
        static int old_data_set = 0;
        static double * old_data;
        
        if (!old_data_set) {
            old_data = vec.data->data;
            old_data_set = 1;
        }

        Rcpp::Rcout << *old_data << \"\\n\";
        vec.free(); // or gsl_vector_free(vec)

    }", depends="RcppGSL")

    a <- 10
    modify_vec_rcppgsl_frees(a) # 10
    modify_vec_rcppgsl_frees(a) # 4.66318e-310

When free is explicitly called, the contents changes (unsure why the specific value is printed - must be a gsl thing). Now lets rely on the destructor instead;

     Rcpp::cppFunction("
    void modify_vec_rcppgsl(RcppGSL::Vector vec) {
        static int old_data_set = 0;
        static double * old_data;
        
        if (!old_data_set) {
            old_data = vec.data->data;
            old_data_set = 1;
        }

        Rcpp::Rcout << *old_data << \"\\n\";
    }", depends="RcppGSL")

    modify_vec_rcppgsl(a) # 10
    modify_vec_rcppgsl(a) # 10

Seems suspiciously like free() wasn't called?

@stephematician
Copy link
Author

@stephematician stephematician commented Oct 13, 2019

As an aside; I couldn't find documentation clarifying the copy-assignment and copy-constructor of RcppGSL::vector perform a shallow copy (but I think this is true). Changing the behaviour of the destructor (to, say, call free()) would change the behaviour of these shallow copies considerably.

It made it difficult to design a(n exposed) class without this knowledge, and I had to look quite deep under the hood of Rcpp to be sure I understood what was going on.

@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Oct 13, 2019

It's been years since I coded this but if it really leaked as you claim I am sure CRAN would have come after me via valgrind evidence.

Rather than triggering it on-the-fly functions which with the dynamic runtime that R offers are somewhat murky, why don't you just add a print here in the destructor or here ? And possibly here and here ?

@stephematician
Copy link
Author

@stephematician stephematician commented Oct 13, 2019

I added Rcout << "free()\n" inside the call. It only outputs "free()" to the console if free() is explicitly called.

valgrind found no memory leaks - however if I run for (j in seq_len(1E7)) modify_vec_rcppgsl(a), rm everything and call gc (not that I know much about R garbage collection), the R process consumes a lot more memory (checked via ps u in another terminal), and this keeps increasing if I run the same command again. I strongly suspect it leaks.

@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Oct 13, 2019

I am out and about at the Chicago marathon so I can't add code tests but I am fairly certain I recollect this working as intended. With that, two caveats: you may need a gc() event from R itself, and you could mirror all this with a print dtors for, say, Rcpp vectors

@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Oct 13, 2019

Disreregards those links from earlier. Those are obviously not the destructors but just the free() calls.

@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Oct 13, 2019

I would need more time there than I have now -- I was just working in parallel on two other packages and PRs that had come in today.

@stephematician
Copy link
Author

@stephematician stephematician commented Oct 13, 2019

Speculation on my part; valgrind might not detect leaks if gsl_vector_calloc is smarter than a plain call to malloc?

@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Oct 13, 2019

Not sure that it is. IIRC then GSL also just calls malloc et al for us -- no different memory pool or other fancy tricks. The trickster really is R as R as a dynamic language has to do some thingd differently.

I have branch littered with Rprintf calls here for the ctor and dtor entry points, and you may be onto something here. But not sure I can offere much more than saying well you could always call .free() yourself ... But there could well be holes in the scheme to autoremove.

@stephematician
Copy link
Author

@stephematician stephematician commented Oct 13, 2019

Not sure that it is. IIRC then GSL also just calls malloc et al for us -- no different memory pool or other fancy tricks.

I had a quick look inside gsl (which removes the need for my speculation) - it is correct that gsl_vector_alloc/calloc are wrappers around calls to malloc.

Odd that valgrind doesn't identify any leaks given the gradual consumption of memory (even when gc() is explicitly called - or should have be triggered) - but that could be my lack of familiarity with valgrind, too, or R.

@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Oct 13, 2019

Valgrind is a fantastic tool. And it is rightly mentioned and highlighed in Writing R Extensions. But what helped me a few times was to establish what we could call "proper baselines". Think of simple C or C++ routines that allocates and then frees memory. With a main() -- and/or as a call to R. Basically even doing nothing gives us, as I recall, some "likely lost" reports from valgrind because of the dynamic nature of R memory mapping. In short, "it's hard". Yet with some care it can helps quite a bit.

@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Oct 14, 2019

The calls to free() were protected by a boolean; I had a logic error in how the boolean was initialized. That corrected, I now actually call free() as advertised :) Please try the new branch, I am moderately hopeful it now works as it should have before. Needless to say, big big "Thank You" for finding / raising this.

@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Oct 15, 2019

It's now PR #22 which I likely merge soon after it passed all tests on reverse depends etc pp.

eddelbuettel added a commit that referenced this issue Oct 15, 2019
correct 'allocated?' logic and actually call free() (closes #20)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.