Skip to content

Fix memory leaks in Clock #1265

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

Closed
leekillough wants to merge 2 commits into from
Closed

Fix memory leaks in Clock #1265

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ebb9354
fix memory leaks in Clock
leekillough Mar 17, 2025
97d14cc
clang-format
leekillough Mar 17, 2025
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
25 changes: 8 additions & 17 deletions src/sst/core/clock.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,31 +26,21 @@ Clock::Clock(TimeConverter* period, int priority) : Action(), currentCycle(0), p
setPriority(priority);
}

Clock::~Clock()
{
// Delete all the handlers
for ( StaticHandlerMap_t::iterator it = staticHandlerMap.begin(); it != staticHandlerMap.end(); ++it ) {
delete *it;
}
staticHandlerMap.clear();
}

bool
Clock::registerHandler(Clock::HandlerBase* handler)
{
staticHandlerMap.push_back(handler);
staticHandlerMap.push_back(std::unique_ptr<Clock::HandlerBase>(handler));
if ( !scheduled ) { schedule(); }
return 0;
}

bool
Clock::unregisterHandler(Clock::HandlerBase* handler, bool& empty)
{

StaticHandlerMap_t::iterator iter = staticHandlerMap.begin();

for ( ; iter != staticHandlerMap.end(); iter++ ) {
if ( *iter == handler ) {
if ( iter->get() == handler ) {
staticHandlerMap.erase(iter);
Copy link
Contributor Author

@leekillough leekillough Mar 17, 2025

Choose a reason for hiding this comment

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

I just noticed: This is one place where there was a memory leak: If you call unregisterHandler(), the old code did not delete the handler, but the new code does. Here we call erase() on an element which contains std::unique_ptr, causing it to delete the handler. This fixes a "definite" memory leak reported by Valgrind.

I suppose to minimize code changes we could simply add delete handler; before the erase() in the old code, and then most if not all of the other changes would be unnecessary, but the new code using std::unique_ptr is much cleaner and avoids having to manually delete everything in the destructor. One thing I always try to do, is: If there is a delete in a destructor, see if changing to smart pointers will make it get deleted automatically and avoid the need to do it in the destructor.

Copy link
Contributor

@feldergast feldergast Mar 17, 2025

Choose a reason for hiding this comment

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

You can't delete the handler because the clock object doesn't own it. I'm work on another PR that already changes how clock handlers are deleted, taking into account who owns them. Once that merges, we can revisit possible memory leaks. Closing this PR for now.

Copy link
Contributor Author

@leekillough leekillough Mar 17, 2025

Choose a reason for hiding this comment

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

Well, I'm definitely seeing memory leaks in Clock() as I pour through Valgrind reports, so you and I are both onto something about Clock ownership.

Copy link
Contributor

@feldergast feldergast Mar 17, 2025

Choose a reason for hiding this comment

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

Yea, before the checkpointing code went in, there was probably no way to avoid memory leaks because there was no way to automatically delete clock handers that were not currently registered with a Clock object. And, CoreTest_Checkpoint definitely deregisters clocks, so I suspect the deregistered clock handlers are the ones that are leaking.

With checkpointing we had to track clock handlers in BaseComponent in order to serialize everything properly. Now, BaseComponent just owns all the Clock::Handlers registered with it and "loans" them out to the Clock objects. So, BaseComponent can delete them as the simulation finishes. While working on watchpoints, which touch clock handlers, I noticed that we hadn't changed how the handlers were deleted. I already have that implemented and haven't run into any errors with the change.

break;
}
Expand All @@ -65,8 +55,8 @@ Clock::unregisterHandler(Clock::HandlerBase* handler, bool& empty)
bool
Clock::isHandlerRegistered(Clock::HandlerBase* handler)
{
for ( auto* h : staticHandlerMap ) {
if ( h == handler ) return true;
for ( auto& h : staticHandlerMap ) {
if ( h.get() == handler ) return true;
}

return false;
Expand Down Expand Up @@ -97,11 +87,12 @@ Clock::execute()

StaticHandlerMap_t::iterator sop_iter;
for ( sop_iter = staticHandlerMap.begin(); sop_iter != staticHandlerMap.end(); ) {
Clock::HandlerBase* handler = *sop_iter;

Clock::HandlerBase* handler = sop_iter->get();

if ( (*handler)(currentCycle) )
if ( (*handler)(currentCycle) ) {
sop_iter->release(); // do not delete the handler
sop_iter = staticHandlerMap.erase(sop_iter);
}
else
++sop_iter;

Expand Down
11 changes: 6 additions & 5 deletions src/sst/core/clock.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ class Clock : public Action
{
public:
/** Create a new clock with a specified period */
Clock(TimeConverter* period, int priority = CLOCKPRIORITY);
~Clock();
explicit Clock(TimeConverter* period, int priority = CLOCKPRIORITY);
~Clock() = default;

/**
Base handler for clock functions.
Expand Down Expand Up @@ -102,10 +102,11 @@ class Clock : public Action
std::string toString() const override;

private:
/* using HandlerMap_t = std::list<Clock::HandlerBase*>; */
using StaticHandlerMap_t = std::vector<Clock::HandlerBase*>;
using StaticHandlerMap_t = std::vector<std::unique_ptr<Clock::HandlerBase>>;

Clock() {}
Clock() = default; // For serialization only
Clock(const Clock&) = delete;
Clock& operator=(const Clock&) = delete;

void execute() override;

Expand Down
Loading