Skip to content

Apple friendly way to madvice on commit/decommit: MADV_FREE_REUSABLE/MADV_FREE_REUSE #1097

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 3 commits into
base: dev
Choose a base branch
from

Conversation

Jura-Z
Copy link

@Jura-Z Jura-Z commented Jun 3, 2025

MADV_FREE_REUSABLE / MADV_FREE_REUSE on decommit/commit helps significantly with memory consumption, leading to much less OOM crashes on iOS and macOS.

I'm pretty sure this will resolve #1025

For more information about Darwin calls check:

// if this fails - fallback to MADV_DONTNEED
#endif

if (err)
Copy link
Contributor

Choose a reason for hiding this comment

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

On non-Apple platforms, err is still 0 here, right? Or am I misreading?

Copy link
Author

Choose a reason for hiding this comment

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

yes, you're right! I made a mistake, sorry!

Copy link
Author

Choose a reason for hiding this comment

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

fixed, could you please look again?

@daanx
Copy link
Collaborator

daanx commented Jun 4, 2025

Thanks for the PR and putting attenting to the MADV_FREE_REUSABLE flags! However, this needs a bit more thought I think to understand the implications well (and probably it is something I should do myself). In particular, I don't think this helps for issue #1025 since we already use MADV_DONTNEED by default when we decommit/commit on macOS. This will do correct accounting (I believe) (unlike MADV_FREE on macOS which "delays" accounting).

For MADV_FREE_REUSABLE it seems we should follow with MADV_FREE_REUSE in general to mimic decommit/commit calls. We should therefore not use MADV_FREE_REUSABLE in mi_os_reset, but only in mi_os_decommit. Moreover, we should set needs_recommit in the latter case so we call MADV_FREE_REUSE if we recommit. Unfortunately, that would mean on macOS repeated (unnecessary) mprotect calls ... and to fix that we need to distinguish between commiting initially reserved memory or actually re-committed memory. We can do this perhaps but it is not clear if that is worth the trouble at this point I think. It would be good to have more insight in the potential benefit.

@Jura-Z
Copy link
Author

Jura-Z commented Jun 4, 2025

we already use MADV_DONTNEED by default when we decommit/commit on macOS. This will do correct accounting (I believe) (unlike MADV_FREE on macOS which "delays" accounting).

So far current code in dev3 is unreleasable for us on iOS, since OOM crash rate is increased x10 compared to system allocator. So I assume current way doesn't do correct accounting.

With MADV_FREE_REUSABLE everything is different. We maybe not 100% correct with this code but OOM rate is significantly better

For MADV_FREE_REUSABLE it seems we should follow with MADV_FREE_REUSE

I added MADV_FREE_REUSE in this PR - in _mi_prim_commit

We should therefore not use MADV_FREE_REUSABLE in mi_os_reset, but only in mi_os_decommit

Thanks!! Could you please explain what mi_os_reset is?

It would be good to have more insight in the potential benefit.

Will let you know soon if it is worth it - will have actual numbers on a big population

@Jura-Z Jura-Z requested a review from pitrou June 4, 2025 19:09
@pitrou
Copy link
Contributor

pitrou commented Jun 4, 2025

@Jura-Z, sorry, only @daanx is competent to review this. I was just taking a quick look and spotted a trivial issue.

@daanx
Copy link
Collaborator

daanx commented Jun 6, 2025

Thanks @Jura-Z for the reply. I am surprised that MADV_DONTNEED doesn't do the right thing and you see the difference with MADV_FREE_REUSABLE -- I will work on using that flag on macOS. So, in mimalloc, the mi_os_reset call "resets" memory as in telling the OS it is not required to keep its contents (but we always keep the reserved range). There is no corresponding "unreset" as we assume we can always start accessing the memory again. This is used for purging memory if you use the MIMALLOC_PURGE_DECOMMITS=0 option.

However, it seems that MADV_FREE_REUSABLE should be followed by MADV_FREE_REUSE before accessing the memory again? I cannot find good documentation on this. If it is not required, we can safely use it for mi_os_reset as well.

By default we use mi_os_commit/decommit calls for purging which is more aggressive and we assume we cannot access decommitted memory. If the mi_os_decommit calls sets the needs_recommit flag, it will be followed by a mi_os_commit when we access the memory again.

daanx added a commit that referenced this pull request Jun 7, 2025
daanx added a commit that referenced this pull request Jun 7, 2025
@daanx
Copy link
Collaborator

daanx commented Jun 7, 2025

I hesitantly added initial support for these flags; it is active in a non-debug build or if MIMALLOC_PURGE_DECOMMITS=0. Can you try if the latest dev3 (or dev/dev2) improves matters for you?

(Also, if it seems better, it would be great if you could compare the latest dev3 against commit aaedb58 since that one does not have it and it might be that what you see is an artifact of an earlier bug where OS allocated memory was not always freed correctly.)

@Noxybot
Copy link
Contributor

Noxybot commented Jun 7, 2025

Hi @daanx, I think we need to add a couple of MI_UNUSED to unix/prim.c:503:26 (_mi_prim_reuse).
Like

--- a/src/prim/unix/prim.c
+++ b/src/prim/unix/prim.c
@@ -503,8 +503,9 @@ int _mi_prim_commit(void* start, size_t size, bool* is_zero) {
 int _mi_prim_reuse(void* start, size_t size) {
   #if defined(__APPLE__) && defined(MADV_FREE_REUSE)
   return unix_madvise(start, size, MADV_FREE_REUSE);
-  #endif
+  #else
   MI_UNUSED(start); MI_UNUSED(size);
+  #endif
   return 0;
 }

Otherwise it does not compile on Linux.

@Jura-Z
Copy link
Author

Jura-Z commented Jun 7, 2025

We still early, but so far looks like with this PR we have completely different OOM picture.

Thanks a lot for your change, the only thing I wanted to point out - MADV_FREE_REUSABLE can fail (I reproduced that on my macbook in the mimalloc stress test) and it makes sense to fallback to DONTNEED. I do it in this PR, but you don't do that in the commit you submitted.

Google Chrome is the main source of knowledge about MADV_FREE_REUSABLE, take a look what they're doing:

https://github.com/chromium/chromium/blob/8ef8a49f7fdd0011e264114c1af0f6eb2e45d0be/base/allocator/partition_allocator/src/partition_alloc/page_allocator_internals_posix.h#L323

@daanx
Copy link
Collaborator

daanx commented Jun 7, 2025

Thanks -- very interesting. On my initial tests it seems a bit slower than MADV_DONTNEED though (but it may be worth it if it fixes the memory accounting). I am mostly hesitant due to lack of understanding/documentation what it does exactly ... :-)

daanx added a commit that referenced this pull request Jun 7, 2025
@Jura-Z
Copy link
Author

Jura-Z commented Jun 7, 2025

It would be really interesting to talk more, maybe share some other things we found in terms of mimalloc v2 vs v3. I sent you an email to the only one email I could find. If you have any way to contact - me and @Noxybot could share more insights if you're interested

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.

4 participants