New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use mimalloc for {Alloc,FreeAll}Temporary() #646
Conversation
src/platform/platform.cpp
Outdated
return ptr; | ||
} | ||
struct MimallocHeap { | ||
mi_heap_t *heap = NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be a little cleaner if the heap was allocated here and not in AllocTemporary
?
mi_heap_t *heap = NULL; | |
mi_heap_t *heap = mi_heap_new(); |
src/platform/platform.cpp
Outdated
struct ArenaChunk { | ||
ArenaChunk *next; | ||
~MimallocHeap() { | ||
if(heap) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is always true now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might not be, considering that mi_heap_new()
heads with
mi_heap_t* mi_heap_new(void) {
mi_heap_t* bheap = mi_heap_get_backing();
mi_heap_t* heap = mi_heap_malloc_tp(bheap, mi_heap_t); // todo: OS allocate in secure mode?
if (heap==NULL) return NULL;
Seeing as mi_heap_malloc()
(and mi_heap_malloc_small()
similarly) starts with
extern inline mi_decl_restrict void* mi_heap_malloc(mi_heap_t* heap, size_t size) mi_attr_noexcept {
if (mi_likely(size <= MI_SMALL_SIZE_MAX)) {
return mi_heap_malloc_small(heap, size);
}
else {
mi_assert(heap!=NULL);
and potentially aborts under us, I opted for a ssassert()
in the constructor instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah good catch!
Can you squash the first two commits too? There's not much point in adding mimalloc if it's not used. Then this PR is good to go. |
The heaps are wrapped in a RAIIish thread_local handler, since being affined affined to a single thread for allocations is required by the API Ref: solvespace#642
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the PR and the quick responses!
Closes #642