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

736 Add mimalloc as opt-in replacement memory allocator #737

Merged
merged 14 commits into from
Mar 26, 2020

Conversation

lifflander
Copy link
Collaborator

@lifflander lifflander commented Mar 10, 2020

Fixes #736

CMake options to enable:

-Dvt_mimalloc_enabled:BOOL=1
-Dvt_mimalloc_static:BOOL=ON
-DMI_INTERPOSE:BOOL=ON
-DMI_OVERRIDE:BOOL=ON

Environment options to add stats at the end of run

export MIMALLOC_SHOW_STATS=1

Verbose mode:

export MIMALLOC_VERBOSE=1

@PhilMiller
Copy link
Member

Does your import modify the upstream mimalloc code at all, or is it 100% stock?

@PhilMiller
Copy link
Member

Ugh, this actually implements its own full allocator. That's way more intrusive than just being able to get instrumentation. It's not clear without digging into the docs - does it have a mode that just tracks (de)allocations, and can provide statistics, without overriding use of whatever would otherwise be called?

@codecov
Copy link

codecov bot commented Mar 10, 2020

Codecov Report

Merging #737 into develop will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #737   +/-   ##
========================================
  Coverage    79.13%   79.13%           
========================================
  Files          335      335           
  Lines        10206    10206           
========================================
  Hits          8077     8077           
  Misses        2129     2129           

@lifflander
Copy link
Collaborator Author

@PhilMiller Yes, it implements it's own allocator. I couldn't find a instrumenting one that is correctly delegates to the original. I tried many variants.

I just pushed the changes to mimalloc, which adds the incremental statistic.

@@ -359,6 +359,8 @@ mi_decl_nodiscard mi_decl_export mi_decl_restrict void* mi_new_n(size_t count, s
mi_decl_nodiscard mi_decl_export void* mi_new_realloc(void* p, size_t newsize) mi_attr_alloc_size(2);
mi_decl_nodiscard mi_decl_export void* mi_new_reallocn(void* p, size_t newcount, size_t size) mi_attr_alloc_size2(2, 3);

mi_decl_export size_t getAllocatedSize();
Copy link
Member

Choose a reason for hiding this comment

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

Matching style and such, this should probably be named mi_get_allocated_size()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point

@lifflander lifflander marked this pull request as ready for review March 17, 2020 22:54
@pnstickne
Copy link
Contributor

pnstickne commented Mar 25, 2020

Would recommend adding a 'fetch' script which also prunes all non-necessary files, such as docs, tests, examples in the TPL to reduce the additions to that which is required for inclusion in VT's repo.

Here is the script for google test, eg. https://github.com/DARMA-tasking/vt/blob/develop/tests/extern/fetch-googletest.sh (so might be lib/fetch-mimalloc.sh if pairing, although maybe the fetch scripts should be elsewhere?)

@pnstickne
Copy link
Contributor

pnstickne commented Mar 25, 2020

It might also be useful to collect the changes to mimalloc in this work into a patch (or set of revisions) also included somewhere in VT for when/if mimalloc is ever updated. This could be part of the 'fetch' script to apply.

The resulting patch (applied after the changes) would be a no-diff, or result in the same being applied to the input. Can be added as last commit. Primary purpose is for future work assistance.

@lifflander lifflander force-pushed the 736-memory-allocator branch from 780b9c9 to 5ad3c82 Compare March 26, 2020 00:11
@lifflander lifflander changed the title 736 Add mimalloc as replacement memory allocator 736 Add mimalloc as opt-in replacement memory allocator Mar 26, 2020
@lifflander lifflander merged commit 170ff38 into develop Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add debugging memory allocator
3 participants