-
Notifications
You must be signed in to change notification settings - Fork 921
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
histogram: add pow2 allocation stats via MIMALLOC_SHOW_HISTOGRAM=1 #529
base: dev
Are you sure you want to change the base?
Conversation
src/stats.c
Outdated
|
||
void _mi_histogram_log(size_t size) | ||
{ | ||
size_t bucket = MI_SIZE_BITS - 1 - mi_clz(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.
Why not using mi_bsr
? That is,
if (mi_unlikely(size == 0)) return;
size_t bucket = mi_bsr(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 idea. I have made these two changes. I think your suggestion for the zero check is necessary unless we were to add another element to the histogram array which also would eliminate a branch. I do a null check in the caller but didn't consider not null pointers with zero size but I believe it does happen when debug is disabled after looking at this conditional statement in mi_heap_malloc_small
:
Lines 77 to 81 in 38a0322
#if (MI_PADDING) | |
if (size == 0) { | |
size = sizeof(void*); | |
} | |
#endif |
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 appreciate the feedback from @michaeljclark. Last year, I was exploring mimalloc
performance issues using the preliminary work. Thus, my proposed change was the part of the workbench.
otherwise we end up printing a title without any histogram if stats are not enabled.
Interesting pr; just based it on top of |
I do not intend this pull to be merged, but am surfacing the feature just in case anyone finds it useful. It is a bit easier to read if one is trying to assess allocation sizes with a power of two granularity. It is currently guarded behind
#ifdef MI_STAT>1
but#if MI_DEBUG>0
might be more appropriate. It currently adds an atomic_increment to counters in a shared histogram array. It could be changed to use thread-local arrays that are merged at program termination.