-
Notifications
You must be signed in to change notification settings - Fork 9
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
Conversation
Does your import modify the upstream mimalloc code at all, or is it 100% stock? |
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 Report
@@ Coverage Diff @@
## develop #737 +/- ##
========================================
Coverage 79.13% 79.13%
========================================
Files 335 335
Lines 10206 10206
========================================
Hits 8077 8077
Misses 2129 2129 |
@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. |
lib/mimalloc/include/mimalloc.h
Outdated
@@ -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(); |
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.
Matching style and such, this should probably be named mi_get_allocated_size()
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.
Good point
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?) |
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. |
780b9c9
to
5ad3c82
Compare
Fixes #736
CMake options to enable:
Environment options to add stats at the end of run
Verbose mode: