Skip to content

Commit

Permalink
Trac #27214: libgap memory allocation on Cygwin
Browse files Browse the repository at this point in the history
As discussed in #27213, when we initialize libgap, we pass it the `-s`
flag which according to the docs is 'set the initially mapped virtual
memory', to a size determined by
`sage.interfaces.gap.get_gap_memory_pool_size()`, which on my system
happens to be ~3GB.

This is fine, in general, as it's an amount that's available to my
system.  Nonetheless, in most usage (especially e.g. in the test suite)
most of this will not be used.

I have to look into exactly how this memory gets allocated, but however
it's being allocated it is unfortunately "committed" memory, meaning
that although those pages are allocated lazily, they are guaranteed by
the system to be made available for that process, so regardless whether
those pages are actually in RAM, space is still reserved for them.  So
perhaps it uses sbrk to expand the process's heap.  When the process is
forked, Cygwin's `fork()` has to copy the parent process's heap into the
child.  This has the unfortunate effect of accessing those pages,
causing them to become allocated in physical memory (even, apparently,
if they're clean / unused).

This is essentially the same issue we faced with PARI in #22633, but
applied to GAP.  In fact, GAP's code sets the default value of the `-s`
flag to 0 on Cygwin, presumably for reasons related to this.  This might
be possible to avoid, as was done in PARI, by instead allocating this
memory with `mmap` and `MAP_NORESERVE`.

**Upstream PR:** gap-system/gap#3335

URL: https://trac.sagemath.org/27214
Reported by: embray
Ticket author(s): Erik Bray
Reviewer(s): Jeroen Demeyer
  • Loading branch information
Release Manager authored and vbraun committed Mar 19, 2019
2 parents 593f669 + 4b359de commit f2a8957
Showing 1 changed file with 78 additions and 0 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
From 78506f0a4fba05a238ec6e752c6d3cf2c8336bc8 Mon Sep 17 00:00:00 2001
From: "Erik M. Bray" <erik.bray@lri.fr>
Date: Tue, 26 Feb 2019 13:26:11 +0100
Subject: [PATCH] Don't set SyAllocPool = 0 for Cygwin, but do use
MAP_NORESERVE for mmaps 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.
---
src/sysmem.c | 16 +++++++++++-----
src/system.c | 4 ----
2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/src/sysmem.c b/src/sysmem.c
index 68a397b2f..995765828 100644
--- a/src/sysmem.c
+++ b/src/sysmem.c
@@ -282,6 +282,12 @@ int SyTryToIncreasePool(void)
#define MAP_ANONYMOUS MAP_ANON
#endif

+#ifdef SYS_IS_CYGWIN32
+#define GAP_MMAP_FLAGS MAP_PRIVATE|MAP_ANONYMOUS|MAP_NORESERVE
+#else
+#define GAP_MMAP_FLAGS MAP_PRIVATE|MAP_ANONYMOUS
+#endif
+
static void *SyMMapStart = NULL; /* Start of mmap'ed region for POOL */
static void *SyMMapEnd; /* End of mmap'ed region for POOL */
static void *SyMMapAdvised; /* We have already advised about non-usage
@@ -342,15 +348,15 @@ void *SyAnonMMap(size_t size) {
size = SyRoundUpToPagesize(size);
#ifdef SYS_IS_64_BIT
/* The following is at 16 Terabyte: */
- result = mmap((void *) (16L*1024*1024*1024*1024), size,
- PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
+ result = mmap((void *) (16L*1024*1024*1024*1024), size,
+ PROT_READ|PROT_WRITE, GAP_MMAP_FLAGS, -1, 0);
if (result == MAP_FAILED) {
result = mmap(NULL, size, PROT_READ|PROT_WRITE,
- MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
+ GAP_MMAP_FLAGS, -1, 0);
}
#else
result = mmap(NULL, size, PROT_READ|PROT_WRITE,
- MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
+ GAP_MMAP_FLAGS, -1, 0);
#endif
if (result == MAP_FAILED)
result = NULL;
@@ -371,7 +377,7 @@ int SyTryToIncreasePool(void)
size = (Int) SyMMapEnd - (Int) SyMMapStart;
newchunk = SyRoundUpToPagesize(size/2);
result = mmap(SyMMapEnd, newchunk, PROT_READ|PROT_WRITE,
- MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
+ GAP_MMAP_FLAGS, -1, 0);
if (result == MAP_FAILED) return -1;
if (result != SyMMapEnd) {
munmap(result,newchunk);
diff --git a/src/system.c b/src/system.c
index 7423d76bc..6f4c03517 100644
--- a/src/system.c
+++ b/src/system.c
@@ -1133,11 +1133,7 @@ void InitSystem (
#else
SyStorMin = 64 * 1024L;
SyStorMax = 1024*1024L; /* This is in kB! */
-#ifdef SYS_IS_CYGWIN32
- SyAllocPool = 0; /* works better on cygwin */
-#else
SyAllocPool = 1536L*1024*1024; /* Note this is in bytes! */
-#endif
#endif
SyStorOverrun = 0;
SyStorKill = 0;
--
2.15.1

0 comments on commit f2a8957

Please sign in to comment.