Builders leak ParameterCollectionStorage memory upon construction
- Dominant language
- C++
- Stars
- 3.4k
- Forks
- 701
- PR merge metrics
- No merged PRs in 30d
Description
In part due to the use of a pointer in `ParameterCollection` for `ParameterCollectionStorage` and the lack of assignment operator or copy constructor for `ParameterCollection`, `RnnBuilders` like a `VanillaLSTMBuilder` will leak memory during their construction. In particular at a line like https://github.com/clab/dynet/blob/93a5cd2d6aabeb8c506f07e51ef3a779506da68b/dynet/lstm.cc#L325 the old pointer will be overwritten and lost.
This can be demonstrated with a short program like this one extracted from train_rnn-autobatch.cc:
```cpp
#include "dynet/lstm.h"
#include
using namespace dynet;
int main(int argc, char** argv) {
initialize(argc, argv);
{
unsigned int HIDDEN = 200;
unsigned int EMBED_SIZE = 200;
std::cout << "m: ";
ParameterCollection m;
std::cout << "fwR: ";
VanillaLSTMBuilder fwR = VanillaLSTMBuilder(1, EMBED_SIZE, HIDDEN, m);
}
cleanup();
return 0;
}
```
If some debugging output is added like these lines in `model.cc`,
```cpp
ParameterCollection::ParameterCollection() : name("/"),
storage(DYNET_NEW(ParameterCollectionStorage(default_weight_decay_lambda))),
parent(nullptr) {
std::cout
<< "Constructing1 " << this
<< " with parent " << parent
<< " with storage " << storage
<< std::endl;
}
ParameterCollection::ParameterCollection(const string & my_name, ParameterCollection* my_parent, float weight_decay_lambda) :
name(my_name), storage(DYNET_NEW(ParameterCollectionStorage(weight_decay_lambda))), parent(my_parent) {
std::cout
<< "Constructing2 " << this
<< " with parent " << parent
<< " with storage " << storage
<< std::endl;
}
ParameterCollection::~ParameterCollection() {
std::cerr
<< "Destructing " << this
<< " with parent " << parent
<< " with storage " << storage
<< std::endl;
if (parent == nullptr && storage != nullptr)
delete storage;
}
```
and a note added to `lstm.cc` in `VanillaLSTMBuilder::VanillaLSTMBuilder`
```cpp
std::cout << "local_model: ";
local_model = model.add_subcollection("vanilla-lstm-builder");
```
one can get output like this:
```
m: Constructing1 000000F8B4EFF8C0 with parent 0000000000000000 with storage 000002411AD0FD40
fwR: Constructing1 000000F8B4EFF9F8 with parent 0000000000000000 with storage 000002411AD0FF80
local_model: Constructing2 000000F8B4EFEEF0 with parent 000000F8B4EFF8C0 with storage 000002411AD101C0
Destructing 000000F8B4EFEEF0 with parent 000000F8B4EFF8C0 with storage 000002411AD101C0
Destructing 000000F8B4EFF9F8 with parent 000000F8B4EFF8C0 with storage 000002411AD101C0
Destructing 000000F8B4EFF8C0 with parent 0000000000000000 with storage 000002411AD0FD40
```
The temporary ParameterCollection displayed in the `fwR` line and stored in `local_model` will leak when `local_model` is overwritten with the value from `model.add_subcollection("vanilla-lstm-builder");`.
Probably the ParameterCollection should be made to assign and copy correctly. In this particular case, one can skip that and initialize `local_model` from the beginning by changing the constructor of `VanillaLSTMBuilder` to
```cpp
VanillaLSTMBuilder::VanillaLSTMBuilder(unsigned layers, unsigned input_dim,
unsigned hidden_dim, ParameterCollection& model, bool ln_lstm, float forget_bias) :
// The initialization of local_model has been added here.
local_model(model.add_subcollection("vanilla-lstm-builder")), layers(layers),
input_dim(input_dim), hid(hidden_dim), ln_lstm(ln_lstm), forget_bias(forget_bias),
dropout_masks_valid(false), _cg(nullptr) {
```
This results in the output
```
m: Constructing1 0000006F4795F890 with parent 0000000000000000 with storage 0000018CA3ED0150
fwR: Constructing2 0000006F4795F9C8 with parent 0000006F4795F890 with storage 0000018CA3ED0390
Destructing 0000006F4795F9C8 with parent 0000006F4795F890 with storage 0000018CA3ED0390
Destructing 0000006F4795F890 with parent 0000000000000000 with storage 0000018CA3ED0150
```
This will still result in a leak because of the condition on the destructor of the `ParameterCollection`:
```cpp
if (parent == nullptr && storage != nullptr)
delete storage;
```
This `parent` doesn't seem to have much to do with anything here. Perhaps it was meant to work around other problems. Removing the condition `parent == nullptr` will prevent the leak in this case. I was a little more cautious and changed `storage` to be a shared pointer, instead. It probably helps in cases when `ParameterCollections` are copied.
This problem probably exists with all or most builders, but I haven't studied whether similar modification will be effective for all the others. Thanks for looking into this.
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.