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

pari: update to 2.13.2. #32453

Merged
merged 3 commits into from Aug 15, 2021
Merged

pari: update to 2.13.2. #32453

merged 3 commits into from Aug 15, 2021

Conversation

tornaria
Copy link
Contributor

@tornaria tornaria commented Aug 11, 2021

  • enable pthreads build
  • switch gp binary to static link libpari; this offsets the slowdown due to pthreads
  • use -flto and -fno-semantic-interposition which improve speed a little bit more
  • ship the static library, so external programs can static link
  • change of maintainer agreed by sgn
  • revbump giac and qcas

Rationale:

  • pthreads gives an interesting functionality, and recent versions of giac require pari with tls; sagemath requires pari with pthreads.
  • The cost for enabling pthreads is quite high, on the order of 30-40% for some tests; -flto saves maybe 1-2% at most, -fno-semantic-interposition saves 5-7%, but we are still about 25-30% behind.
  • On the other hand, switching to static binary yields some savings on the order of 5-10%
  • More important: the cost of enabling pthread with -flto for a static binary is only about 5-10%
  • In summary: enabling pthread while switching to static is roughly the same speed (even maybe 5-10% better overall) as current binary, with an interesting feature (esp. if one has many cores) and apparently better compatibility.
  • Shipping static library as well is easy and allows for external code linking to the static library.

@sgn: if you agree, I am willing to take over maintainership. I have used pari for 25+ years, and I use it almost daily.

Have the results of the proposed changes been tested?

  • I use the packages affected by the proposed changes on a regular basis and confirm this PR works for me

@sgn
Copy link
Member

sgn commented Aug 11, 2021

The package is yours.

@ericonr
Copy link
Member

ericonr commented Aug 11, 2021

switch gp binary to static link libpari; this offsets the slowdown due to pthreads

Can you try the -fno-semantic-interposition compiler argument? Python has started using it for libpython for similar performance gains. I think it might allow us to keep the space gain of dynamic linking and the speed gain of static linking.

@tornaria
Copy link
Contributor Author

Can you try the -fno-semantic-interposition compiler argument? Python has started using it for libpython for similar performance gains. I think it might allow us to keep the space gain of dynamic linking and the speed gain of static linking.

A quick informal test shows that while adding that option improves the dynamic binary a bit, it's not close to recovering what is lost from pthreads. Running the script below with the current version in the repos (no pthreads, no CFLAGS):

$ gp -q regress-best5.gp
4470 4491 4500 4532 4509   best=4470

Now with the new version, enabling pthreads:

  • CFLAGS=""
    gp-dyn: 5970 6000 6006 6010 6020 best=5970
    gp-sta: 4219 4280 4210 4154 4203 best=4154

  • CFLAGS="-flto"
    gp-dyn: 6226 6168 6145 6196 6095 best=6095
    gp-sta: 4166 4045 4028 4010 4013 best=4010

  • CFLAGS="-flto -fno-semantic-interposition"
    gp-dyn: 5669 5680 5702 5686 5628 best=5628
    gp-sta: 3900 3934 4050 3921 3963 best=3900

So for this particular test, there's a 40% loss by going dynamic with -fno-semantic-interposition instead of 50% loss without.

Note that we are NOT static linking system libraries. It's only libpari that is statically linked into the gp instead of being dynamically linked.


  • regress-best_of_5.gp
sigmatwist1(n,k)=
{
   if(denominator(n)>1||n<=0,return(0));
   n>>=valuation(n,2);
   sumdiv(n,d,if(d%4==1,d^k,-d^k));
}

sumtwist1(k,m,N)=
{
   sigmatwist1(m/N,k-1)+2*sum(s=1,sqrtint(m),sigmatwist1((m-s^2)/N,k-1));
}

a={vector(5,i,
  gettime();
  sumtwist1(3,(10^12+4)/4,1);
  t=gettime();
  print1(t," ");
  t)};
  print("  best=",vecmin(a));
\q

@tornaria
Copy link
Contributor Author

Failures are due to a dependant package (giac) which is affected since the soname changed (because of the switch to pthreads).

I'll deal with that, and maybe take the chance to upgrade giac.

@ericonr
Copy link
Member

ericonr commented Aug 12, 2021

I think linking it in statically is valid. Quite unfortunate that the flag wasn't enough, but it might be more pronounced on python than on gp, especially if the binary makes lots of calls to the library instead of a single call and now the rest runs inside the library.

@tornaria
Copy link
Contributor Author

All tests are ok, except on aarch64 and aarch64-musl it's trying to build qt5, which is a dependency of qcas.

I think pari and giac are built ok before that, so I'd take this as check ok for all architectures.

I didn't change qcas at all, only revbumped and fix a xlint in the short_desc. As a matter of fact, the current version of qcas in the repos is already broken before me changing pari due to a missing symbol; maybe no one uses qcas (I don't). Anyway after this PR it works at least on x86-64 so it's definitely an improvement.

srcpkgs/giac/patches/pari_2_13.patch Show resolved Hide resolved
srcpkgs/pari/template Outdated Show resolved Hide resolved
common/shlibs Outdated Show resolved Hide resolved
- enable pthreads build
- switch gp binary to static link libpari; this offsets the slowdown due
  to pthreads
- use -flto and -fno-semantic-interposition which improve speed a little
  bit more
- ship the static library, so external programs can static link
- change of maintainer agreed by sgn

I had to add a minimal patch to the makefile, so that the build step
builds and the install step installs. I will try to upstream that patch.
@tornaria
Copy link
Contributor Author

If all checks pass, then it's ready. Thanks a lot for your careful and constructive review.

@ericonr
Copy link
Member

ericonr commented Aug 15, 2021

aarch64 is just a tad stuck :p

Thanks!

@ericonr ericonr merged commit ef30eca into void-linux:master Aug 15, 2021
@tornaria tornaria deleted the pari branch August 18, 2021 20:25
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants