lanl / spiner

Performance portable routines for generic, tabulated, multi-dimensional data
https://lanl.github.io/spiner
BSD 3-Clause "New" or "Revised" License
7 stars 3 forks source link

Refactor of memory management #2

Closed Yurlungur closed 3 years ago

Yurlungur commented 3 years ago

A bug in downstream code singularity-eos prompted me to run valgrind on Spiner. I discovered a number of issues with Spiner's pseudo-reference-counting where it tried to manage its own memory on the host but gave up on the device. This design had several problems:

  1. It was conceptually difficult to track and bug-prone.
  2. A DataBox that managed memory could not go out of scope. If it did, all DataBoxes that depended on it would be pointing at unitialized memory.

I therefore re-design DataBox memory management in this PR. I do the following:

  1. DataBoxes can allocate memory. They free memory before re-allocating, but otherwise never free memory.
  2. Freeing memory must be performed externally via the DataBox::finalize() or free(DataBox&) methods, which release allocated memory.

This provides several advantages.

Thanks to @jdolence and @dholladay00 for useful discussions while I chased this down. Thanks @brryan for the idea about the custom deleter for the shared pointer. @jdolence @dholladay00 please take a look at this and let me know what you think. I'd like to merge it in quickly. Then I will need to update singularity-eos to utilize the new API.

Yurlungur commented 3 years ago

I also replaced a large fraction of boiler plate in the class with variatic templates. Although the templates are a little harder to read, they improve readability by reducing the number of function overloads by a factor of 6 in some cases.

dholladay00 commented 3 years ago

I'm a fan of these changes. I know that variadic templates can be somewhat impenetrable, but I think this is an excellent usage of them and the reduction in SLOC is a big plus in my book.

There was a comment about one of the functions and that one I need further clarification on before merging if that's alright.

Yurlungur commented 3 years ago

Thanks @dholladay00 ! Yes, let's discuss your comments a bit inline before proceeding.

Yurlungur commented 3 years ago

Thanks for all the feedback @dholladay00 ! I think I've addressed all concerns. Can you click approve if you're happy for now?

dholladay00 commented 3 years ago

I don't see an approve button.

dholladay00 commented 3 years ago

I don't see an approve button.

I found it

Yurlungur commented 3 years ago

Travis CI is refusing to run jobs. I'm merging.