Skip to content
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

Merged
merged 2 commits into from Jul 12, 2020

Conversation

@nabijaczleweli
Copy link
Contributor

@nabijaczleweli nabijaczleweli commented Jul 9, 2020

Closes #642

return ptr;
}
struct MimallocHeap {
mi_heap_t *heap = NULL;
Copy link
Contributor

@whitequark whitequark Jul 9, 2020

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?

Suggested change
mi_heap_t *heap = NULL;
mi_heap_t *heap = mi_heap_new();

Loading

struct ArenaChunk {
ArenaChunk *next;
~MimallocHeap() {
if(heap)
Copy link
Contributor

@whitequark whitequark Jul 9, 2020

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

Loading

Copy link
Contributor Author

@nabijaczleweli nabijaczleweli Jul 9, 2020

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.

Loading

Copy link
Contributor

@whitequark whitequark Jul 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah good catch!

Loading

@whitequark
Copy link
Contributor

@whitequark whitequark commented Jul 9, 2020

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.

Loading

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
Copy link
Contributor

@whitequark whitequark left a comment

LGTM, thanks for the PR and the quick responses!

Loading

@whitequark whitequark merged commit c4ca4be into solvespace:master Jul 12, 2020
2 of 3 checks passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants