[uma] Replace list of providers with explicit data and metadata providers. #641
Conversation
by returning the name directly instead of through out param. This function is not allowed to fail anyway.
data and metadata providers in initialize. Also, replace get_memory_providers with get_data_memory_provider and get_metadata_memory_provider. Implementation of get_*_provider is supposed to be supplied by each memory pool: this is to allow ignoring passed memory provider (especially the metadata provider).
| size_t numProviders, void *params, | ||
| uma_memory_pool_handle_t *hPool); | ||
| uma_memory_provider_handle_t data_provider, | ||
| uma_memory_provider_handle_t metadata_provider, |
There was a problem hiding this comment.
it is a non-flexible API. As I remember in the past you had an idea of passing a list of memory providers with the lists of types. Have you considered it as an alternative.
There was a problem hiding this comment.
Yes, as I mentioned in the PR description we might want to accept a list of provides with types. However, for that we would need to define some properties for memory providers (so that the pool can choose the appropriate one) and we don't have those right now. I feel like it's better to start with a simpler API and evolve it over time if needed.
There was a problem hiding this comment.
My current feeling (and your comment confirms that) is that it is not the final API and furthermore it is specific to disjoint_pool implementation.
So why we cannot keep with the current approach when we pass just the list of memory providers and rely on the order of providers in that list?
I am worried because our goal was to upstream UMF as a part of UR but make API generic enough so that we can refer to it during discussions with other potential customers of the future standalone solution. Now it looks like we are making it USM (or disjoint_pool) specific.
There was a problem hiding this comment.
We don't strictly need the interface to pass multiple memory providers to the memory pool right now. Even for metadata provider, it was mostly a 'future-looking' feature - the idea was that we could use it in DisjointPool (or any other) pool implementation, but we don't really use it right now (we just use regular new/delete).
As Piotr said, we should probably avoid implementing things we don't need to, perhaps for now, just having a single memory provider is enough - even without the metadata one (since we don;t have any implementation that would use it.)
I can revert the whole change that introduced the list of memory providers and just keep a single provider as it was originally. I would actually prefer that over defining which provider should be on position 0 or 1 in the list, etc. as that can change over time and we might silently break some (probably PoC) implementation silently - with function signature change, there would at least be a compilation error.
There was a problem hiding this comment.
So, I think it's just a question: do we think that having separate data and metadata memory provider is useful (and generic enough) or we'd rather wait for an actual implementation that will want to leverage that distinction.
There was a problem hiding this comment.
We don't strictly need the interface to pass multiple memory providers to the memory pool right now. Even for metadata provider, it was mostly a 'future-looking' feature - the idea was that we could use it in DisjointPool (or any other) pool implementation, but we don't really use it right now (we just use regular new/delete).
I agree with that.
I can revert the whole change that introduced the list of memory providers and just keep a single provider as it was originally.
Sounds like we came up to the conclusion that the current pool creation API and the proposed one in this PR are not required by the current UR implementation. Because disjoint_pool does not use the second memory provider today. Do we want to extend disjoint_pool to use "metadata" memory provider? If so, then the pool creation API should be able to accept more than one memory provider. But I think that this PR should be independant from #640.
My current feeling is that #640 is more important and first we should address the native error handling use case. And I think error handling should not depend on how the pool is created. Otherwise, native error handling will depend on the Pool creation API and whenever we change the pool creation API the client code for native error handling is also impacted.
We can discuss it by call. In general, we agreed in the past that API and/or architectural changes should be discussed prior to actual code changes.
| @@ -49,7 +49,7 @@ struct uma_memory_provider_ops_t { | |||
| size_t *pageSize); | |||
| enum uma_result_t (*purge_lazy)(void *provider, void *ptr, size_t size); | |||
| enum uma_result_t (*purge_force)(void *provider, void *ptr, size_t size); | |||
| void (*get_name)(void *provider, const char **ppName); | |||
| const char *(*get_name)(void *provider); | |||
There was a problem hiding this comment.
nit: looks like a change unrelated to this PR.
There was a problem hiding this comment.
yes, that's why I've put it into a separate commit, it's just a small improvement
with METADATA and DATA provider specific errors.
| @@ -77,7 +76,8 @@ INSTANTIATE_TEST_SUITE_P( | |||
| providerInitializeTest, providerInitializeTest, | |||
| ::testing::Values(UMA_RESULT_ERROR_OUT_OF_HOST_MEMORY, | |||
| UMA_RESULT_ERROR_POOL_SPECIFIC, | |||
| UMA_RESULT_ERROR_MEMORY_PROVIDER_SPECIFIC, | |||
| UMA_RESULT_ERROR_METADATA_MEMORY_PROVIDER_SPECIFIC, | |||
| UMA_RESULT_ERROR_DATA_MEMORY_PROVIDER_SPECIFIC, | |||
There was a problem hiding this comment.
I think we need a test with memory provider which always returns the error. Should it be part of this PR or we are adding tests in separate PRs?
There was a problem hiding this comment.
You mean memory pool that returns this error? Those two errors are supposed to be returned by umaPoolGetLastAllocationError (from my second PR) to indicate that the error is provider-specific.
There is some overlap between those 2 PR so I guess once one of the is merged, I will rebase (and I can add those tests) to the second one.
|
Also, regarding a list of memory providers - I've been thinking and perhaps a better interface might be just to expose a separate function like |
Existing API exposes list of memory provider but does not specify type/purpose of those providers. This makes it challenging to connect a memory allocation from memory pool with a memory provider (in case, someone wants to query properties of that memory provider or needs to get a provider-specific error message, as here: #640).
There was a discussion on #315 about passing some extra information along providers to initialize() (e.g. type) but I don't think we have a use case for that right now - passing data and metadata providers seems simpler. In future, if we'll need to pass more then 1 metadata and 1 data provider (e.g. to let pool choose) we might revert the change for initialize(). However, for querying - I think we should always return only 1 data and 1 metadata provider (those that are actually used).