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.

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

Bug/646 observer memory leaks #647

Merged
merged 7 commits into from
Aug 20, 2021
Merged

Conversation

restenb
Copy link
Member

@restenb restenb commented Aug 19, 2021

This PR aims to fix the memory leaks in time_series_observer.cpp and file_observer.cpp.

For file_observer, the std::stringstream ss_ that was "collecting" data to write to file has been reduced in scope to a local variable in the functions that used it.

For time_series_observer, the culprit was the zero argument constructor requesting a buffer size of 0, which bypassed the adjustIfFull test and led to constantly increasing buffer sizes for all variable types. The zero argument constructor will now instead provide a default buffer size of 10000 samples. A zero sample observer is no longer allowed and will throw, the minimum buffer size is 1.

The changes have been tested with cosim-cli and memory use found to be stable at 5-6 MB over a 6000 second simulation of the techlaunch-tutorial configuration while logging everything. Running the same simulation on the current master branch leads to memory usage steadily climbing to 10 times that.

Copy link
Member

@kyllingstad kyllingstad left a comment

Choose a reason for hiding this comment

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

Everything looks good, except that the default buffer size of 10000 needs to be mentioned in the documentation.

Copy link
Member

@kyllingstad kyllingstad left a comment

Choose a reason for hiding this comment

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

LGTM

@ljamt ljamt mentioned this pull request Aug 20, 2021
@restenb restenb merged commit 836558c into master Aug 20, 2021
@restenb restenb deleted the bug/646-observer-memory-leaks branch August 20, 2021 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants