#27214 closed defect (fixed)

libgap memory allocation on Cygwin

Reported by: embray Owned by: embray
Priority: blocker Milestone: sage-8.7
Component: porting: Cygwin Keywords:
Cc: Merged in:
Authors: Erik Bray Reviewers: Jeroen Demeyer
Report Upstream: Fixed upstream, but not in a stable release. Work issues:
Branch: 4b359de (Commits) Commit: 4b359de75fe85f481d889d0c0ace4086cb130088
Dependencies: #27070, #27490 Stopgaps:

Description (last modified by embray)

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: https://github.com/gap-system/gap/pull/3335

Change History (38)

comment:1 follow-up: Changed 12 months ago by embray

I think this is even sort of implicitly acknowledged in the GAP source code where it sets the default SyAllocPool = 0 if some macro SYS_IS_CYGWIN32 is defined (from the commit history it's hard to tell exactly why that was added, or if any consideration has been given whether the same issue affects 64-bit Cygwin).

I found that as a short term workaround passing -s 0 when initializing libgap on Cygwin is good enough. In that case it will allocate memory as-needed, which may have some performance impact when allocating lots of GAP objects, but for most usage I don't know that it will be noticeable (I'm not sure how best to test this--write a test that allocates lots of GAP objects?)

comment:2 in reply to: ↑ 1 ; follow-up: Changed 12 months ago by jdemeyer

Replying to embray:

I found that as a short term workaround passing -s 0 when initializing libgap on Cygwin is good enough.

That makes me wonder... if it's good enough for Cygwin, would it be good enough to do that on all systems unconditionally?

comment:3 Changed 12 months ago by embray

I don't know. The default on 64-bit systems is SyAllocPool = 4096L*1024*1024; /* Note this is in bytes! */ or 4GB. Our routine for determining get_gap_memory_pool_size() actually gives less than this on my system (which has 32GB). I don't really fully understand what the impact is of this in general, though I assumed there were good reasons for how we do it currently.

It's also a question of what is meant by "good enough". Like I wrote, this likely does have impact when allocating a number of large GAP objects; or at least it ensures that there is enough memory reserved for GAP. But I don't really have a good way at hand to measure that. Might have to dig through the history to see what motivated this in the first place.

For my purposes "good enough" is "it works, and most users will never tell the difference either way".

comment:4 Changed 12 months ago by embray

The current use of -s in the libgap interface was actually introduced by #13588, apparently in order to reduce it, and not out of any performance consideration.

comment:5 Changed 12 months ago by embray

Okay, I think I misunderstood exactly what -s does and how it impacts memory allocation in GAP (it doesn't help that a lot of documentation about this is out of date / inaccurate).

In fact it only uses the manual sbrk() calls in the specific case of -s 0. Otherwise, for -s > 0 (note: the internal variable that corresponds with -s is called SyAllocPool) it bypasses the manual sbrk() manipulations (thankfully) and instead allocates its own reserved pool for GAP objects, either using mmap (but only, for some reason or other, if madvise is available) or just one big calloc() call.

Cygwin does have madvise so I guess GAP is using mmap here. I think the trick to prevent this behavior is to pass MMAP_NORESERVE when making a big mmap on Cygwin, as this has the effect of preventing the MEM_COMMIT flag from being passed to the underlying VirtualAlloc call. This has an effect similar to overcommitting memory on Linux.

comment:6 in reply to: ↑ 2 Changed 12 months ago by embray

Replying to jdemeyer:

Replying to embray:

I found that as a short term workaround passing -s 0 when initializing libgap on Cygwin is good enough.

That makes me wonder... if it's good enough for Cygwin, would it be good enough to do that on all systems unconditionally?

So to answer your question, no that would not be a good idea on any systems, really.

comment:7 Changed 11 months ago by embray

  • Priority changed from major to critical

This issue is actually pretty bad I think. It doesn't immediately break anything, but it can become a real drag when doing anything that involves forking a lot of new processes, and in particular it can be bad when running the doctests in parallel.

comment:8 Changed 11 months ago by embray

What I do wonder is what changed that I'm only starting to notice this now. Before upgrading to GAP 4.10, we were still passing the same flags to Volker's libGAP, and I'm pretty sure it would have had the same behavior w.r.t. memory allocation.

Maybe just since we've merged some other changes to make more use of the libGAP interface the problem has become more pronounced. I'd have to install an older version of Sage to compare.

comment:9 Changed 11 months ago by embray

  • Priority changed from critical to blocker

I'm actually moving this up to blocker: I cannot, even with all other existing fixes applied, get the tests for sage.interfaces.gap to pass. Somewhere early on in that module's tests it invokes libgap. Then later, when it forks to start GAP subprocesses all of them are huge in memory usage. Something about the nature of the tests in this module are such that it forks enough subprocesses, all with the weight of this GAP memory allocation, that it eats up all 32GB of my system's memory and eventually crashes.

I'm still a little unclear as to why this has gotten so bad only with the recent GAP updates, but at least there are a couple possible workarounds, which I'll try shortly...

comment:10 Changed 11 months ago by embray

I tried applying a patch for GAP (just on Cygwin) to use MAP_NORESERVE when mmap-ing the GAP pool. When I run GAP directly, or even when I run the GAP API tests (in the GAP sources, that is) it appears to work fine, at least superficially.

However, when I use the libgap interface in Sage with this patch, it immediately segfaults:

$ ./sage
┌────────────────────────────────────────────────────────────────────┐
│ SageMath version 8.7.beta4, Release Date: 2019-02-15               │
│ Using Python 2.7.15. Type "help()" for help.                       │
└────────────────────────────────────────────────────────────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
┃ Warning: this is a prerelease version, and it may be unstable.     ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
sage: libgap(1)
------------------------------------------------------------------------
An error occurred during signal handling.
This probably occurred because a *compiled* module has a bug
in it and is not properly wrapped with sig_on(), sig_off().
Python will now terminate.
------------------------------------------------------------------------
Segmentation fault (core dumped)

which is completely bizarre and I can't explain it. I will dig in a bit to see what is happening here...

comment:11 Changed 11 months ago by embray

I see: When you create an mmap with MAP_NORESERVE on Cygwin, if you try to access the allocated region it creates without committing it results in an access violation--you can't read or write to that memory until it's been explicitly committed with an additional VirtualAlloc call.

Normally Cygwin handles this transparently: when its vectored exception handler catches a STATUS_ACCESS_VIOLATION it first checks if the access violation was on a memory address inside an mmap created with MAP_NORESERVE. If so, it the commits the page containing the address in question, and if that is successful the exception handler returns a special value (understood by the kernel ExceptionContinueExecution) which basically means the error has been handled, and it should retry the operation (in this case the memory access) that resulted in the exception.

This works fine normally, which is why I don't see any problem when running GAP directly. However, here's the tricky part: The reason this results in a SIGSEGV in Sage is because of Cysignals. Specifically, the custom "vectored continue handler" I added in cysignals#83 in order to better handle exceptions that occur on a sigaltstack. The reason I used a "vector continue handler" here is that, according to this invaluable analysis, a vectored continue handler can be run even when the kernel detects a failed SEH validation, which is what happens when you're suddenly running on a stack that Windows thinks is off in the weeds, as is the case with sigaltstack.

The problem is that vectored continue handlers also run after a normal exception occurs and the SEH handler returns ExceptionContinueExecution. This is what happens immediately after Cygwin's exception handler handles those explicit memory commits. So rather than returning from this particular exception normally, we wind up in Cysignals' win32_altstack_handler even though we should no longer be handling an exception, much less on an alt-stack.

One aspect of this that is not totally clear to me is why this doesn't happen normally: That is, why is this behavior normally avoided in cysignals' signal handling? If I had to wager a guess, it's simply that when cysignals does a longjmp from within the signal handler, we never return to the original exception handler in Cygwin and the vectored continue handler isn't run. Whereas, in this one obscure case, we wind up in the win32_altstack_handler before Cygwin even has a chance to send the process the SIGSEGV signal.

I need to see if there's way to modify win32_altstack_handler in order to detect this situation and avoid doing the wrong thing. For example, it should check whether or not we're actually running on the signal stack.

comment:12 Changed 11 months ago by embray

  • Owner changed from (none) to embray

comment:13 Changed 11 months ago by embray

One quite easy workaround: Just bail early out of win32_altstack_handler completely if cysigs.inside_signal_handler. There's no reason to do anything in that function if we're not handling a signal in the first place.

This could still result in the undesired behavior if we happen to access some uninitialized memory in a MAP_NORESERVE mmap from within a signal handler. This seems generally unlikely to me, but perhaps a more reliable solution should be found for that case as well. But as a starting point this solves the basic problem completely.

comment:14 Changed 11 months ago by embray

  • Dependencies set to #27384

comment:15 Changed 11 months ago by embray

  • Authors set to Erik Bray
  • Branch set to u/embray/cygwin/patch-gap-mmap-noreserve-pool
  • Commit set to f16f8102103b9eef812e5c8eb9089f9fb9238352
  • Status changed from new to needs_review

This adds a patch to GAP, which works fine on plain GAP but for use in Sage also requires #27384. By design the patch only affects GAP on Cygwin. In principle the same change could be made on other systems that support mmap() with the MAP_NORESERVE flag, though it's not clear whether or not that's strictly desirable.

It just means we can allocate a virtual address range for GAP's objects to live in up to any arbitrary size supported by the system, without regard to whether there is enough physical memory to use that entire address space (either at the time of allocation or sometime in the future) so it's still possible to run out of memory and get memory errors.

The reason this is needed on Cygwin is, as explained above, due to strange notions in Windows as to how copy-on-write pages are supposed to work, particularly in the context of making a MAP_PRIVATE mmap available to a child process after forking, which means that unfortunately such mmaps must be copied into the child process. Thus if a huge mmap needs to be made for GAP's memory pool, and then we fork the process, that entire mmap (even if much of it has not been written to yet) needs to copied in physical RAM. The only way to prevent this and keep the footprint of large mmaps small is to use the non-POSIX MAP_NORESERVE flag.


New commits:

f95dcffTrac #27384: Patch cysignals with fix for Cygwin's sigaltstack exception
f16f810Trac #27214: Patch GAP to allocate its memory pool using MAP_NORESERVE

comment:16 Changed 11 months ago by embray

  • Report Upstream changed from N/A to Not yet reported upstream; Will do shortly.

comment:17 follow-up: Changed 11 months ago by jdemeyer

  • Dependencies changed from #27384 to #27070
  • Status changed from needs_review to needs_work
  1. Wouldn't it make sense to use MAP_NORESERVE unconditionally (on all systems)?
  1. #27214 needs to be removed from this branch.

comment:18 in reply to: ↑ 17 ; follow-up: Changed 11 months ago by embray

Replying to jdemeyer:

  1. Wouldn't it make sense to use MAP_NORESERVE unconditionally (on all systems)?

It might be useful, but I would rather discuss that with upstream before imposing it that broadly. It may be useful on Linux by allowing to overcommit memory, but it isn't strictly needed to solve any specific problem I'm aware of.

  1. #27214 needs to be removed from this branch.

I assume you meant #27384.

comment:19 Changed 11 months ago by embray

  • Description modified (diff)

comment:20 in reply to: ↑ 18 ; follow-up: Changed 11 months ago by jdemeyer

Replying to embray:

Replying to jdemeyer:

  1. Wouldn't it make sense to use MAP_NORESERVE unconditionally (on all systems)?

It might be useful, but I would rather discuss that with upstream before imposing it that broadly. It may be useful on Linux by allowing to overcommit memory, but it isn't strictly needed to solve any specific problem I'm aware of.

Sure, maybe it doesn't solve any problem on Linux. But it's a good idea in general to avoid OS-specific fixes. Unless there is a good reason to apply this only on Cygwin, we should just apply it in general.

comment:21 in reply to: ↑ 20 Changed 11 months ago by embray

Replying to jdemeyer:

Replying to embray:

Replying to jdemeyer:

  1. Wouldn't it make sense to use MAP_NORESERVE unconditionally (on all systems)?

It might be useful, but I would rather discuss that with upstream before imposing it that broadly. It may be useful on Linux by allowing to overcommit memory, but it isn't strictly needed to solve any specific problem I'm aware of.

Sure, maybe it doesn't solve any problem on Linux. But it's a good idea in general to avoid OS-specific fixes. Unless there is a good reason to apply this only on Cygwin, we should just apply it in general.

I would agree if it weren't with something like GAP, where I'd rather have cooperation from upstream before making changes that could have impact other OS's (namely Linux in this case, I think) when it's not strictly necessary to. In other words, I think if a patch is being made to support one platform, we should keep it restricted to that one platform to minimize the effects on other platforms, which may result in bug reports that are sent to the project's developer, or something like that.

That's just as a general principle. I agree that in this case it should be low-impact on other platforms as well. But it's still a change that's better avoided for now if possible.

Also, I want to keep this ticket open a bit longer since I'm now experiencing another segfault, that curiously is only appearing (so far) during documentation building, and which seems like it might be related to this.

comment:22 Changed 11 months ago by embray

  • Report Upstream changed from Not yet reported upstream; Will do shortly. to Reported upstream. Developers acknowledge bug.

Let's see what the GAP folks say.

comment:23 Changed 11 months ago by embray

There is definitely a bizarre problem when initializing libgap from a process started by Python's multiprocessing module. For example:

>>> import multiprocessing
>>> def foo():
...     from sage.all import libgap
...     print(libgap(1))
>>> p = multiprocessing.Pool(1, maxtasksperchild=1)
>>> p.apply(foo)
1  # eventually
>>> p.apply(foo)  # again
... hangs ...

The hang in multiprocessing is due to this issue which we already know. The question is why is the child process dying unexpectedly. I added some additional debugging to the multiprocessing module and found that it's segfaulting (only the second time, not the first time, and it's a new process the second time due to maxtasksperchild=1). The segfault seems to originate from InitBags but that's all I can say for now, because even gdb is going haywire on this, which leads me to suspect a bug in Cygwin.

As a matter of fact, there was a bug in Cygwin fixed very recently (like just a couple weeks ago) that might be relevant to this, but I haven't confirmed. That is what I am going to check next.

I think the issue may be especially screwy because multiprocessing.Pool forks worker processes from within a thread, which technically is fine in principle, but is obviously thorny territory in practice, especially in conjunction with proper exception and signal handling. The bug that was fixed in Cygwin was in particular to do with exception handling on threads so I do believe it could be related. One of the tracebacks I managed to get from the segfault in InitBags() even showed:

#76 0x00000003bc41f842 in t_bootstrap ()
   from /home/embray/src/sagemath/sage/local/bin/libpython2.7.dll
#77 0x0000000180141a1b in pthread::thread_init_wrapper (arg=0x6021cdb30)
    at /usr/src/debug/cygwin-2.9.0-3/winsup/cygwin/thread.cc:2015
#78 0x00000001800bed31 in pthread_wrapper (arg=<optimized out>)
    at /usr/src/debug/cygwin-2.9.0-3/winsup/cygwin/miscfuncs.cc:459
#79 0x0000000000000000 in ?? ()
Backtrace stopped: previous frame inner to this frame (corrupt stack?)

which is fishy.

I believe a workaround might be to go ahead and initialize libgap before forking any subprocesses. Alternatively, just running the docbuild without multiprocessing, at least on Cygwin, is an okay workaround so long as it works.

Last edited 11 months ago by embray (previous) (diff)

comment:24 Changed 11 months ago by embray

  • Report Upstream changed from Reported upstream. Developers acknowledge bug. to Fixed upstream, but not in a stable release.

They would also prefer to keep it Cygwin-specific for now: https://github.com/gap-system/gap/pull/3335#issuecomment-472033940

I don't necessarily agree, but it's not worth fighting either.

comment:25 Changed 11 months ago by embray

After looking into this a bit more and obtaining some more stack traces, plus taking a look again at the multiprocessing.pool sources I see what's going on here:

In my example code in comment:23 the strange thing was that on the first p.apply(foo) it works, and then on the second one it segfaults. The reason for that is simply that Pool.__init__ directly calls Pool._repopulate_pool(), the method which forks off worker processes.

Thus, the N processes created at the time of Pool.__init__ are all forked directly from the main thread and function normally (in particular with the fix from #27384 in effect).

However, Pool then starts up a thread, as I mentioned previously, which is responsible for checking for completed worker processes and starting up new ones as needed. This is when things go awry. Cygwin supports forking from a thread just fine--perhaps too well in fact, because it successfully duplicates the thread and resumes execution on it in the new process, and this means it's susceptible to the bug I previously mentioned, where Cygwin exception handling wasn't working properly on threads, and in particular meaning that the first time accessing a mmap created with MAP_NORESERVE results immediately in an unhandled STATUS_ACCESS_VIOLATION, which Cysignals' win32_altstack_handler takes and exits the process with exit(-SIGSEGV) as though an unhandled SIGSEGV occurred.

I'm correct, then Cygwin 3.0.0 should fix that. I'm trying that next.

In the meantime a workaround is still needed. I would propose re-introducing the option to build the docs in serial, without using multiprocessing at all. This could be hidden normally much like the doctest runner's --serial flag, and on Cygwin use that at least for Cygwin<3.0.0.

Another option we've talked about before is replacing multiprocessing.Pool with something else which doesn't require threads, such as a generalization of sage.doctest.forker. I would still like to do that. But I think in the short-term a simpler hack is needed. After all, most other problems with using multiprocessing here have been otherwise dealt with, it's just this last stupid issue with Cygwin+gap+mmap+fork :(

comment:26 Changed 11 months ago by embray

Yep, Cygwin 3.0 fixes it. So that's good news at least. I'll make sure Cygwin>=3.0 is used for new builds of Sage and for the buildbot (probably >=3.0.4 since 3.0.0 had some unrelated problems with fork()). It will still be good to have a workaround though.

comment:27 Changed 11 months ago by embray

Okay, I'm going to rebase this to work with #27070, and I'm adding a little patch I'm experimenting with currently to work around the docbuild issue on older Cygwins that have this bug.

comment:28 Changed 11 months ago by embray

  • Dependencies changed from #27070 to #27070, #27490

comment:29 follow-up: Changed 11 months ago by vbraun

I think I mentioned the MAP_NORESERVE once to gap developers and they were sympathetic; IMHO it should definitely be upstreamed.

Realistically, this ticket is not going to make it for Sage 8.7 though.

comment:30 Changed 11 months ago by embray

  • Status changed from needs_work to needs_review

comment:31 in reply to: ↑ 29 Changed 11 months ago by embray

Replying to vbraun:

I think I mentioned the MAP_NORESERVE once to gap developers and they were sympathetic; IMHO it should definitely be upstreamed.

Realistically, this ticket is not going to make it for Sage 8.7 though.

What does "realistically" mean in this case? On what basis? I gave you plenty of advance notice that it's needed.

comment:32 Changed 11 months ago by jdemeyer

  • Status changed from needs_review to needs_work

Did you forget to update the branch to #27070?

comment:33 Changed 11 months ago by git

  • Commit changed from f16f8102103b9eef812e5c8eb9089f9fb9238352 to 4b359de75fe85f481d889d0c0ace4086cb130088

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

14b1551Upgrade to Cysignals 1.10.2
a2ac9c7cysignals should be a normal dependency
4b359deTrac #27214: Patch GAP to allocate its memory pool using MAP_NORESERVE

comment:34 Changed 11 months ago by embray

  • Status changed from needs_work to needs_review

I thought I did, but apparently not? I must have pushed to a different remote.

comment:35 Changed 11 months ago by jdemeyer

Please add a reference to the upstream PR in the .patch file. Once you did that, you can set this to positive review.

comment:36 Changed 11 months ago by jdemeyer

  • Reviewers set to Jeroen Demeyer

comment:37 Changed 11 months ago by embray

  • Status changed from needs_review to positive_review

comment:38 Changed 10 months ago by vbraun

  • Branch changed from u/embray/cygwin/patch-gap-mmap-noreserve-pool to 4b359de75fe85f481d889d0c0ace4086cb130088
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.