Opened 19 months ago

Closed 18 months ago

Last modified 18 months ago

#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) Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

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 19 months ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 19 months ago by jdemeyer

  • Description modified (diff)

comment:3 Changed 19 months ago by jdemeyer

  • Branch set to u/jdemeyer/pari__use_prot_none_for_unused_virtual_stack_memory

comment:4 Changed 19 months ago by git

  • Commit set to f54ecb5a5ef39fd67dd64e64d5f29840a33013d0

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

f54ecb5Improve support for Linux with vm.overcommit = 2

comment:5 Changed 19 months ago by jdemeyer

  • Description modified (diff)

comment:6 Changed 19 months ago by jdemeyer

  • Description modified (diff)

comment:7 Changed 19 months ago by jdemeyer

  • Description modified (diff)

comment:8 Changed 19 months ago by git

  • Commit changed from f54ecb5a5ef39fd67dd64e64d5f29840a33013d0 to d165794ed50d715606fe7d5c101f5c5b258b5a57

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

d165794Improve support for Linux with vm.overcommit = 2

comment:9 Changed 19 months ago by jdemeyer

  • 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 19 months ago by git

  • Commit changed from d165794ed50d715606fe7d5c101f5c5b258b5a57 to ccd9c1ebb4393b9c0fc741fd4dc608a67fe7c536

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

ccd9c1eImprove support for Linux with vm.overcommit = 2

comment:11 follow-up: Changed 19 months ago by bober

  • 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 19 months ago by jdemeyer

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 19 months ago by jdemeyer

Thanks for the review by the way!

comment:14 Changed 19 months ago by bober

Thank you for fixing problems that might only affect me!

comment:15 Changed 19 months ago by jdemeyer

  • Description modified (diff)

comment:16 Changed 19 months ago by leif

  • Cc leif added

Curious as to whether I'll get a notification when this gets merged...

comment:17 Changed 19 months ago by jdemeyer

  • Description modified (diff)

comment:18 Changed 18 months ago by vbraun

  • Status changed from positive_review to needs_work

See patchbot

comment:19 Changed 18 months ago by jdemeyer

  • Description modified (diff)
  • Status changed from needs_work to positive_review

The reason is that several tickets bump the PARI version number.

#21622 has an explicit instruction to merge it together with #21582 (this ticket). That did not happen.

Instead, please merge this together with #21637, which also includes a PARI version bump.

comment:20 Changed 18 months ago by vbraun

  • 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 18 months ago by fbissey

  • Commit ccd9c1ebb4393b9c0fc741fd4dc608a67fe7c536 deleted

Jeroen, do you know what the status is upstream? The thread do not point to a final commit.

comment:22 Changed 18 months ago by jdemeyer

  • 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 18 months ago by jdemeyer

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: Changed 18 months ago by fbissey

Thanks for the info, saves me some work. It will give a doctest failure though.

comment:25 in reply to: ↑ 24 Changed 18 months ago by jdemeyer

Replying to fbissey:

It will give a doctest failure though.

True, but that might be easily fixed with a minor adjustment to my patch.

Note: See TracTickets for help on using tickets.