Memory leaks in gbt.load()

[sondreus]

I think I have detected a memory leak in gbt.load. Could be I'm failing to understand how R handles memory (and the imperfections of that process), but maybe adding a manual garbage collection in the C subroutine could be the way to solve this?

Minimal replication:

x <- runif(50000, 0, 10)
y <- rnorm(50000, x, 1)

mod <- gbt.train(y, as.matrix(x))
gbt.save(mod, 'gbt_model.gbt')

Memory usage (by the R process, not the objects in R workspace) increments on my system by roughly 1mb every time the below line is run:

mod <- gbt.load('gbt_model.gbt')

[eddelbuettel]

Howdy. @sondreus had reached out to me as we have had a few earlier DMs about his most excellent model and its use of agtboost.

The package is in good standing on CRAN, and Prof Ripley keeps us all honest by running checks with the (very powerful) valgrind tool on his instances. I have been at the receiving end of such "valgrind detected a leak" notes myself.

And agtboost comes out clean as a whistle.

I can replicate that with a quick R -d valgrind -f issue55.R where the R file re-groups the demo above. (Even with the load() part in a loop for extra effort.) No leak ... in the sense of memory being allocated and allocated and allocated and never freed.

The fact that @sondreus sees increase memory usage may have mean that R assigns new objects it keeps track of. It is tricky to disambiguate that. I'll think some but at glance the package seems to not be doing anything. Happy to help some more and chat but no smoking gun as far as I can tell.

PS I always find it instructive to have a baseline. So putting, say, these two lines in a file and running them as above with valgrind does generate the output below (among much more output)

Rcpp::cppFunction("double leakMe() { double *p = new double[1000]; return p[0]; }")
leakMe()

yields

==4133893== LEAK SUMMARY:                                                                                
==4133893==    definitely lost: 8,000 bytes in 1 blocks                
==4133893==    indirectly lost: 0 bytes in 0 blocks                                                      
==4133893==      possibly lost: 0 bytes in 0 blocks                                                      
==4133893==    still reachable: 56,622,975 bytes in 11,188 blocks                                        
==4133893==                       of which reachable via heuristic:                                      
==4133893==                         newarray           : 4,264 bytes in 1 blocks
==4133893==         suppressed: 0 bytes in 0 blocks                                                      
==4133893== Reachable blocks (those to which a pointer was found) are not shown.
==4133893== To see them, rerun with: --leak-check=full --show-leak-kinds=all

We have none of that in agtboost. Which is good!

[sondreus]

Thanks @eddelbuettel!

Look forward to hear from @Blunde1 who might look into this too. My guess is that my language is imprecise, so that there is no memory leak in the proper sense, just that when run many times, gbt.load takes up ever more memory (I got a colleague to replicate this to be sure), even as the loaded objects are no longer usable by any process from within R.

All the best,
Sondre

[eddelbuettel]

Yes. R uses copy-on-write internally so a changed object always becomes a new object (where plain assignment copied A <- B for whatever B do not yet create a new one but keep a reference).

I had massive headaches with things like too in the past (though maybe mostly in the distant past, on Windows, with smaller memory systems) so I don't have a great set of recommendations. A simple rm(A); gc() always looks promising but in practice may not work.

In theory, "maybe" we could bring the loading of the model to the C++ side (where we have finer control) and use accessors for the data. In practice that maybe harder because ... these model structures are larger. But I guess they are regular?

We could possibly try alternate serialization from the C++ side too. I'd be game for helping a little but I too would love to hear
what @Blunde1 thinks as he of course knows his model inside out...

[eddelbuettel]

As the cliche goes, I just had some more thoughts about this while on the morning run (and in the shower, cliche of cliches).

To clarify: @sondreus you have this issue when bootstrapping / looping over N model samples and fits, correct? I may have an idea or two (which do not require @Blunde1 to change anything) so maybe you and can (and should) take this to a scrap repo to noodle around?

[sondreus]

@eddelbuettel Yes indeed. I've "solved" the issue by dividing this portion of the update process into two batched, and moving the offending portion to a separate script and running it in a new R process which closes upon completion (so that the rest of the update process won't run out of memory).

https://github.com/TheEconomist/covid-19-the-economist-global-excess-deaths-model/blob/main/scripts/0_excess_deaths_global_estimates_autoupdater_B.R

[eddelbuettel]

That is pretty much what I was about to suggest. I happen to use external scripts lot and really like batching for R -- and wrote littler to give us r (at least on Linux, on macOS one has rename to lr or alike as it thinks r == R thanks to the genuises in Cupertino). And if one does not like that / is on a different OS then Gabor et al also wrote callr which should do the same. The key is to process each i-th R job to really get the memory back.

[Blunde1]

@sondreus Thanks, I am looking into this. I observe the same as you, and also from running mod <- gbt.train(y, as.matrix(x)) multiple times the memory seems to increase. I will "noodle around" some myself to try to find the cause (the only thing I can think of right now is perhaps improper use of new keyword on C++ side, or something similar...?) and a proper fix. For the time being, it seems you and @eddelbuettel have a workaround in place (nice to be aware of littler!).

@eddelbuettel thanks for the checks and running valgrind (and also Rcpp and its extensions)!

In theory, "maybe" we could bring the loading of the model to the C++ side (where we have finer control) and use accessors for the data. In practice that maybe harder because ... these model structures are larger. But I guess they are regular?

We could possibly try alternate serialization from the C++ side too. I'd be game for helping a little but I too would love to hear what @Blunde1 thinks as he of course knows his model inside out...

I believe the (de-)serialization can be improved upon, and would love to hear your thoughts on it. The models are regular in the sense that they are deterministic and in essence consists of nodes that are easily traversable (directed graph), thus the same logic may be applied all over. Do you know of any code/packages from which inspiration could/should be drawn on? In particular, are there any "using accessors for the data"-examples you could share?

[eddelbuettel]

Yes, it's a weird issue and it would be good to get a handle on it. We may still be doing something wrong but it apparently it is not something obvious.

Serialization is a fantastic (and complicated !!) topic. Per se you are doing nothing wrong, but of course writing "in text" is typically as seen as the most expensive way (formating out the way out, parsing on the way in). It is also human-readable and portable so there is that on the plus side. If you are only serializing 'for yourself here' (i.e. agtboost writes and reads) then saveRDS / loadRDS will be faster at the expense of human readability (and portability). I personally like fst an awful lot: it is very fast and backed by a library part one could use elsewhere (directly from C++, to interface Python, ...) And then there are msgpack and feather and writing to database (SQLite is good for this too) and more. You can go and on.

But you don't have to. Simple is good too. For now, I think, we need to understand why/how the memory use grows. Maybe try another baseline where we just repeatedly assign a matrix in which we change one or two values (to trigger R's copy-on-write) ? I would think that by assigning a new one in loop 'i' it would free / recover the memory from iteration 'i-1'. Why this does not happen here is odd. (And I think I am more puzzled about R here than about agtboost but we should double check).

[Blunde1]

@eddelbuettel Thanks!
For the memory issues I wanted to see if this was something that happened purely with agtboost, or if I would observe the same behaviour of increasing memory with a more simple C++ class exposed with RcppModules. I thus stole the yada example from the vignette

// yada.cpp
#include <Rcpp.h>
class World {
public:
    World() : msg("hello") {}
    void set(std::string msg) { this->msg = msg; }
    std::string greet() { return msg; }
private:
    std::string msg;
};
RCPP_MODULE(yada){
    using namespace Rcpp;
    class_<World>("World")
        .constructor()
        .method("greet", &World::greet)
        .method("set", &World::set)
    ;
}

and then use the set() function from R where a very large string (approx 1GB) is passed to the C++ object.

# yada.R
Rcpp::sourceCpp("yada.cpp")
n <- 2e8
long_string <- strrep("hello",n) # approx 1 GB
# Running the next two lines multiple times, memory seems to be incremented
world <- new(World)
world$set(long_string)

From running the last two lines incrementally I seem to experience the same memory issue. This is seen from visual inspection in RStudio, and from inspecting the rsession object in the Activity Monitor (MacOS). I will see if this is replicated on other systems. Please also tell me if I am doing something wrong here, and it would be interesting if you also observe this behaviour.

If you also indeed observe the above behaviour, I would assume it is not a problem with agtboost but rather some weird (because I do not understand it 😄) behaviour on the language-memory-handling side. Which seems to be aligned with your comments above.

[eddelbuettel]

That's exactly what's needed and maybe also a possible fix I had not tought about.

The long story is that some time many years ago (but after I had started using R too) the Core team changed string representation and made it much better. (It was fairly awful before and one of the reasons for the old bias of using matrices over data.frames or anything indexed by rownames as those used to be expensive.) String got a better hashed representation, but that representation is still different from numbers (int, double, ...) which are all consecutive for a vector, CharacterVectors are essentially pointers to to individual 'words' (the individual string entries).

Moreover, these are not shared between C++'s std::string and R''s whereas we shared int or double vectors! So I think when we invoke new(World) twice R cannot know and does not know it already has the vector and creates a new one.

This may be different if we used Rcpp::CharacterVector. That is more of a promise and hunch from me than a firm guarantee.

But it gets us back to @sondreus issue because your serialization is ... vectors of strings, right? We might avoid it all if we went to storing what are for just numbers as numbers. That may be more work, and more fragile, so we would make sure it is worth it.

But great existence proof above. This may lead us down a road to improve the issue.

[Blunde1]

@eddelbuettel thanks, the long story and its insights is appreciated! I think there is no way I would have understood this on my own.

I changed the attribute of World (in the example above) to a Rcpp::NumericVector x and correspondingly in the World::set() function, and from the R side passed a very large numerical vector. And now, the garbage collection (through gc()) indeed seems to work perfectly.

For agtboost, a model object holds the loss function as an std::string object, but this could (and probably should) just be an enum. All other attributes are numerical. I will take a look and perhaps do some refactoring so that the model objects are purely numerical.

[eddelbuettel]

Really nice work @Blunde1! I think the 'how strings are represented' is a bit of an insider story I have happened to have picked up on.

I'd be up for cotinuing to discuss and prototype (and help, once this vacation is over,,,,) with a 'more efficient' serializartion approach. This is essentially undocumented stuff so if we can put up a better reference implementation it may help others.

[Blunde1]

• Make agtboost model object entirely numerical
• Refactor serialization to be more efficient