From 44d49f3fa63df372a575adfaaa3419cc8df1dfa9 Mon Sep 17 00:00:00 2001 From: Tim Spain Date: Tue, 3 Sep 2024 15:02:40 +0200 Subject: [PATCH 1/4] Lazily initialize the file maps for ParaGridIO. --- core/src/ParaGridIO.cpp | 27 ++++++--------------------- core/src/include/ParaGridIO.hpp | 32 +++++++++++++++++++++++++------- 2 files changed, 31 insertions(+), 28 deletions(-) diff --git a/core/src/ParaGridIO.cpp b/core/src/ParaGridIO.cpp index 98149daeb..1ee7b3d9d 100644 --- a/core/src/ParaGridIO.cpp +++ b/core/src/ParaGridIO.cpp @@ -57,12 +57,6 @@ const std::map ParaGridIO::isDG = { }; std::map ParaGridIO::dimCompMap; -#ifdef USE_MPI -std::map ParaGridIO::openFiles; -#else -std::map ParaGridIO::openFiles; -#endif -std::map ParaGridIO::timeIndexByFile; void ParaGridIO::makeDimCompMap() { @@ -492,26 +486,17 @@ void ParaGridIO::writeDiagnosticTime( void ParaGridIO::close(const std::string& filePath) { - if (openFiles.count(filePath) > 0) { - openFiles.at(filePath).close(); - openFiles.erase(filePath); - timeIndexByFile.erase(filePath); + if (getOpenFiles().count(filePath) > 0) { + getOpenFiles().at(filePath).close(); + getOpenFiles().erase(filePath); + getFileTimeIndices().erase(filePath); } } void ParaGridIO::closeAllFiles() { - size_t closedFiles = 0; - for (const auto& [name, handle] : openFiles) { - if (!handle.isNull()) { - close(name); - closedFiles++; - } - /* If the following break is not checked for and performed, for some - * reason the iteration will continue to iterate over invalid - * string/NcFile pairs. */ - if (closedFiles >= openFiles.size()) - break; + while (getOpenFiles().size() > 0) { + close(getOpenFiles().begin()->first); } } diff --git a/core/src/include/ParaGridIO.hpp b/core/src/include/ParaGridIO.hpp index 25b102927..44d0e5965 100644 --- a/core/src/include/ParaGridIO.hpp +++ b/core/src/include/ParaGridIO.hpp @@ -26,8 +26,16 @@ namespace Nextsim { */ class ParaGridIO : public ParametricGrid::IParaGridIO { public: +#ifdef USE_MPI + typedef NetCDFFileType netCDF::NcFilePar; +#else + typedef netCDF::NcFile NetCDFFileType ; +#endif + ParaGridIO(ParametricGrid& grid) : IParaGridIO(grid) + , openFiles(getOpenFiles()) + , timeIndexByFile(getFileTimeIndices()) { if (dimCompMap.size() == 0) makeDimCompMap(); @@ -91,6 +99,9 @@ class ParaGridIO : public ParametricGrid::IParaGridIO { const std::set& forcings, const TimePoint& time, const std::string& filePath); private: + typedef std::map FileMap; + typedef std::map IndexMap; + ParaGridIO() = delete; ParaGridIO(const ParaGridIO& other) = delete; ParaGridIO& operator=(const ParaGridIO& other) = delete; @@ -107,13 +118,20 @@ class ParaGridIO : public ParametricGrid::IParaGridIO { static void closeAllFiles(); // Existing or open files are a property of the computer outside the individual - // class instance, so they are static. -#ifdef USE_MPI - static std::map openFiles; -#else - static std::map openFiles; -#endif - static std::map timeIndexByFile; + // class instance, so they are singletons. + FileMap& openFiles; + inline static FileMap& getOpenFiles() + { + static FileMap fm; + return fm; + } + + IndexMap& timeIndexByFile; + inline static IndexMap& getFileTimeIndices() + { + static IndexMap tm; + return tm; + } }; } /* namespace Nextsim */ From 09a4026ae4da8ca711b2eff4297905dc0748f13b Mon Sep 17 00:00:00 2001 From: Tim Spain Date: Tue, 3 Sep 2024 15:34:05 +0200 Subject: [PATCH 2/4] Combine the two filename based maps to ensure consistency. --- core/src/ParaGridIO.cpp | 36 +++++++++++++++++---------------- core/src/include/ParaGridIO.hpp | 21 ++++++------------- 2 files changed, 25 insertions(+), 32 deletions(-) diff --git a/core/src/ParaGridIO.cpp b/core/src/ParaGridIO.cpp index 1ee7b3d9d..1900d2118 100644 --- a/core/src/ParaGridIO.cpp +++ b/core/src/ParaGridIO.cpp @@ -337,24 +337,27 @@ void ParaGridIO::dumpModelState( void ParaGridIO::writeDiagnosticTime( const ModelState& state, const ModelMetadata& meta, const std::string& filePath) { - bool isNew = openFiles.count(filePath) <= 0; - size_t nt = (isNew) ? 0 : ++timeIndexByFile.at(filePath); + bool isNew = openFilesAndIndices.count(filePath) <= 0; + size_t nt = (isNew) ? 0 : ++openFilesAndIndices.at(filePath).second; if (isNew) { // Open a new file and emplace it in the map of open files. + // Set the initial time to be zero (assigned above) + // Piecewise construction is necessary to correctly construct the file handle/time index + // pair #ifdef USE_MPI - openFiles.try_emplace(filePath, filePath, netCDF::NcFile::replace, meta.mpiComm); + openFilesAndIndices.emplace(std::piecewise_construct, std::make_tuple(filePath), + std::forward_as_tuple(std::piecewise_construct, + std::forward_as_tuple(filePath, netCDF::NcFile::replace, meta.mpiComm), + std::forward_as_tuple(nt))); #else - openFiles.try_emplace(filePath, filePath, netCDF::NcFile::replace); + openFilesAndIndices.emplace(std::piecewise_construct, std::make_tuple(filePath), + std::forward_as_tuple(std::piecewise_construct, + std::forward_as_tuple(filePath, netCDF::NcFile::replace), + std::forward_as_tuple(nt))); #endif - // Set the initial time to be zero (assigned above) - timeIndexByFile[filePath] = nt; } // Get the file handle -#ifdef USE_MPI - netCDF::NcFilePar& ncFile = openFiles.at(filePath); -#else - netCDF::NcFile& ncFile = openFiles.at(filePath); -#endif + NetCDFFileType& ncFile = openFilesAndIndices.at(filePath).first; // Get the netCDF groups, creating them if necessary netCDF::NcGroup metaGroup = (isNew) ? ncFile.addGroup(IStructure::metadataNodeName()) @@ -486,17 +489,16 @@ void ParaGridIO::writeDiagnosticTime( void ParaGridIO::close(const std::string& filePath) { - if (getOpenFiles().count(filePath) > 0) { - getOpenFiles().at(filePath).close(); - getOpenFiles().erase(filePath); - getFileTimeIndices().erase(filePath); + if (getOpenFilesAndIndices().count(filePath) > 0) { + getOpenFilesAndIndices().at(filePath).first.close(); + getOpenFilesAndIndices().erase(filePath); } } void ParaGridIO::closeAllFiles() { - while (getOpenFiles().size() > 0) { - close(getOpenFiles().begin()->first); + while (getOpenFilesAndIndices().size() > 0) { + close(getOpenFilesAndIndices().begin()->first); } } diff --git a/core/src/include/ParaGridIO.hpp b/core/src/include/ParaGridIO.hpp index 44d0e5965..4d24a9b8d 100644 --- a/core/src/include/ParaGridIO.hpp +++ b/core/src/include/ParaGridIO.hpp @@ -34,8 +34,7 @@ class ParaGridIO : public ParametricGrid::IParaGridIO { ParaGridIO(ParametricGrid& grid) : IParaGridIO(grid) - , openFiles(getOpenFiles()) - , timeIndexByFile(getFileTimeIndices()) + , openFilesAndIndices(getOpenFilesAndIndices()) { if (dimCompMap.size() == 0) makeDimCompMap(); @@ -99,8 +98,7 @@ class ParaGridIO : public ParametricGrid::IParaGridIO { const std::set& forcings, const TimePoint& time, const std::string& filePath); private: - typedef std::map FileMap; - typedef std::map IndexMap; + typedef std::map> FileAndIndexMap; ParaGridIO() = delete; ParaGridIO(const ParaGridIO& other) = delete; @@ -119,18 +117,11 @@ class ParaGridIO : public ParametricGrid::IParaGridIO { // Existing or open files are a property of the computer outside the individual // class instance, so they are singletons. - FileMap& openFiles; - inline static FileMap& getOpenFiles() + FileAndIndexMap& openFilesAndIndices; + inline static FileAndIndexMap& getOpenFilesAndIndices() { - static FileMap fm; - return fm; - } - - IndexMap& timeIndexByFile; - inline static IndexMap& getFileTimeIndices() - { - static IndexMap tm; - return tm; + static FileAndIndexMap fim; + return fim; } }; From 721e4bf582e56aa30e32ca3ab4538c77549c8b74 Mon Sep 17 00:00:00 2001 From: Tim Spain Date: Wed, 4 Sep 2024 14:13:46 +0200 Subject: [PATCH 3/4] Use the finalizer in place of std::atexit. --- core/src/Model.cpp | 2 ++ core/src/ParaGridIO.cpp | 5 +++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/core/src/Model.cpp b/core/src/Model.cpp index 63bf2f9e6..a187639ab 100644 --- a/core/src/Model.cpp +++ b/core/src/Model.cpp @@ -10,6 +10,7 @@ #include "include/Configurator.hpp" #include "include/ConfiguredModule.hpp" #include "include/DevStep.hpp" +#include "include/Finalizer.hpp" #include "include/IDiagnosticOutput.hpp" #include "include/MissingData.hpp" #include "include/Module.hpp" @@ -71,6 +72,7 @@ Model::Model() Model::~Model() { + Finalizer::finalize(); /* * Try writing out a valid restart file. If the model and computer are in a * state where this can be completed, great! If they are not then the diff --git a/core/src/ParaGridIO.cpp b/core/src/ParaGridIO.cpp index 1900d2118..c87488169 100644 --- a/core/src/ParaGridIO.cpp +++ b/core/src/ParaGridIO.cpp @@ -9,6 +9,7 @@ #include "include/CommonRestartMetadata.hpp" #include "include/FileCallbackCloser.hpp" +#include "include/Finalizer.hpp" #include "include/MissingData.hpp" #include "include/NZLevels.hpp" #include "include/gridNames.hpp" @@ -67,8 +68,7 @@ void ParaGridIO::makeDimCompMap() }; // Also initialize the static map of tables and register the atexit // function here, since it should only ever run once - // openFiles.clear(); - std::atexit(closeAllFiles); + Finalizer::atfinalUnique(closeAllFiles); // Further one-off initialization: allow distant classes to close files via a callback. FileCallbackCloser::onClose(ParaGridIO::close); } @@ -497,6 +497,7 @@ void ParaGridIO::close(const std::string& filePath) void ParaGridIO::closeAllFiles() { + std::cout << "ParaGridIO::closeAllFiles: closing " << getOpenFilesAndIndices().size() << " files" << std::endl; while (getOpenFilesAndIndices().size() > 0) { close(getOpenFilesAndIndices().begin()->first); } From 18a8aac8a3cef6cd5ebadc4aaf425f3f5e48e990 Mon Sep 17 00:00:00 2001 From: Tim Spain Date: Wed, 4 Sep 2024 15:17:56 +0200 Subject: [PATCH 4/4] Make the final static maps const. Do things once for real. --- core/src/ParaGridIO.cpp | 87 +++++++++++++++++---------------- core/src/include/ParaGridIO.hpp | 19 ++++--- 2 files changed, 55 insertions(+), 51 deletions(-) diff --git a/core/src/ParaGridIO.cpp b/core/src/ParaGridIO.cpp index c87488169..818e8a991 100644 --- a/core/src/ParaGridIO.cpp +++ b/core/src/ParaGridIO.cpp @@ -28,49 +28,53 @@ namespace Nextsim { -// Accept both post-May 2024 (xdim, ydim, zdim) dimension names and pre-May 2024 (x, y, z) -const std::map ParaGridIO::dimensionKeys = { - { "yx", ModelArray::Type::H }, - { "ydimxdim", ModelArray::Type::H }, - { "zyx", ModelArray::Type::Z }, - { "zdimydimxdim", ModelArray::Type::Z }, - { "yxdg_comp", ModelArray::Type::DG }, - { "ydimxdimdg_comp", ModelArray::Type::DG }, - { "yxdgstress_comp", ModelArray::Type::DGSTRESS }, - { "ydimxdimdgstress_comp", ModelArray::Type::DGSTRESS }, - { "ycgxcg", ModelArray::Type::CG }, - { "yvertexxvertexncoords", ModelArray::Type::VERTEX }, -}; - -// Which dimensions are DG dimension, which could be legitimately missing -const std::map ParaGridIO::isDG = { - // clang-format off - { ModelArray::Dimension::X, false }, - { ModelArray::Dimension::Y, false }, - { ModelArray::Dimension::Z, false }, - { ModelArray::Dimension::XCG, true }, - { ModelArray::Dimension::YCG, true }, - { ModelArray::Dimension::DG, true }, - { ModelArray::Dimension::DGSTRESS, true }, - // NCOORDS is a number of components, but not in the same way as the DG components. - { ModelArray::Dimension::NCOORDS, false }, - // clang-format on -}; - -std::map ParaGridIO::dimCompMap; - -void ParaGridIO::makeDimCompMap() -{ - dimCompMap = { +ParaGridIO::ParaGridIO(ParametricGrid& grid) + : IParaGridIO(grid) + , openFilesAndIndices(getOpenFilesAndIndices()) + , dimensionKeys({ + // clang-format off + // Accept post-May 2024 (xdim, ydim, zdim) dimension names and pre-May 2024 (x, y, z) + { "yx", ModelArray::Type::H }, + { "ydimxdim", ModelArray::Type::H }, + { "zyx", ModelArray::Type::Z }, + { "zdimydimxdim", ModelArray::Type::Z }, + { "yxdg_comp", ModelArray::Type::DG }, + { "ydimxdimdg_comp", ModelArray::Type::DG }, + { "yxdgstress_comp", ModelArray::Type::DGSTRESS }, + { "ydimxdimdgstress_comp", ModelArray::Type::DGSTRESS }, + { "ycgxcg", ModelArray::Type::CG }, + { "yvertexxvertexncoords", ModelArray::Type::VERTEX }, + // clang-format on + }) + , isDG({ + // clang-format off + { ModelArray::Dimension::X, false }, + { ModelArray::Dimension::Y, false }, + { ModelArray::Dimension::Z, false }, + { ModelArray::Dimension::XCG, true }, + { ModelArray::Dimension::YCG, true }, + { ModelArray::Dimension::DG, true }, + { ModelArray::Dimension::DGSTRESS, true }, + // NCOORDS is a number of components, but not in the same way as the DG components. + { ModelArray::Dimension::NCOORDS, false }, + // clang-format on + }) + , dimCompMap({ + // clang-format off { ModelArray::componentMap.at(ModelArray::Type::DG), ModelArray::Type::DG }, { ModelArray::componentMap.at(ModelArray::Type::DGSTRESS), ModelArray::Type::DGSTRESS }, { ModelArray::componentMap.at(ModelArray::Type::VERTEX), ModelArray::Type::VERTEX }, - }; - // Also initialize the static map of tables and register the atexit - // function here, since it should only ever run once - Finalizer::atfinalUnique(closeAllFiles); - // Further one-off initialization: allow distant classes to close files via a callback. - FileCallbackCloser::onClose(ParaGridIO::close); + // clang-format on + }) +{ + if (doOnce()) { + // Register the finalization function here + Finalizer::atfinalUnique(closeAllFiles); + // Since it should only ever run once, do further one-off initialization: allow distant + // classes to close files via a callback. + FileCallbackCloser::onClose(ParaGridIO::close); + doOnce() = false; + } } ParaGridIO::~ParaGridIO() = default; @@ -497,7 +501,8 @@ void ParaGridIO::close(const std::string& filePath) void ParaGridIO::closeAllFiles() { - std::cout << "ParaGridIO::closeAllFiles: closing " << getOpenFilesAndIndices().size() << " files" << std::endl; + std::cout << "ParaGridIO::closeAllFiles: closing " << getOpenFilesAndIndices().size() + << " files" << std::endl; while (getOpenFilesAndIndices().size() > 0) { close(getOpenFilesAndIndices().begin()->first); } diff --git a/core/src/include/ParaGridIO.hpp b/core/src/include/ParaGridIO.hpp index 4d24a9b8d..c1b83d195 100644 --- a/core/src/include/ParaGridIO.hpp +++ b/core/src/include/ParaGridIO.hpp @@ -32,13 +32,7 @@ class ParaGridIO : public ParametricGrid::IParaGridIO { typedef netCDF::NcFile NetCDFFileType ; #endif - ParaGridIO(ParametricGrid& grid) - : IParaGridIO(grid) - , openFilesAndIndices(getOpenFilesAndIndices()) - { - if (dimCompMap.size() == 0) - makeDimCompMap(); - } + ParaGridIO(ParametricGrid& grid); virtual ~ParaGridIO(); /*! @@ -104,10 +98,10 @@ class ParaGridIO : public ParametricGrid::IParaGridIO { ParaGridIO(const ParaGridIO& other) = delete; ParaGridIO& operator=(const ParaGridIO& other) = delete; - static const std::map dimensionKeys; + const std::map dimensionKeys; - static const std::map isDG; - static std::map dimCompMap; + const std::map isDG; + const std::map dimCompMap; // Ensures that static variables are created in the correct order. static void makeDimCompMap(); @@ -123,6 +117,11 @@ class ParaGridIO : public ParametricGrid::IParaGridIO { static FileAndIndexMap fim; return fim; } + inline static bool& doOnce() + { + static bool firstTime = true; + return firstTime; + } }; } /* namespace Nextsim */