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

[WIP] Change ExecutionContext.hpp/MemoryManager in runtime to use smart pointers #1284

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

paul0403
Copy link
Contributor

@paul0403 paul0403 commented Nov 6, 2024

Context:
During applying clang-tidy to catalyst, we picked up some raw pointers, which should be converted to smart pointers.
We start with the class MemoryManager in ExecutionContext.cpp.

Description of the Change:
Change ExecutionContext.hpp/MemoryManager in runtime to use smart pointers instead of raw pointers.

Benefits:
Better memory management. Potentially, this can resolve some memory leak issues.

Possible Drawbacks:

Related GitHub Issues:

@paul0403 paul0403 changed the title Change ExecutionContext.hpp/MemoryManager in runtime to use smart pointers [WIP] Change ExecutionContext.hpp/MemoryManager in runtime to use smart pointers Nov 6, 2024
target = sharedP;
}
}
_impl.erase(target);
Copy link
Contributor

@erick-xanadu erick-xanadu Nov 6, 2024

Choose a reason for hiding this comment

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

Doesn't the implementation of erase will call the destructor of element equivalent to target? And since target is a std::shared_ptr it will decrement its reference count? This goes against what we want erase to do, which is to erase the pointer of the set which needs to be deleted. I.e., erase will make sure that the pointer is not free'd.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, so this class is kind of like the anti-smart_ptr, where we hold a "dead" pointer, waiting for it to potentially be recycled, and manually free the pointer once the holder itself goes out of scope?

I wasn't familiar with the context here and was just trying to fix clang tidy warnings, but now that I know I think this is one instance where we have to manually silence clang tidy. I reckon this can be closed then?

Copy link
Contributor

@erick-xanadu erick-xanadu Nov 13, 2024

Choose a reason for hiding this comment

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

Ah, so this class is kind of like the anti-smart_ptr, where we hold a "dead" pointer, waiting for it to potentially be recycled,

No. It is like a unique_ptr. But with a way to not free the memory. If we knew they were dead, we would delete them. Essentially:

  1. Elements in the set may or may not be alive.
  2. When mlir_memref_free is called, the pointer is removed from this set and freed.
  3. When the set is destroyed, everything in the set is freed. (Imagine it like every element in the set is reference counted by a single counter, and here we decrement that counter).
  4. You can remove elements from the set without free-ing. This is useful to transfer the ownership of memory (and technically not what an arena allocator is like).

I call this an "arena-like" allocator, but it is not exactly an arena allocator.

and manually free the pointer once the holder itself goes out of scope?

Yes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just as a note, this design you are proposing may work if you return a shared_pointer but erase it from the set. It is then up to the caller to keep the pointer alive.

Copy link
Contributor

github-actions bot commented Nov 6, 2024

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.md on your branch with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

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