Skip to content

Exposing raw heap stats for threads. #995

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

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

jm4games
Copy link

An implementation of exposing the thread local statistics for heaps.

Not sure if project maintainers are open to this, but thought I give it a go. Also not 100% the stats I am returning are the best stats to return. Basically I want to open a pr to start a discussion around this.

This could help address issue 558

@jm4games
Copy link
Author

@microsoft-github-policy-service agree

@SasLuca
Copy link

SasLuca commented Feb 10, 2025

I support this idea, I would love to see it merged.
I am glad someone else noticed the lack of these APIs, this seems like very useful information that shouldn't be hidden.

Some notes as I tried to add this to my own codebase:

  1. The mimalloc-statistics.h headers I think needs guards for extern "C" like:
#ifdef __cplusplus
extern "C" {
#endif 

...

#ifdef __cplusplus
}
#endif
  1. I would also add a function to get global stats as well like:
const mi_stats_t* mi_global_stats(void) mi_attr_noexcept {
    mi_stats_merge();
    return &_mi_subproc()->stats;
}

@jm4games
Copy link
Author

@SasLuca I have fixes for the globals in a local branch that I need to add to this. I initially didn't realize those metrics are collected separately from the other thread local metrics. Will try to push those today or tomorrow.

@jm4games
Copy link
Author

jm4games commented Feb 11, 2025

Here are the patches (includes globals) I'm using for the 3.0.1 release. I just need them ported to the latest dev branch edition of code.

metrics.PATCH

@daanx
Copy link
Collaborator

daanx commented Mar 21, 2025

Hi @jm4games -- thank you for the work you put into these patches. However, I recently worked on providing this and there is an initial API now that is in include/mimalloc-stats.h. I think this addresses the goals of this PR. Sorry if this caused you to do duplicate work :-(... but also I hope the new API will work for you. Thanks!

@jm4games
Copy link
Author

@daanx That is no problem, as long as we expose the stats I am happy. I do see some potential things the current checked in code does not cover. The goal of my provided patch was to expose metrics at runtime so that they can be sampled/monitored during the applications lifetime. With the current implementation we will only be able to see the separate thread level stats on application exit as the stats from _mi_stats_main are only fully populated when the other threads exit (if I recall correctly). My patch exposes the ability to acquire the thread local version of the stats at any time. The one other thing i see potentially missing from your work is the ability to expose the os level statistics, for example from my patch.

typedef struct mi_os_stats_s {
   mi_stat_count_t   reserved;
   mi_stat_count_t   committed;
   mi_stat_count_t   reset;
   mi_stat_count_t   purged;
   mi_stat_counter_t mmap_calls;
   mi_stat_counter_t commit_calls;
   mi_stat_counter_t reset_calls;
   mi_stat_counter_t purge_calls;
} mi_os_stats_t;

mi_os_stats_t mi_os_stats(void) mi_attr_noexcept {
  mi_os_stats_t stats;
  memset(&stats, 0, sizeof(mi_os_stats_t));

  mi_subproc_t* subproc = _mi_subproc();

  mi_stat_atomic_copy(&stats.reserved, &subproc->stats.reserved);
  mi_stat_atomic_copy(&stats.committed, &subproc->stats.committed);
  mi_stat_atomic_copy(&stats.reset, &subproc->stats.reset);
  mi_stat_atomic_copy(&stats.purged, &subproc->stats.purged);

  mi_stat_counter_atomic_copy(&stats.mmap_calls, &subproc->stats.mmap_calls);
  mi_stat_counter_atomic_copy(&stats.commit_calls, &subproc->stats.commit_calls);
  mi_stat_counter_atomic_copy(&stats.reset_calls, &subproc->stats.reset_calls);
  mi_stat_counter_atomic_copy(&stats.purge_calls, &subproc->stats.purge_calls);
 return stats;
}

I'd be happy to rework my current work to address the above if desired.

@jm4games
Copy link
Author

Updated with latest from dev branch

@jm4games jm4games reopened this Mar 23, 2025
@jm4games jm4games force-pushed the dev branch 3 times, most recently from b82606f to 495f2c7 Compare March 23, 2025 17:14
@jm4games
Copy link
Author

@daanx I have updated my PR with my previously described changes.

@daanx
Copy link
Collaborator

daanx commented Mar 25, 2025

Hi @jm4games -- you are so fast :-) Thanks for the updates.

With the current implementation we will only be able to see the separate thread level stats on application exit as the stats from _mi_stats_main are only fully populated when the other threads exit (if I recall correctly). My patch exposes the ability to acquire the thread local version of the stats at any time.

The latest releases now call mi_stats_merge to merge the thread level statistics up to the main (subproc) statistics every N generic allocations (N = 10000 by default, mi_option_generic_collect). This way the main statistics are only slightly behind (which should be fine if monitoring a service while it is running). It is done this way to minimize performance impact of keeping detailed statistics. Also, most process wide statistics (like committed etc) are now done directly on the main statistics as well. The hope is that we thus never need to look at thread local statistics and view it purely as an optimization.

Also note we cannot directly return statistics (as mi_stats_t) since the main program may be out of date with a dynamic mimalloc.dll -- this is why we pass the size and pointer to mi_stats_h (and set the version field as well).

The mi_os_stats is convenient, nice!, but since these fields are already included in the main mi_stats_t this is not really needed? I would like to keep the mimalloc API as small is possible :-)

@jm4games
Copy link
Author

That all sounds reasonable, the mi_stats_merge is the new piece I was missing. If the stats are a little behind that is totally fine, cause I was also trying to minimize the performance impact of the stats. When I get some time I will do some sanity checking to make sure this is doing what I think its doing and should be able to close this PR. Thanks for getting all this done @dannx!

@daanx
Copy link
Collaborator

daanx commented Mar 25, 2025

Thanks! Let me know how it goes. The page.c:mi_malloc_generic is guaranteed to be called every once in a while by mimalloc, and it now calls every N times heap.c:mi_collect which now calls mi_stats_merge ensuring that the main stats get merged with the thread local ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants