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

Important speed regression because pari is forced to be --mt=pthread #31572

Open
tornaria opened this issue Mar 28, 2021 · 10 comments
Open

Important speed regression because pari is forced to be --mt=pthread #31572

tornaria opened this issue Mar 28, 2021 · 10 comments

Comments

@tornaria
Copy link
Contributor

The giac upgrade to 1.6 in #30537 (9.3.beta9) made it necessary to switch pari to thread mode:

2c783788d3 build/pkgs/pari: Use Configure --mt=pthread
f49c6bf458 build/pkgs/pari/spkg-configure.m4: Check PARI_MT_ENGINE

The issue I find with this is that pari compiled with pthread is ~40% slower than pari in make test-all, e.g.:

  • single thread pari:
+++ Total bench for gp-sta is 275142
+++ Total bench for gp-dyn is 281436
  • pari with --mt=pthread:
+++ Total bench for gp-sta is 344547
+++ Total bench for gp-dyn is 399066

I think sage uses gp-dyn so that's a 40% slowdown (even using gp-sta there would be a 25% slowdown).

The performance hit might be even worse than that in some gp scripts, e.g.:

$ echo "sum(i=1,100 000 000, i);" | time gp -q
5.09user 0.00system 0:05.10elapsed 99%CPU (0avgtext+0avgdata 10700maxresident)k
0inputs+0outputs (0major+1568minor)pagefaults 0swaps
$ echo "sum(i=1,100 000 000, i);" | time sage-9.2/sage -gp -q
4.70user 0.04system 0:04.77elapsed 99%CPU (0avgtext+0avgdata 10496maxresident)k
16inputs+16outputs (0major+8362minor)pagefaults 0swaps
$ echo "sum(i=1,100 000 000, i);" | time sage-9.3.rc0/sage -gp -q
8.79user 0.02system 0:08.84elapsed 99%CPU (0avgtext+0avgdata 10852maxresident)k
8inputs+16outputs (0major+8729minor)pagefaults 0swaps

The performance of gp scripts may not be so important for sage, but it is for a system install of pari-gp, which means this places a conflict between what is good for a distro pari and what is required (?) for compiling sage. See void-linux/void-packages#29635 for an example of this.

CC: @mkoeppe @dkwo @antonio-rojas @sagetrac-parisse @kiwifb @pjbruin

Component: packages: standard

Keywords: pari

Issue created by migration from https://trac.sagemath.org/ticket/31572

@antonio-rojas
Copy link
Contributor

comment:1

This is required by giac, see #30537

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 28, 2021

Replying to @tornaria:

I couldn't find a relevant ticket.

$ git trac find 2c783788d3
Commit has been merged in Updated SageMath version to 9.3.beta8.
commit 8ff8b0b90c78d09fb2e71ee35205e96371e60f05
Merge: 1d5df64577 f05720bf63
Author: Release Manager <release@sagemath.org>
Date:   Mon Mar 1 01:25:23 2021 +0100

    Trac #30537: Upgrade giac to 1.6.0-47

@tornaria
Copy link
Contributor Author

comment:3

Thanks for the info (and sorry, I'm not familiar with git-trac, I see now that the way branches get merged I should have followed the branch up the tree to find 8ff8b0b, but that's not completely obvious at first sight).

For the record, I think #30537 comment:45 shows an example failure (doctesting src/sage/rings/polynomial/multi_polynomial_ideal.py).

I recompiled sage-9.3.rc0 with single thread pari and indeed I get a lot of failures in doctesting this file, the first one being the one in the comment quoted above. Weird enough, when I try to reproduce the doctest by hand, step-by-step, it does not fail (?).

What could be done about it? Having a 40% or more penalty for pari seems a huge cost to pay.

Is there an indication of when and where giac added this requirement?

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 29, 2021

comment:5

Replying to @tornaria:

I think #30537 comment:45 shows an example failure (doctesting src/sage/rings/polynomial/multi_polynomial_ideal.py).

That's right.

@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe changed the title Important speed regresion because pari is forced to be --mt=pthread Important speed regression because pari is forced to be --mt=pthread Mar 29, 2021
@kiwifb
Copy link
Member

kiwifb commented Mar 29, 2021

comment:8

For the record, debian and therefore ubuntu have shipped pari with threads for a while now. But their pari is also statically linked. Do we have speed regression examples with the library as well as the executable?

@mkoeppe mkoeppe modified the milestones: sage-9.3, sage-9.4 Apr 2, 2021
@dimpase
Copy link
Member

dimpase commented May 28, 2021

comment:10

Perhaps giac can use single-threaded Pari, as well? It can test whether Pari is single- or multi-threaded, and use different code paths for these two scenarios.

@roed314
Copy link
Contributor

roed314 commented Jun 10, 2021

comment:11

Another consequence of the change to --mt=pthread is that it makes it impossible to use Sage functions that depend on pari while running with threads, since pari requires special functions to be called when joining or leaving a thread. This showed up in the LMFDB as a segfault when starting the webserver; see also this cypari2 issue.

@mkoeppe mkoeppe modified the milestones: sage-9.4, sage-9.5 Aug 22, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.5, sage-9.6 Dec 14, 2021
@antonio-rojas
Copy link
Contributor

comment:15

FTR, upstream pari recommends compiling gp with -lto to make up for the performance loss caused by multithreading. We've been doing that in the Arch package for a while.

@tornaria
Copy link
Contributor Author

comment:16

Just a quick note, since I reported this I've given up and switched to pari with pthreads, since it seems just too difficult otherwise.

The mitigations I'm using are:

  • -flto -fno-semantic-interposition
  • link gp with static libpari

See: void-linux/void-packages#32453

@mkoeppe mkoeppe modified the milestones: sage-9.6, sage-9.7 Apr 7, 2022
@mkoeppe mkoeppe modified the milestones: sage-9.7, sage-9.8 Sep 19, 2022
@mkoeppe mkoeppe removed this from the sage-9.8 milestone Jan 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants