-
-
Notifications
You must be signed in to change notification settings - Fork 473
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
Pari default stack size results in huge physical memory commit after fork on Cygwin #22633
Comments
comment:1
The additional patch I've attached seems to fix this, but I would like to do more testing to see if any more work is needed. This definitely fixes the immediate problem of memory being eaten up after New commits:
|
Branch: u/embray/cygwin/pari-mmap |
Author: Erik Bray |
Commit: |
Upstream: Not yet reported upstream; Will do shortly. |
comment:2
Per jdemeyer it might be fine to simplify this by including |
comment:3
Replying to @embray:
If you have a patch ready, I can test it. |
comment:4
Do you want me to report this upstream? |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:6
I just updated the patch with your suggestion. I'll report it now. |
Changed upstream from Not yet reported upstream; Will do shortly. to Reported upstream. No feedback yet. |
This comment has been minimized.
This comment has been minimized.
comment:9
Can you check if it suffices to add |
comment:10
Beware of #22675: we might need to synchronize these tickets. |
comment:11
I can update these after #22675. |
comment:12
Hm, for some reason, while I got Bill's initial reply to my report I didn't get your response in my e-mail. Maybe I need to explicitly subscribe to updates on the bug... Yeah, I can try making that change. I don't see what difference it would make--what harm does it have on the other allocations? But if you think it will help I can try it. It should be fine. |
comment:13
Replying to @jdemeyer:
ping |
comment:14
Right. Working on it. |
comment:15
A minimal example in C seems to indicate that this would not work. Perhaps it's a limitation of Cygwin and/or Windows, but calling |
comment:16
Thanks for testing. I will push a new patch here, hang on. |
Changed branch from u/embray/cygwin/pari-mmap to u/jdemeyer/cygwin/pari-mmap |
comment:18
Does this work for you? New commits:
|
comment:19
Hang on, I will add a check for the return value of |
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
|
comment:22
Does it work on Cygwin? |
comment:23
It looks like the explicit munmap before remapping works. I don't exactly see what advantage this has over my original patch but this is fine too. |
Changed upstream from Reported upstream. No feedback yet. to Fixed upstream, but not in a stable release. |
comment:24
Accepted by upstream. |
comment:25
Reviewer name missing... |
Changed author from Erik Bray to Erik Bray, Jeroen Demeyer |
Reviewer: Erik Bray, Jeroen Demeyer |
Changed branch from u/jdemeyer/cygwin/pari-mmap to |
It turns out #21582 causes big problems for fork on Cygwin. This is due to an implementation detail of how Cygwin handles copy-on-write private mappings when forking. A fact that might, unfortunately, be difficult to avoid. The problem with this on Cygwin is that, while Windows does not commit pages to physical memory until they are accessed, they do become committed upon any access (even reads, when they haven't already been written to).
So when a process with a private mmap is forked, it loops over all pages in the mmap'd region and copies them into the child's copy of the mmap'd region. This results in committing physical memory on both the parent and the child, even for pages that haven't been written to yet.
TL;DR, if there's a huge private|anonymous mmap in a process, and that process is forked, it will commit the full size of the mmap to memory in both the parent and child processes.
This is a big problem in Sage since we set a very large default stack size for Pari. This was not a problem prior to #21582, since Pari used the MAP_NORESERVE flag which circumvents this issue (only dirty pages need to be copied). The changes in #21582 make sense for Linux, but for Cygwin the opposite is true. Different but unfortunate implementation details on both platforms.
The best way forward is to use
MAP_NORESERVE
anyway, which helps Cygwin and won't hurt other platforms.Upstream bug: https://pari.math.u-bordeaux.fr/cgi-bin/bugreport.cgi?bug=1912
Upstream: Fixed upstream, but not in a stable release.
CC: @jdemeyer
Component: porting: Cygwin
Keywords: days85 windows cygwin pari mmap
Author: Erik Bray, Jeroen Demeyer
Branch/Commit:
343f241
Reviewer: Erik Bray, Jeroen Demeyer
Issue created by migration from https://trac.sagemath.org/ticket/22633
The text was updated successfully, but these errors were encountered: