Implement umfMemoryProviderAllocation[Split/Merge] #140
Conversation
There was a problem hiding this comment.
as for concurrency, I think if we don't add a lock we are creating a footgun. I can imagine a scenario where for example, split is called concurrently twice on the same block.
So unless we add some mechanism for exclusive ownerships of ranges of memory, I think we should add a lock.
|
I confirm that the memory leak described in #147 does not occur with this change, because |
It does, ASan no longer reports any leaks in jemalloc. |
Hm, should only add a mutex for split/merge calls or should we also protect trackingFree? I think freeing a memory region that is being split/merged can just be UB, right? |
igchor
force-pushed
the
jemalloc_fixes_and_provider_funcs
branch
from
37b2184 to
bb04c81
Compare
igchor
changed the title
igchor
force-pushed
the
jemalloc_fixes_and_provider_funcs
branch
2 times, most recently
from
52fbd11 to
ab5a5eb
Compare
|
This PR is required by #153 |
I've added mutex for split/merge only for now. |
igchor
force-pushed
the
jemalloc_fixes_and_provider_funcs
branch
from
ab5a5eb to
4633b5c
Compare
That is needed to support jemalloc (avoid leaking memory).
igchor
force-pushed
the
jemalloc_fixes_and_provider_funcs
branch
from
4633b5c to
3caed7c
Compare
| #ifdef __cplusplus | ||
| extern "C" { | ||
| #endif | ||
|
|
||
| extern umf_memory_provider_ops_t UMF_TRACKING_MEMORY_PROVIDER_OPS; | ||
|
|
||
| struct umf_memory_tracker_t { | ||
| critnib *map; | ||
| os_mutex_t *splitMergeMutex; |
There was a problem hiding this comment.
Global split/merge lock might kill scalability. As I understand the lock is needed only when concurrent split/merge occurs for the same memory buffer.
There was a problem hiding this comment.
well, mmap is also fully synchronized, so I wouldn't worry that much. This shouldn't be a super common operation. I'd start with a simple mutex and then we can think about locking out ranges of memory if there's a perf problem.
| secondSize); | ||
| if (ret != UMF_RESULT_SUCCESS) { | ||
| fprintf(stderr, "tracking split: umfMemoryTrackerAdd failed\n"); | ||
| // TODO: what now? should we rollback the split? This can only happen due to ENOMEM |
There was a problem hiding this comment.
yes, preallocation makes sense - should be doable once we have the base allocator.
That is needed to support jemalloc (avoid leaking memory).
@kswiecicki could you verify if this solves all the memory leaks? On my machine, it seems to work fine after those changes.