-
Notifications
You must be signed in to change notification settings - Fork 52
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
Implement less aggressive and configurable caching #840
base: main
Are you sure you want to change the base?
Conversation
This commit relaxes our caching a little bit by implementing a custom caching algorithm which caches only small allocations. This prevents us from allocating massive amounts of memory only to never free them. Also generalizes the way we currently handle caching memory resource handling in order to make it more flexible.
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The initiative I'm very much on board with. But we don't need to re-invent the wheel here.
std::unique_ptr<vecmem::memory_resource> make_caching_memory_resource( | ||
vecmem::memory_resource& base_mr, std::size_t threshold); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What exactly does this additional function bring to the table here? 😕 We don't use this pattern anywhere else.
#include <vecmem/memory/memory_resource.hpp> | ||
|
||
namespace traccc { | ||
class caching_memory_resource final : public vecmem::memory_resource { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea, but I would not like to have this here. Under the examples in this repository.
I would propose to teach vecmem::binary_page_memory_resource
about this threshold itself. 🤔 Similar to:
Or... we may not need to do anything actually... 🤔 Instead of creating a new memory resource class from scratch, you should just use:
That's the thing that you wrote it for years ago already after all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, the class to be used should be:
This commit relaxes our caching a little bit by implementing a custom caching algorithm which caches only small allocations. This prevents us from allocating massive amounts of memory only to never free them. Also generalizes the way we currently handle caching memory resource handling in order to make it more flexible. The caching threshold is taken as a command line parameter. This should help with the memory issues when processing ITk data seen by @paradajzblond.