[umf] implement memory tracking using critnib #796
Conversation
|
|
||
| // TODO: handle removing part of the range | ||
| (void)size; | ||
| int ret = critnib_insert((critnib *)hTracker, (uintptr_t)ptr, value, 0); |
There was a problem hiding this comment.
Why can we not use void* as a key and should use uintptr_t instead?
There was a problem hiding this comment.
uintptr_t is more generic as a key and I guess we will use critnib for other cases, other than mapping a pointers, as well
There was a problem hiding this comment.
Sorry, I forgot that it is not a C++ where we can use the power of metaprogramming. :)
source/common/unified_malloc_framework/src/critnib/pmdk-compat.h
Outdated
Show resolved
Hide resolved
| @@ -23,7 +23,8 @@ TEST_F(test, memoryProviderTrace) { | |||
|
|
|||
| size_t call_count = 0; | |||
|
|
|||
| auto ret = umfMemoryProviderAlloc(tracingProvider.get(), 0, 0, nullptr); | |||
| void *ptr; | |||
There was a problem hiding this comment.
Is this and *ptr = NULL; in provider.c needed?
There was a problem hiding this comment.
Yes, this is because the implementation handles NULL pointers differently now (so we need to special case it in the tracker for both alloc and free).
igchor
force-pushed
the
umf_critnib
branch
13 times, most recently
from
c48fd12 to
4d9484c
Compare
|
Sorry, I updated your PR incorrectly (through a merge, not a rebase). Can you rebase manually please? |
| } | ||
|
|
||
| // There is no good way to do atomic_load on windows... | ||
| #define util_atomic_load_acquire(object, dest) \ |
There was a problem hiding this comment.
C++ has atomic loads implemented directly, so we could change these to be declarations (should be fine for performance if we enable lto) and implement it using the C++ apis in the .cpp file.
There was a problem hiding this comment.
In C++, you need to declare the variable as std::atomic (similarly as with _Atomic in C, and by the way, _Atomic is not supported by MSVC) to use the atomic operations. Only in C++ 20 you have https://en.cppreference.com/w/cpp/atomic/atomic_ref which can be used on any variable.
| __atomic_add_fetch(object, 1, __ATOMIC_ACQ_REL) | ||
| #endif | ||
|
|
||
| #define Malloc malloc |
There was a problem hiding this comment.
I think it's fine for now, but, since we are moving the entire critnib code here, we can just modify it to use ur/umf "native" methods for allocating/logging/asserting.
|
|
||
| return nullptr; | ||
| if (ret == ENOMEM) { |
There was a problem hiding this comment.
There's also a possible EEXIST return value. Maybe we could return UMF_RESULT_ERROR_INVALID_ARGUMENT and log that the provided ptr argument already exists.
There was a problem hiding this comment.
Yeah, I left a comment that we should add some logging there. The problem is that right now we don't have a UMF logging framework and we shouldn't use the UR-one since those files will need to be moved to a different repo.
There was a problem hiding this comment.
You have changed memory_tracker.cpp to memory_tracker.c, shouldn't this file also have .c extension?
There was a problem hiding this comment.
Right now I need it to be a C++ file because I use C++ global variables in case we build UMF as static library to do cleanup. We probably could use some windows-specific API here as well but we plan to only support shared lib in the future so this soon won't be necessary.
This implementation is lock-free and does not require C++ runtime.
Implement locks using C++ std::mutex and atomics by using Interlocked* functions and calling _ReadWriteBarrier
Done. |
Ref: #442