#21582 closed enhancement (fixed)
PARI: use PROT_NONE for unused virtual stack memory
Reported by: | jdemeyer | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-7.4 |
Component: | packages: standard | Keywords: | |
Cc: | bober, leif | Merged in: | |
Authors: | Jeroen Demeyer | Reviewers: | Jonathan Bober |
Report Upstream: | Reported upstream. Developers acknowledge bug. | Work issues: | |
Branch: | ccd9c1e (Commits, GitHub, GitLab) | Commit: | |
Dependencies: | Stopgaps: |
Description (last modified by )
This ticket is meant to improve support for Linux with vm.overcommit = 2.
In Sage, the PARI stack takes a large amount of virtual memory but only a small part is usually used.
When allocating memory, this memory is "committed" which means that it is made available as virtual memory. However, as long as a program is not using that memory, this costs nothing because the kernel does not need to make available actual memory (physical or swap) for it. This is different with vm.overcommit = 2
: then all committed memory is counted. In that case, even unused virtual memory means that the system will have less memory available.
This can be solved by using the PROT_NONE
flag of mmap(2). This means that the memory cannot be read nor written. In this case, the kernel does not commit this memory (it does show up as virtual memory in top
). Note that this is not actually documented anywhere, but that is how it works.
This branch adds a PARI patch which implements this PROT_NONE
trick. We first allocate the PARI stack in the usual way (so the maximum stack size is limited by the available virtual memory, which makes sense) but then uncommit most of the stack by reallocating it with PROT_NONE
.
Finally, note that this patch does not functionally affect PARI (but I did run doctests just in case).
Upstream report: http://pari.math.u-bordeaux.fr/archives/pari-dev-1609/msg00008.html
Release manager: merge with #21637
Change History (25)
comment:1 Changed 6 years ago by
- Description modified (diff)
comment:2 Changed 6 years ago by
- Description modified (diff)
comment:3 Changed 6 years ago by
- Branch set to u/jdemeyer/pari__use_prot_none_for_unused_virtual_stack_memory
comment:4 Changed 6 years ago by
- Commit set to f54ecb5a5ef39fd67dd64e64d5f29840a33013d0
comment:5 Changed 6 years ago by
- Description modified (diff)
comment:6 Changed 6 years ago by
- Description modified (diff)
comment:7 Changed 6 years ago by
- Description modified (diff)
comment:8 Changed 6 years ago by
- Commit changed from f54ecb5a5ef39fd67dd64e64d5f29840a33013d0 to d165794ed50d715606fe7d5c101f5c5b258b5a57
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
d165794 | Improve support for Linux with vm.overcommit = 2
|
comment:9 Changed 6 years ago by
- Description modified (diff)
- Report Upstream changed from Not yet reported upstream; Will do shortly. to Reported upstream. No feedback yet.
- Status changed from new to needs_review
comment:10 Changed 6 years ago by
- Commit changed from d165794ed50d715606fe7d5c101f5c5b258b5a57 to ccd9c1ebb4393b9c0fc741fd4dc608a67fe7c536
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
ccd9c1e | Improve support for Linux with vm.overcommit = 2
|
comment:11 follow-up: ↓ 12 Changed 6 years ago by
- Reviewers set to Jonathan Bober
- Status changed from needs_review to positive_review
Looks good to me. make ptest gives a few unrelated doctest failures that seem to be completely unrelated (and also occur on other branches).
The memory commit accounting works as expected for me; an immediate test is that I can test and build the documentation in parallel, while I couldn't before. Another test is that I can get parisizemax in my gprc and watch the committed memory go up and then down during a significant computation.
Also, it is at least not completely broken on OS X: https://groups.google.com/d/msg/sage-devel/rw4h229qZbk/oxeKCX-kBgAJ
I just want to add that I don't really like that this seems to be relying on undocumented (as far as I can tell) low level memory management features, though it is behavior that seems to be used by other libraries that have interesting memory management strategies, such as openJDK and jemalloc. So be it.
comment:12 in reply to: ↑ 11 Changed 6 years ago by
Replying to bober:
I just want to add that I don't really like that this seems to be relying on undocumented (as far as I can tell) low level memory management features, though it is behavior that seems to be used by other libraries that have interesting memory management strategies, such as openJDK and jemalloc. So be it.
Indeed. It seems to be a more-or-less known trick, but it's not officially documented.
comment:13 Changed 6 years ago by
Thanks for the review by the way!
comment:14 Changed 6 years ago by
Thank you for fixing problems that might only affect me!
comment:15 Changed 6 years ago by
- Description modified (diff)
comment:16 Changed 6 years ago by
- Cc leif added
Curious as to whether I'll get a notification when this gets merged...
comment:17 Changed 6 years ago by
- Description modified (diff)
comment:19 Changed 6 years ago by
- Description modified (diff)
- Status changed from needs_work to positive_review
comment:20 Changed 6 years ago by
- Branch changed from u/jdemeyer/pari__use_prot_none_for_unused_virtual_stack_memory to ccd9c1ebb4393b9c0fc741fd4dc608a67fe7c536
- Resolution set to fixed
- Status changed from positive_review to closed
comment:21 Changed 6 years ago by
- Commit ccd9c1ebb4393b9c0fc741fd4dc608a67fe7c536 deleted
Jeroen, do you know what the status is upstream? The thread do not point to a final commit.
comment:22 Changed 6 years ago by
- Report Upstream changed from Reported upstream. No feedback yet. to Reported upstream. Developers acknowledge bug.
It's still being discussed on pari-dev (the mailing list website tracks messages per month, so you need to look at october 2016): http://pari.math.u-bordeaux.fr/archives/pari-dev-1610/msg00015.html
comment:23 Changed 6 years ago by
Note that this patch really only matters in very specific cases. So it won't matter much if you don't apply it for sage-on-gentoo.
comment:24 follow-up: ↓ 25 Changed 6 years ago by
Thanks for the info, saves me some work. It will give a doctest failure though.
comment:25 in reply to: ↑ 24 Changed 6 years ago by
Replying to fbissey:
It will give a doctest failure though.
True, but that might be easily fixed with a minor adjustment to my patch.
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
Improve support for Linux with vm.overcommit = 2