Skip to content
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

Reduce memory usage on windows when running external programs #3335

Merged
merged 1 commit into from
Mar 12, 2019

Conversation

embray
Copy link
Contributor

@embray embray commented Mar 11, 2019

This is a version of the patch from https://trac.sagemath.org/ticket/27214

TL;DR This is an issue that has come up in the past with Pari as well. When you mmap a large region of memory in Cygwin, and then fork() the process, the entirety of that region has to have physical memory committed to it.

This is a known issue on Cygwin and is sort-of by design (or rather a known limitation due to the underlying OS). The workaround, generally, is to use the non-POSIX MAP_NORESERVE flag which comes from Linux, but has slightly different, albeit similar implications.

For now we just use the MAP_NORESERVE flag on Cygwin, though it could be used on Linux as well. The implication is that it is possible to reserve a larger region of memory even if there are not enough physical pages at the time of the mmap() call to fill the entire region (though if there is enough physical memory later then it's no problem). The main downside to this is that if you reach a point where you do run out of physical memory to handle writes to the mmap'd region, then the application will crash (segfault most likely). Though I haven't tried to reproduce such a situation in GAP.

on Cygwin

Using MAP_NORESERVE on Cygwin prevents the need to commit physical pages
for the entirety of mmap'd regions until they are actually used.
@ChrisJefferson
Copy link
Contributor

Interesting. On Linux we assume people haven't disabled over-commit, as the 64-bit GAP can reserve very large chunks of memory space upfront, which we assume will be allocated lazily when we use it.

Therefore I don't think this will make anything worse, and might well make cygwin better (particularly as we move to larger 64-bit memory spaces)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 85.075% when pulling d0f592a on embray:kernel/cygwin-alloc-pool into 1213dab on gap-system:master.

@embray
Copy link
Contributor Author

embray commented Mar 11, 2019

On Cygwin you're ultimately dealing with the Windows memory manager, so some things are different, and what MAP_NORESERVE does has much stronger implications there. Without it, it's like Linux with overcommit turned off: You can't return an mmap larger than the system's combined RAM and swap space. Memory isn't used immediately but it's essentially "reserved" for that process.

@embray
Copy link
Contributor Author

embray commented Mar 11, 2019

One could also argue that it would be better, rather than making this fix Cygwin-specific, there should be no harm in passing MAP_NORESERVE on Linux or in principle other platforms where it's defined. I think it is ignored if overcommit is disabled anyways.

For starters I kept it Cygwin-specific, but if you agree I could change that.

@ChrisJefferson ChrisJefferson merged commit e73953f into gap-system:master Mar 12, 2019
@embray embray deleted the kernel/cygwin-alloc-pool branch March 12, 2019 13:07
@embray
Copy link
Contributor Author

embray commented Mar 12, 2019

Any thoughts on the question of not making this cygwin-specific?

@ChrisJefferson
Copy link
Contributor

If we don't need it on Linux, I'd probably leave it, just because this code has in the past proved to be sensitive to changes causing problems on weird BSDs like DragonFly and the like, so (personally) I try not to change the memory allocation unless we are fixing a specific issue (like this one).

@fingolfin fingolfin added os: windows Issues and PRs that are (at least partially) specific to Windows topic: kernel labels Apr 15, 2019
@markusbaumeister markusbaumeister added the release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes label Apr 16, 2019
@fingolfin fingolfin changed the title Don't set SyAllocPool = 0 for Cygwin, but do use MAP_NORESERVE for mmap Reduce memory usage on windows when running external programs Aug 22, 2019
@fingolfin fingolfin added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 22, 2019
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.11.0 milestone Aug 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
os: windows Issues and PRs that are (at least partially) specific to Windows release notes: added PRs introducing changes that have since been mentioned in the release notes topic: kernel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants