sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.35k stars 461 forks source link

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

Open tornaria opened 3 years ago

tornaria commented 3 years ago

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.:

+++ Total bench for gp-sta is 275142
+++ Total bench for gp-dyn is 281436
+++ 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 https://github.com/void-linux/void-packages/pull/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 commented 3 years ago
comment:1

This is required by giac, see #30537

mkoeppe commented 3 years ago

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 commented 3 years ago
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 commented 3 years ago
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 commented 3 years ago

Description changed:

--- 
+++ 
@@ -1,10 +1,9 @@
-In sage-9.3.beta9, the following two commits showed up:
+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

-There is no indication at all of why this change is necessary, no doctest to exhibit a bug without it, and I couldn't find a relevant ticket.

 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:
@@ -36,4 +35,4 @@

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 https://github.com/void-linux/void-packages/pull/29635 for an example of this.

-AFAIK pari has been always compiled without pthread, and I don't recall issues related to that. +

kiwifb commented 3 years ago
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?

dimpase commented 3 years ago
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 commented 3 years ago
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.

antonio-rojas commented 2 years ago
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 commented 2 years ago
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:

See: https://github.com/void-linux/void-packages/pull/32453