Add a basic pool manager for memory providers #562
Conversation
|
|
||
| public: | ||
| std::pair<uma_result_t, std::optional<pool_manager>> | ||
| create(std::vector<std::pair<D, uma_memory_provider_handle_t>> |
There was a problem hiding this comment.
I'm not sure if we should pass memory providers to this function. The descriptor should contain enough information to create a memory_provider on the fly.
Since we templatized the descriptor, I think we could require descriptor to implement a function like createProvider and just call it here.
| } | ||
|
|
||
| void *alloc(D desc, size_t size) noexcept { | ||
| auto poolHandleOpt = getPool(desc); |
There was a problem hiding this comment.
I think the pool_manager can be just a container abstraction with API like: getPool, create (and perhaps addPool,). The rest (actual allocation API) can be implemented on top of that.
You will need some extra code to get an instance of a pool manager anyway (assuming we'll have pool_manager per ur_usm_pool_handle_t) so you can just put that code in one place (above pool_manager).
| pool_manager<D> poolManager; | ||
| for (auto &[desc, memProvider] : descHandlePairs) { | ||
| auto [ret, hPool] = | ||
| uma::poolMakeUnique<proxy_pool>(&memProvider, 1, desc); |
There was a problem hiding this comment.
The type of the pool should be parametrized, right? Also, the pool can take some extra arguments (pool-specific). Perhaps we should pass a callback to the create function that actually creates the pool?
| {UR_USM_TYPE_SHARED, {8, 4, 256}}}; | ||
| } PoolConfig; | ||
|
|
||
| struct proxy_pool { |
There was a problem hiding this comment.
What is the responsibility of this struct?
There was a problem hiding this comment.
It was meant to forward the alloc/dealloc calls to the memory provider and perform pooling.
There was a problem hiding this comment.
So, is it an alternative to the disjoint pool? Is it just one more implementation of the heap manager?
There was a problem hiding this comment.
I think this implementation is no longer needed (at least for now), the PR just hasn't been updated yet.
There was a problem hiding this comment.
Got it. It was an intermediate solution while we deal with license issues.
|
Closed in favor of #630. |
No description provided.