incorrect resource lifetimes due to static data #614
Assignees
Labels
Milestone
Comments
|
A greater problem is the static data at the foundation of every Also check the rest of the infrastructure in |
|
Classes to look at:
|
This was referenced Sep 3, 2024
timspainNERSC
added a commit
that referenced
this issue
Sep 3, 2024
# Remove the static implementaion of NullIterator. Fixes #614 (partially)
This was referenced Sep 3, 2024
Merged
Merged
timspainNERSC
added a commit
that referenced
this issue
Sep 4, 2024
# Change ConfigMap static constants to an enum. Fixes #614 (partially) --- # Change Description Change static const ints to an enum.
timspainNERSC
added a commit
that referenced
this issue
Sep 4, 2024
# Remove the `FileCallbackCloser` static list. Fixes #614 (partially) --- # Change Description Replace with a singleton function providing a list. --- # Test Description The class unit test still passes.
timspainNERSC
added a commit
that referenced
this issue
Sep 5, 2024
# Localize `Configured<>::keyMap`s Fixes #614 (partially) --- # Change Description Nothing depends on the keyMap template member variable being a template member variable. Redefine each key map as `static` (compilation unit localized) `const std::map`. Since many classes are configured, this PR changes many files. Only the source `.cpp` files are affected. --- # Test Description Configuration tests still pass.
timspainNERSC
added a commit
that referenced
this issue
Sep 5, 2024
# Replace static non-const `MissingData` with a singleton double Fixes #614 (partially) --- # Change Description Replace the `static double` missing data value with a singleton, inlined access function. The missing data value is created on first access, but still destroyed at the end of execution. This is fine, since it is a scalar double. Removed the `MissingData.cpp` source file as it is no longer needed.
timspainNERSC
added a commit
that referenced
this issue
Sep 5, 2024
# Remove static data from `Configurator`. Fixes #614 (partially) --- # Change Description Made the static data into singletonoid functions. The data in question are: - The list of configuration sources - The true command line - The pointer to the `AdditionalConfiguration` object The command line values were combined into a struct to avoid passing more than one return value. The pointer to the additional configuration was set to a `nullptr` initially, with null checks used when the pointer value is used. --- # Test Description The `Configurator_test` still passes.
timspainNERSC
added a commit
that referenced
this issue
Sep 9, 2024
# Model Component static data Fixes #614 (partially) --- # Change Description Removed all references to the `registerModule` and the data members it uses. Also remove `hField()` and similar functions, which don't now do anything, and were originally planned to be used alongside the module register. Moved the `ModelArrayRefStore` to be lazily allocated. Moved the ocean mask and ocean index look-up vector to be lazily allocated. --- # Test Description The `ModelComponent_test` still passes. Adds some test to `ModelArray_test` to check sizes around assignment operations.
timspainNERSC
added a commit
that referenced
this issue
Sep 9, 2024
# Model Component static data Fixes #614 (partially) --- # Change Description Remove `hField()` and similar functions, which don't now do anything, and were originally planned to be used alongside the module register. Make `setOceanMask()` public. --- # Test Description The constant ocean boundary condition test now passes.
timspainNERSC
added a commit
that referenced
this issue
Sep 9, 2024
# ModelComponent fix 3 Fixes #614 (partially, incrementally) --- # Change Description Add masking to the thermodynamics test. --- # Test Description The thermodynamics tests now pass.
timspainNERSC
added a commit
that referenced
this issue
Sep 23, 2024
# Issue 614 Module & ConfiguredModule Fixes #614 (partially) --- # Change Description ## Changes to `Module<>` * Make `Module` independent of Nextsim. It is now a separate Module base class that can be used for any C++ project. * Introduce a `NextsimModule` class that specializes the newly generic `Module` for nextSIM-DG. * Replace `#include "Module.hpp"` with `#include "NextsimModule.hpp"`. * Add a map from interface name to implementation name in the `Module` namespace alongside the `Module` template class. This string-to-string map is not available elsewhere. The data is stored as a function-local `static` variable. * Remove the class-static instance of the reference implementation from the `Module<>` template. This static instance is now a function-local `static` variable, so at least the construction order is well defined. The lifetime is still until the executable epilogue. * Delete the module classes. Where there was originally (for example) a `DiagnosticOutputModule` class that inherited from `Module<IDiagnosticModule>`, everything now refers to the template instantiation. The `…Module` class was necessary to run the register the module at static start-up. With this no longer necessary, * the classes are no longer needed * the class headers (`…Module.hpp`) have been removed * the class header creation script `module_header.py` has been removed * the command in `CMakeLists.txt` to run the header script has been removed. * `#include`s of the `…Module.hpp` file have been replaced with an `#include` of `NextsimModule.hpp` and an `#include` of the interface header file. * Change the API of `Module<>`. It now depends on the definition of two classes before it is `#include`d: * `HelpMap` * `Config` which defines a `std::string Config::getImpl(const std::string&)` function. * There may be a better way to handle this, but more template arguments was much too complex a solution. * Add internal options to set the implementation without setting the unique instance to prevent recursive initialization. The external API remains the same. * Move the class-static data in `Module<>` to be function-local statics. This requires a slight change to the API and a different format in the generated `module.cpp` files. * The `fn` type is now a plain function pointer, rather than a `std::function` object. This required replacing a lambda with an ad-hoc function in one test. * The variables holding the implementation names are now namespaced into `namespace Module::InterfaceClass`. * The necessary changes to `module_builder.py` to support the above changes. * Adds a finalizer method to the `Module<>` template. This will call the destructor on the static instance, allowing all the data arrays the static instance holds to be deleted. ## Changes to `ConfiguredModule` * Remove static data from `ConfiguredModule`. Module configurations are now parsed on demand. * Remove the call from `main` to parse the configuration for the modules. * With the change to lazy configuration, it is impossible to reconfigure modules automatically from the config text. Change to explicit calls to `setImplementation()`. * The `ConfiguredModule_test` now only checks that the module configuration options are parsed correctly. Everything else is handeled by the `Module<>` template class. ## Miscellaneous other changes * Removed the directory `ModuleLoaderTestModules`. These are very defunct. * A fix to `IceGrowth_test`. This is so far from the Module changes, I don't see how it could have been working before. --- # Test Description All unit tests still pass.
timspainNERSC
added a commit
that referenced
this issue
Oct 1, 2024
# ParaGridO static data Fixes #614 (partially) --- # Change Description Moved the maps between netCDF concepts and `ModelArray` equivalents to `const` instance variables that are initialized as part of the ctor. Moved the static maps from file names to netCDF files and current time indices to a singleton-like function. Also combined the maps, to ensure they remain consistent. Added the `Finalizer` class to to what `std::atexit` does, except at our convenience, rather than executable clean-up. This includes a new test for the new class. This change is the same as that contained in PR #683 --- # Test Description Added a test for the new `Finalizer` class. The `ParaGrid_test` still passes, this mainly tests the parts that pass through to `ParaGridIO`.
timspainNERSC
added a commit
that referenced
this issue
Oct 21, 2024
# Resolving static lifetime problems Fixes #614 --- # Change Description ## Deleted classes - `ScopedTimer` - `Timer` * Removed these classes as they are currently unused and need to be heavily refactored to be thread- and possibly MPI-aware. ## Edited classes ### `ConfigMap` Changed the `static const` type constants to an `enum`. ### `Configurator` Made the static data into singletonoid functions. The data in question are: * The list of configuration sources * The true command line * The pointer to the `AdditionalConfiguration` object The command line values were combined into a struct to avoid passing more than one return value. The pointer to the additional configuration was set to a `nullptr` initially, with null checks used when the pointer value is used. ### `Configured` class template Nothing depends on the keyMap template member variable being a template member variable. Redefine each key map as `static` (compilation unit localized) `const std::map`. Since many classes are configured, this affects many files. Only the source `.cpp` files are affected. ### `ConfiguredModule` & `Module` #### Changes to `Module<>` * Make `Module` independent of Nextsim. It is now a separate Module base class that can be used for any C++ project. * Introduce a `NextsimModule` class that specializes the newly generic `Module` for nextSIM-DG. * Replace `#include "Module.hpp"` with `#include "NextsimModule.hpp"`. * Add a map from interface name to implementation name in the `Module` namespace alongside the `Module` template class. This string-to-string map is not available elsewhere. The data is stored as a function-local `static` variable. * Remove the class-static instance of the reference implementation from the `Module<>` template. This static instance is now a function-local `static` variable, so at least the construction order is well defined. The lifetime is still until the executable epilogue. * Delete the module classes. Where there was originally (for example) a `DiagnosticOutputModule` class that inherited from `Module<IDiagnosticModule>`, everything now refers to the template instantiation. The `…Module` class was necessary to run the register the module at static start-up. With this no longer necessary, * the classes are no longer needed * the class headers (`…Module.hpp`) have been removed * the class header creation script `module_header.py` has been removed * the command in `CMakeLists.txt` to run the header script has been removed. * `#include`s of the `…Module.hpp` file have been replaced with an `#include` of `NextsimModule.hpp` and an `#include` of the interface header file. * Change the API of `Module<>`. It now depends on the definition of two classes before it is `#include`d: * `HelpMap` * `Config` which defines a `std::string Config::getImpl(const std::string&)` function. * There may be a better way to handle this, but more template arguments was much too complex a solution. * Add internal options to set the implementation without setting the unique instance to prevent recursive initialization. The external API remains the same. * Move the class-static data in `Module<>` to be function-local statics. This requires a slight change to the API and a different format in the generated `module.cpp` files. * The `fn` type is now a plain function pointer, rather than a `std::function` object. This required replacing a lambda with an ad-hoc function in one test. * The variables holding the implementation names are now namespaced into `namespace Module::InterfaceClass`. * The necessary changes to `module_builder.py` to support the above changes. * Adds a finalizer method to the `Module<>` template. This will call the destructor on the static instance, allowing all the data arrays the static instance holds to be deleted. #### Changes to `ConfiguredModule` * Remove static data from `ConfiguredModule`. Module configurations are now parsed on demand. * Remove the call from `main` to parse the configuration for the modules. * With the change to lazy configuration, it is impossible to reconfigure modules automatically from the config text. Change to explicit calls to `setImplementation()`. * The `ConfiguredModule_test` now only checks that the module configuration options are parsed correctly. Everything else is handeled by the `Module<>` template class. #### Miscellaneous other changes * Removed the directory `ModuleLoaderTestModules`. These are very defunct. * A fix to `IceGrowth_test`. This is so far from the Module changes, I don't see how it could have been working before. ### `FileCallbackCloser` Replaced the static `std::list` with a singleton function providing a `std::list`. ### `Iterator` Removed the static implementation of `NullIterator`. ### `MissingData` Replaced the `static double` missing data value with a singleton, inlined access function. The missing data value is created on first access, but still destroyed at the end of execution. This is fine, since it is a scalar double. Removed the `MissingData.cpp` source file as it is no longer needed. ### `ModelComponent` * Removed all references to the `registerModule` and the data members it uses. Also remove `hField()` and similar functions, which don't now do anything, and were originally planned to be used alongside the module register. * Moved the `ModelArrayRefStore` to be lazily allocated. * Moved the ocean mask and ocean index look-up vector to be lazily allocated. ### `ModelConfig` Moved the key specification to an `.ipp` file to avoid repeating the text in `Model.cpp`. ### `ParaGridIO` * Moved the maps between netCDF concepts and `ModelArray` equivalents to `const` instance variables that are initialized as part of the ctor. * Moved the static maps from file names to netCDF files and current time indices to a singleton-like function. Also combined the maps, to ensure they remain consistent. * Added the `Finalizer` class to to what `std::atexit` does, except at our convenience, rather than executable clean-up. This includes a new test for the new class. This change is the same as that contained in PR #683 ## New classes ### `Finalizer` * Added the `Finalize` class, which mimics `std::atexit()`, but can be called before the end of `main()`. * Added the `Finalizer.cpp` source file to the CMake system. * Added a test of the `Finalizer` class. * Added a `Module::finalize<>` function template. * Added the instantiation of the above function template to the module generation file. * Added registration of finalization for a module wherever the module implementation if configured. * By calling `registerUnique()` each module instantiation is only called once when the finalization occurs. --- # Test Description Apart from deleted tests for deleted classes, all tests should still pass. ### `ModelComponent` Adds some test to `ModelArray_test` to check sizes around assignment operations. ### `Finalizer` There is a test for the new `Finalizer` class. --- # Documentation Impact No changes to existing documentation. All classes should provided the same interfaces as previously.
github-project-automation
bot
moved this from Discussion
to Done
in neXtSIM_DG overview
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Currently significant parts of the model are constructed and managed through
ConfiguredModule::configuredModulesand therefore have static lifetimes. This makes resource management via RAII challenging since the order of init/release is hard to track and destructors will only be called during the epilogue ofmain.A concrete example for a problem this causes can be found in the GPU code. Usage of the GPU requires both manual
initializeandfinalizecalls and the management of memory buffers. Currently the simulation will always terminate with an exception because the buffers are freed by a destructor which is only called after thefinalize.The easiest solution would probably be to make the
ConfiguredModuledata non-static and to introduce a finalize call.The text was updated successfully, but these errors were encountered: