stcui007
commented
3 months ago I just cloned the PR code and did a run using Bushland and Phillipsburg initial configs. Valgrind report for Bushland:
==106320== LEAK SUMMARY:
==106320== definitely lost: 0 bytes in 0 blocks
==106320== indirectly lost: 0 bytes in 0 blocks
==106320== possibly lost: 0 bytes in 0 blocks
==106320== still reachable: 1,824 bytes in 27 blocks
==106320== suppressed: 0 bytes in 0 blocksFor Phillipsburg:
==107020== LEAK SUMMARY:
==107020== definitely lost: 6,701,496 bytes in 279,229 blocks
==107020== indirectly lost: 0 bytes in 0 blocks
==107020== possibly lost: 48 bytes in 2 blocks
==107020== still reachable: 2,424 bytes in 52 blocks
==107020== suppressed: 0 bytes in 0 blocks
Don't understand why the initial config causes the difference.
hellkite500
peterlafollette
Serious memory leaks exist in this implementation, as noted in #27. Running through the llvm address-sanitizer with leak detection, the total memory leaked by the bmi test using
is nearly 30 MB!
This happens predominantly because the linked list data structure supporting the layered wetting fronts dynamically allocates list elements using malloc, but these are never freed/deleted. These lists are then copied using
listCopy, between each time step to track previous state and current state, but once copied, the old memory is inaccessible and "leaks". This is extremely problematic when used in the NextGen model engine (ngen) since we potentially initialize over 800k models, and then run each for a period of time ranging from hours to decades! These memory leaks at that scale quickly exhaust system resources (as evident by the 10 day CONUS tests where #27 was first identified!).Back of the envelop math implies this could leak up to 24 TB!!!! of memory in only a year long simulation (the length of time of the simulation used in the test above)!
This PR address the bulk of the memory leaks, especially those occurring in the BMI implementation. With these changes, the memory leaks are now minimal and seem to be related mostly to the use of
fprintfandscanfand shouldn't be leaking memory for every simulation time step!Care should be taken in future developments and work with this customized linked list to ensure that dynamic memory allocations are properly freed. This PR also shows where other model states and temporaries have dynamic memory allocation and had no corresponding cleanup code. Particularly with dynamic allocation in loops (such as in Update which had nested loops in a function intended to be called in a loop) dynamic memory use must be handled correctly.
Additions
Changes
Testing
Notes
Todos
Checklist
Target Environment support
Merging this PR closes #27.