uaf-arctic-eco-modeling / dvm-dos-tem

A process based Dynamic Vegetation, Dynamic Organic Soil, Terrestrial Ecosystem Model.
MIT License
22 stars 24 forks source link

Potential Memory Leak #375

Closed ThomasThelen closed 1 year ago

ThomasThelen commented 6 years ago

In Ground.cpp there are some layer objects that are getting created on the heap and then put in a linked list. When the linked list destructs, it's not going to free the memory region up, leaving the memory as dead space (unless I'm not seeing all of the calls to removeLayer).

This can easily get fixed by using std::unique_ptr if the only reference to it is the linked list, otherwise a simple std::shared_ptr will suffice and automatically garbage collect. C++ also provides a doubly linked list data structure that supports removal of objects. When the unique_ptr is removed from the list, the reference count drops to 0, and it is automatically deleted.

Examples, https://github.com/ua-snap/dvm-dos-tem/blob/ccc811597e586f74a1c8eaa49ccfbc9de75eb7e7/src/Ground.cpp#L351

https://github.com/ua-snap/dvm-dos-tem/blob/ccc811597e586f74a1c8eaa49ccfbc9de75eb7e7/src/Ground.cpp#L362

edit: I haven't tested this, but it should be possible with Visual Studio's memory profiler.

rarutter commented 6 years ago

Hi @ThomasThelen

I would not be surprised to find memory leaks. I haven't profiled for memory yet, but will definitely add it to the to-do list for the next round of fixing.

Thanks for organizing an issue for this!

Out of curiosity, how did you encounter dvm-dos-tem?

ThomasThelen commented 6 years ago

We're separated by maybe 3 degrees of freedom. Someone at work had posted a fun repository by Person 1. I looked at their profile and sorted by language (I've been looking for ecology related C++ projects for a while now; the organization I'm a part of is heavy on R). I ended up on another person's page and found maybe this repo or the organization.

I'd have fun fixing this but I don't have a VM with redhat on it; I may get it set up this weekend.

tobeycarman commented 6 years ago

@ThomasThelen thats great, we can use any interest/help we get!

Last I tried the code will run nicely on Ubuntu as well. You may have found the boostrap scripts in the project root as well as the Vagrantfile. If you use Vagrant, in theory you could clone the repo, and then run vagrant up in the project root and you'd end up with a VM built to specification with dvmdostem and all dependencies installed on the VM. Alternatively you can use the bootstrap scripts as a reference and build the VM by hand.

Give a shout if you run into any issues.

ThomasThelen commented 6 years ago

After doing the following,

  1. Cloning
  2. Running vagrant up

I ran into the error.

data@data:~/Documents/dev/dvm-dos-tem$ vagrant up
Building your guest VM with 4 processors and 756MB of RAM...
Bringing machine 'default' up with 'virtualbox' provider...
==> default: Box 'TFDuesing/Fedora-20' could not be found. Attempting to find and install...
    default: Box Provider: virtualbox
    default: Box Version: >= 0
==> default: Loading metadata for box 'TFDuesing/Fedora-20'
    default: URL: https://vagrantcloud.com/TFDuesing/Fedora-20
==> default: Adding box 'TFDuesing/Fedora-20' (v1.1.8) for provider: virtualbox
    default: Downloading: https://vagrantcloud.com/TFDuesing/boxes/Fedora-20/versions/1.1.8/providers/virtualbox.box
==> default: Successfully added box 'TFDuesing/Fedora-20' (v1.1.8) for 'virtualbox'!
There are errors in the configuration of this machine. Please fix
the following errors and try again:

vm:
* Forwarded port '2222' (host port) is declared multiple times
with the protocol 'tcp'.

I ended up adding ssh as the id and it seemed to work. config.vm.network "forwarded_port", guest:22, host:2222 to config.vm.network "forwarded_port", id: 'ssh', guest:22, host:2222

After the dependencies installed, I ran into

==> default: Running provisioner: shell...
    default: Running: /tmp/vagrant-shell20180616-31719-274ub9.sh
==> default: Running bootstrap-sel-custom.sh script...
==> default: Should not require sudo priviledges to run!
==> default: Appending github's key to ~/.ssh/known_hosts...
==> default: # github.com SSH-2.0-libssh_0.7.0
==> default: # github.com SSH-2.0-libssh_0.7.0
==> default: no hostkey alg
==> default: Cloning into '/home/vagrant/dvm-dos-tem'...
==> default: Warning: Permanently added the RSA host key for IP address '192.30.255.112' to the list of known hosts.
==> default: Permission denied (publickey).
==> default: fatal: Could not read from remote repository.
==> default: 
==> default: Please make sure you have the correct access rights
==> default: and the repository exists.
==> default: /tmp/vagrant-shell: line 36: cd: dvm-dos-tem: No such file or directory
==> default: fatal: Not a git repository (or any of the parent directories): .git
==> default: fatal: Not a git repository (or any of the parent directories): .git
==> default: fatal: Not a git repository (or any of the parent directories): .git
==> default: Setting up bashrc preferences file....
==> default: DONE setting up a dvm-dos-tem environment. You should be ready to go!

It looked like the repo wasn't being found, so in bootstrap-sel-cusom.sh I changed git clone git@github.com:ua-snap/dvm-dos-tem.git "$HOME"/dvm-dos-tem to git clone https://github.com/ua-snap/dos-tem.git "$HOME"/dvm-dos-tem

This got the VM up and running with the repo!

I did see this error, but it hasn't seemed to have had any effect

==> default: Running bootstrap-sel-custom.sh script...
==> default: Should not require sudo priviledges to run!
==> default: Appending github's key to ~/.ssh/known_hosts...
==> default: # github.com SSH-2.0-libssh_0.7.0
==> default: # github.com SSH-2.0-libssh_0.7.0
==> default: no hostkey alg
==> default: Cloning into '/home/vagrant/dvm-dos-tem'...
==> default: Already on 'master'

I'll probably want to use CLion to play with the project (I've actually never used vagrant); I'll do some googling on how to do this but I'm assuming it's similar to notepad++

I'm not sure if the two changes that I made were just edge cases on my machine so I won't send a PR in with them unless requested.

After running make I get a handful of object files and DOSTEM*. Following the guide, I should have a file ./dvmdostem. Any tips?

tobeycarman commented 6 years ago

@ThomasThelen, excellent, that is good progress. A few notes:

  1. re: the edit to the Vagrantfile to add id:'ssh', I wonder if this has to do with you having a more recent version of Vagrant?
  2. When you changed the repo clone address, you accidentally got DOS-TEM, and not DVM-DOS-TEM. DOS-TEM is a simplified version of the model that does not include dynamic vegetation. It also looks like you changed the protocol from git@github... to https://github.com..., which is what fixes the permissions error I think. So the address you actually need is this:

    git clone https://github.com/ua-snap/dvm-dos-tem.git "$HOME"/dvm-dos-tem

This will fix the later error where you end up with the wrong executable (and the wrong repo).

I have not used CLion, but it looks like a nice IDE. I should note that Vagrant is not an IDE, nor does it inherently provide one. It simply helps setup a VM with the dvmdostem repo cloned inside the VM and all the dependencies installed. If you are using Vagrant, it is expected that your development work will take place inside this Virtual Machine. There are many ways this can be done - by installing a graphical desktop on the VM, by sshing into the VM and using Vim, or by setting up something like Notepad++ that can access files inside the VM over ssh/sftp. When working with a Vagrant VM, you will still need to compile and run the code inside the VM, not on your host operating system. You don't actually even need to clone the repo to your host machine, it just happens to be a convenient way to get the Vagrantfile.

rarutter commented 6 years ago

It's worth noting that the Vagrant VM will have Boost 1.54, which isn't sufficient for dvm-dos-tem. I usually manually install 1.55 (since I know it works with the parallel I/O), but any newer version should work.

Edit: to clarify, parallel I/O isn't necessary, and takes some extra work to set up.

ThomasThelen commented 6 years ago

Instead of playing with building and deploying Boost 1.55 I just bumped Fedora up a few version, fedora/27-cloud-base.

I unsurprisingly hit computational limits (memory) on my machine so I'll try this with AWS!

Thanks for all the help!

rarutter commented 6 years ago

Yeah, the VM definitely needs updating. I had a branch out to do that (https://github.com/ua-snap/dvm-dos-tem/commit/93cbb9db33181c75b7ca970804194fc807ccbd78 for reference), but I think I had to focus on another issue before I finished testing it for merging.

I'd be interested in hearing how it goes with AWS if you do that! If I can eventually wrangle the output to be actually parallel, I want to try using it for runs closer to the actual domain size.