Conversation
bratpiorka
force-pushed
the
rrudnick_coarse_provieder
branch
from
78435f3 to
90b2547
Compare
igchor
left a comment
igchor
left a comment
There was a problem hiding this comment.
I haven't looked at the provider implementation yet but regarding tests, I think it would be good to:
- add some multithreaded tests
- add asan/memcheck annotations to coarse provider ( and enable them at least when we're testing coarse-provider directly)
| umf_ba_global_free(coarse_provider); | ||
| } | ||
|
|
||
| static umf_result_t coarse_memory_provider_alloc(void *provider, size_t size, |
There was a problem hiding this comment.
this function is too long, please split it into multiple function
| return block; | ||
| } | ||
|
|
||
| static bool debug_check(coarse_memory_provider_t *provider) { |
There was a problem hiding this comment.
please split this into multiple checks/functions
| size_t max_alloc_size = 0; | ||
|
|
||
| // s1 | ||
| for (int j = 0; j < 2; j++) { |
There was a problem hiding this comment.
magic values: 2, 6, please use some consts
| umf_result_t ret = umfPoolFree(pool, ptr); | ||
| ASSERT_EQ(ret, UMF_RESULT_SUCCESS); | ||
|
|
||
| allocs.erase((*it)); |
There was a problem hiding this comment.
erase(it) would probably be faster
| return "malloc"; | ||
| } | ||
|
|
||
| struct umf_memory_provider_ops_t UMF_MALLOC_MEMORY_PROVIDER_OPS = { |
There was a problem hiding this comment.
You don't have to implement this provider - we already have one: https://github.com/oneapi-src/unified-memory-framework/blob/main/test/common/provider.hpp#L113
If you need to customize the provider_malloc you can also just create a struct that inherits from provider_base_t and overrides the functions that you need.
| std::uniform_real_distribution<float> actions_dist(0, 1); | ||
|
|
||
| std::set<alloc_ptr_size> allocs; | ||
| for (size_t i = 0; i < 100; i++) { |
There was a problem hiding this comment.
This whole loop could be simplified (and made more readable) if you'd implement the logic for choosing size/action, etc. in a separate function/class:
allocation_state state; // holds std::set<alloc_ptr_size>
for (size_t i = 0; i < 100; i++) {
auto action = generator.get_action();
if (action.is_alloc()) {
state.do_alloc(action.count, ...);
} else {
state.do_free(action.acount, ...);
}
}
There was a problem hiding this comment.
Also, this way you could reuse this for other tests (even multithreaded perhaps)
|
|
||
| const unsigned char alloc_check_val = 11; | ||
| coarse_memory_provider_params_t coarse_memory_provider_params = { | ||
| NULL, // upstream_memory_provider |
There was a problem hiding this comment.
Can't you just set each field separately as you do with disjoint_pool_params below instead of using comments?
| const size_t init_buffer_size = 200 * MB; | ||
|
|
||
| // Preallocate some memory | ||
| void *buf = malloc(init_buffer_size); |
There was a problem hiding this comment.
Please avoid raw allocations in C++ and use unique_ptr or a container (e.g. vector or string). If this test is run under valgrind and some assert triggers or an exception is thrown buf will not be freed and valgrind will report a memory leak (which makes it harder to debug).
There was a problem hiding this comment.
Ideally we would use unique_ptr for pools and providers as well but that would require more refactoring I guess.
bratpiorka
force-pushed
the
rrudnick_coarse_provieder
branch
2 times, most recently
from
a0d1606 to
8e578e6
Compare
|
This PR is huuuuugeeeee. also it seems there is a lot small random fixes which do not belong to this change. To make reviewer life easier please:
Also having more smaller self-contained commits, helps in case of debuging/bisecting, |
So that users of umf pools (which are static libraries) do not have to link with umf_utils themselves. It's not possible to link static libraries to other static libraries.
bratpiorka
force-pushed
the
rrudnick_coarse_provieder
branch
from
8e578e6 to
f9b4dff
Compare
4 participants
Description
This PR adds the Coarse Memory Provider that could be used as a cache for upstream providers (like Level Zero Memory Provider) or handle user-provided buffers e.g. allocated using mmap.
Checklist