harrisonvanderbyl / rwkv-cpp-accelerated

A torchless, c++ rwkv implementation using 8bit quantization, written in cuda/hip/vulkan for maximum compatibility and minimum dependencies
MIT License
303 stars 19 forks source link

Memory leak in rwkv.h #17

Open maksmaisak opened 1 year ago

maksmaisak commented 1 year ago

RWKV::loadFile allocates the state arrays with new but never deallocates them with delete. Unless I'm missing something, you should delete them later to prevent leaking memory. Same goes for the tensors and out variables. They're allocated but never deallocated, so their memory will never be cleaned up until the whole program exits, even if the RWKV object itself is destroyed.

Or better yet just use a std::vector<double> instead of double* to store the state tensors and other vars and use vector.getData() to get a raw pointer when you need it.

harrisonvanderbyl commented 1 year ago

You make an excellent point, totally forgot about c++ destructors being a thing

maksmaisak commented 1 year ago

It's typically most convenient to just use standard containers that handle memory memory management: if you use std::vector, then instances of the RWKV class will be cleanly destructible and copyable: if you just add some delete statements to the destructor of RWKV, then a) it'll break if you didn't create an instance of RWKV using RWKV::loadFile and b) it'll break if you copy an instance of RWKV since both objects will have pointers to the same memory.

maksmaisak commented 1 year ago

This is a step in the right direction, but like I said in the comment above, the following code will now crash:

int main() {
    RWKV rwkv;
    return 0;
}

(trying to deallocate memory via uninitialized pointers, because they were never set and thus have random values)

and this one too:

int main() {
    RWKV rwkv;
    rwkv.load_file(path);
    RWKV copyOfRwkv = rwkv;
    return 0;
}

(trying to deallocate the same memory twice)