Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Output cleanup -- release as many file resources as possible when they are no longer needed #722

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Output cleanup -- release as many file resources as possible when they are no longer needed #722

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1b5fc82
refactor(output): manage output stream handler as unique pointer
hellkite500 Feb 7, 2024
e2546d4
feat(formulation_manager): add public function to clear formulations …
hellkite500 Feb 7, 2024
7102588
feat(hy_features): add public function to release formulation pointers
hellkite500 Feb 7, 2024
d64795d
feat(ngen): release resources before launching routing
hellkite500 Feb 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions include/core/HY_Features.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,11 @@ namespace hy_features {
std::cout<<"Catchment topology is dendritic."<<std::endl;
}

/**
* Release formulations
*/
void release_formulations();

/**
* @brief Destroy the hy features object
*
Expand Down
8 changes: 5 additions & 3 deletions include/core/catchment/HY_CatchmentArea.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,17 @@ class HY_CatchmentArea : public HY_CatchmentRealization, public GM_Object
HY_CatchmentArea();
HY_CatchmentArea(std::shared_ptr<data_access::GenericDataProvider> forcing, utils::StreamHandler output_stream);
//HY_CatchmentArea(forcing_params forcing_config, utils::StreamHandler output_stream); //TODO not sure I like this pattern
void set_output_stream(std::string file_path){output = utils::FileStreamHandler(file_path.c_str());}
void write_output(std::string out){ output<<out; }
void set_output_stream(std::string file_path){
output = std::make_unique<utils::FileStreamHandler>(file_path.c_str());
}
void write_output(std::string out){ *output<<out; }
virtual ~HY_CatchmentArea();

protected:

polygon_t bounds;

utils::StreamHandler output;
std::unique_ptr<utils::StreamHandler> output;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going to use pointers anyways, why not just use std::ostream classes directly?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StreamHandler is a bit of an abstract wrapper around ostream...the original intent was the ability to have an abstract output interface that was easily connected to existing mechanics (like ofstream for csv/file output) but also be able to implement more complicated output mechanics, such as writing to NetCDF, using the same stream operator/semantics and have a simple way of reconfiguring and switching between these using the base StreamHandler interface.

This could definitely evolve and change.

I think the reason I moved these to unique pointers was to provide some protection against pass by value copies, but I could be wrong on that motivation. I know I definitely wanted to ensure that a given formulation/realization was only ever associated with a single output resource that was tightly tied to it its lifecycle.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an aside to actually reviewing this PR, I'm pretty sure we should just make StreamHandler go away.


private:
};
Expand Down
2 changes: 1 addition & 1 deletion include/realizations/catchment/Bmi_Multi_Formulation.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -659,7 +659,7 @@ namespace realization {
template<class T>
std::shared_ptr<T> init_nested_module(int mod_index, std::string identifier, geojson::PropertyMap properties) {
std::shared_ptr<data_access::GenericDataProvider> wfp = std::make_shared<data_access::WrappedDataProvider>(this);
std::shared_ptr<T> mod = std::make_shared<T>(identifier, wfp, output);
std::shared_ptr<T> mod = std::make_shared<T>(identifier, wfp, *output);

// Since this is a nested formulation, support usage of the '{{id}}' syntax for init config file paths.
Catchment_Formulation::config_pattern_substitution(properties, BMI_REALIZATION_CFG_PARAM_REQ__INIT_CONFIG,
Expand Down
3 changes: 3 additions & 0 deletions include/realizations/catchment/Formulation_Manager.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ namespace realization {

public:

void clear(){
formulations.clear();
}
std::shared_ptr<Simulation_Time> Simulation_Time_Object;

Formulation_Manager(std::stringstream &data) {
Expand Down
8 changes: 7 additions & 1 deletion include/utilities/FileStreamHandler.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,13 @@ namespace utils
stream->open(path, std::ios::trunc);
output_stream = stream;
}
virtual ~FileStreamHandler(){}

virtual ~FileStreamHandler() override {
auto tmp = std::static_pointer_cast<std::ofstream>(output_stream);
if(tmp){
tmp->close();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is needed, unless we were checking the failbit. When output_stream's lifetime ends, its destructor will be called, so if it's an ofstream it'll close the file anyways.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not remembering exactly why I did this (that's what I get for leaving it sitting in my working tree for so long 😵‍💫). I think you are correct, and this may be an artifact of some early attempts to ensure the files were were closed but due to the shared references of formulations they were not and it took me a while to sort that out haha. I'll take a closer look and remove this if it isn't being useful.

}
}
};

}
Expand Down
2 changes: 1 addition & 1 deletion include/utilities/StreamHandler.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ namespace utils

/** Deconstructor for a StreamHandler */

~StreamHandler()
virtual ~StreamHandler()
{

}
Expand Down
11 changes: 11 additions & 0 deletions src/NGen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,6 +539,17 @@ int main(int argc, char *argv[]) {

} //done time

//Close down any file handles we have
for(auto& f : nexus_outfiles){
f.second.close();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to below, nexus_outfiles should be moved into the function and placed within an RAII block, so the destructor can handle it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nexus output handling is still pretty rough, there's been a long standing TODO to refactor it -- but its been pretty low priority.

}
//std::cout<<"Clearning manger with "<<manager.use_count()<<" references\n";
//Need to release all formulation references, those stored in hy_features object and the
//references used by the formulation manager
//this will ensure all formulation destructors are called and resources released.
features.release_formulations();
manager->clear();
Comment on lines +550 to +551
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm not understanding it, what's the reason for using shared pointers if we're going to handle manually decrementing the reference counts anyways? It would make more sense to put features and manager within an RAII block so they go out of scope before needing to call these.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jumping around here in my responses, but they are all somewhat related.

Formulations exist as shared pointers because we want to ensure we only create one formulation per feature and reuse that formulation in different contexts.

The formulation manager (which superseded the network indexing I'll discuss momentarily) exists entirely to abstract and encapsulate the parsing, configuring, and instantiation of formulations, i.e. ready to execute models associated with some feature.

It does this reasonalby well, and ultimately creates a map of feature_id -> Formulation. Each formulation must be instantiated and ready to "run" before be we can begin using them in the modeling steps, so this container does just that. The hy_features container, then, is responsible for aligning the topology and network with these formulations. It uses the constructed formulations from the manager and "ties" them to the HY_Catchment realization. The network uses feature indexing and maps to provide the indirection back to to the formulation when used in the actual computational steps...

I'm not sure if that helped in understanding or not. I can try to articulate that more completely if needed. To your point on using RAII blocks...yes, that would be one solution. The functions to explicitly release the resources by clearing the underlying maps that hold the references are another. Having these functions provides some additional flexibility beyond relying on the RAII blocks, though. For example, a Network still has a valid feature index that may be useful/needed without having a need for the model formulation associated with the features indexed by the network (the complete HY_Catchment "realization"). Similarly, the manager actually manages a little bit more than just formulations -- it manages time and other configurations that have utility without require references to the model formuations for each catchment.


#if NGEN_MPI_ACTIVE
MPI_Barrier(MPI_COMM_WORLD);
#endif
Expand Down
5 changes: 5 additions & 0 deletions src/core/HY_Features.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ HY_Features::HY_Features( geojson::GeoJSON catchments, std::string* link_key, st
HY_Features(network::Network(catchments, link_key), formulations, catchments){
}

void HY_Features::release_formulations(){
formulations->clear();
_catchments.clear();
}

HY_Features::HY_Features(network::Network network, std::shared_ptr<Formulation_Manager> formulations, geojson::GeoJSON fabric)
:network(network), formulations(formulations)
{
Expand Down
2 changes: 1 addition & 1 deletion src/core/catchment/HY_CatchmentArea.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ HY_CatchmentArea::HY_CatchmentArea()
//ctor
}

HY_CatchmentArea::HY_CatchmentArea(std::shared_ptr<data_access::GenericDataProvider> forcing, utils::StreamHandler output_stream) : HY_CatchmentRealization(forcing), output(output_stream) { }
HY_CatchmentArea::HY_CatchmentArea(std::shared_ptr<data_access::GenericDataProvider> forcing, utils::StreamHandler output_stream) : HY_CatchmentRealization(forcing), output(std::make_unique<utils::StreamHandler>(output_stream)) { }

HY_CatchmentArea::~HY_CatchmentArea()
{
Expand Down
2 changes: 1 addition & 1 deletion src/realizations/catchment/Bmi_C_Formulation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ std::shared_ptr<Bmi_C_Adapter> Bmi_C_Formulation::construct_model(const geojson:
get_allow_model_exceed_end_time(),
is_bmi_model_time_step_fixed(),
reg_func,
output);
*output);
}

std::string Bmi_C_Formulation::get_output_header_line(std::string delimiter) {
Expand Down
2 changes: 1 addition & 1 deletion src/realizations/catchment/Bmi_Cpp_Formulation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ std::shared_ptr<Bmi_Cpp_Adapter> Bmi_Cpp_Formulation::construct_model(const geoj
is_bmi_model_time_step_fixed(),
model_create_fname,
model_destroy_fname,
output);
*output);
}

std::string Bmi_Cpp_Formulation::get_output_header_line(std::string delimiter) {
Expand Down
2 changes: 1 addition & 1 deletion src/realizations/catchment/Bmi_Fortran_Formulation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ std::shared_ptr<Bmi_Fortran_Adapter> Bmi_Fortran_Formulation::construct_model(co
get_allow_model_exceed_end_time(),
is_bmi_model_time_step_fixed(),
reg_func,
output);
*output);
}

std::string Bmi_Fortran_Formulation::get_formulation_type() {
Expand Down
2 changes: 1 addition & 1 deletion src/realizations/catchment/Bmi_Py_Formulation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ shared_ptr<Bmi_Py_Adapter> Bmi_Py_Formulation::construct_model(const geojson::Pr
python_type_name,
get_allow_model_exceed_end_time(),
is_bmi_model_time_step_fixed(),
output);
*output);
}

time_t realization::Bmi_Py_Formulation::convert_model_time(const double &model_time) {
Expand Down
Loading