Opened 2 years ago

Closed 13 months ago

#29339 closed defect (invalid)

Fix NTL spkg-configure.m4 so it rejects NTLs built with NTL_THREADS

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: build: configure Keywords:
Cc: dimpase, fbissey, gh-mwageringel, jhpalmieri, jpflori Merged in:
Authors: Reviewers: Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: u/mkoeppe/fix_ntl_spkg_configure_m4_so_it_rejects_ntls_built_with_ntl_threads__without_ntl_gmp_lip__without_ntl_gf2x_lib (Commits, GitHub, GitLab) Commit: c3bc092ba9e7bea69092085b65231212f0367f6d
Dependencies: Stopgaps:

Status badges

Description (last modified by mkoeppe)

... such as homebrew's NTL (#29104).

Builds with NTL_THREADS breaks Singular (as observed in #29104).

In fact, as of the NTL upgrade ticket #20590, we build the NTL SPKG with NTL_THREADS=off. (On that ticket, it was noted "we cannot take advantage of the threading until a number of things are resolved. Threading requires C++11 and a number of packages are behind the curve.")

In a follow-up ticket we may consider issuing a warning if NTL is configured without NTL_GMP_LIP, without NTL_GF2X_LIB, which may lead to a performance degradation.

Change History (40)

comment:1 Changed 2 years ago by dimpase

the wise thing is to enable TLS as much as possible, IMHO. The configuration with enabled TLS on NTL, pari, and flint used to work, IIRC.

comment:2 Changed 2 years ago by dimpase

I also don't think gf2x should be a hard requirement. It is entirely optional for NTL.

comment:3 Changed 2 years ago by mkoeppe

Do you mean NTL_TLS_HACK?

comment:4 Changed 2 years ago by mkoeppe

homebrew's NTL is configured with NTL_TLS_HACK and NTL_THREADS, and I am getting the reported error with Singular. What do you suggest?

comment:6 Changed 2 years ago by mkoeppe

  • Cc jpflori added
  • Description modified (diff)

comment:7 Changed 2 years ago by dimpase

see #27764

comment:8 Changed 2 years ago by mkoeppe

  • Branch set to u/mkoeppe/fix_ntl_spkg_configure_m4_so_it_rejects_ntls_built_with_ntl_threads__without_ntl_gmp_lip__without_ntl_gf2x_lib

comment:9 Changed 2 years ago by mkoeppe

  • Authors set to Matthias Koeppe
  • Commit set to 96553db01783fcd6c86e451b0b16b4bcb88140f7
  • Status changed from new to needs_review

New commits:

96553dbbuild/pkgs/ntl/spkg-configure.m4: Check for NTL configuration

comment:10 follow-up: Changed 2 years ago by dimpase

once again - some distros build NTL without gf2x support, and it results in some deterioration of performance, but this is not something Sage should worry too much. Print a warning if you must.

After all, it is by far not the only place where the performance is suboptimal on pre-build installs, untuned for the hardware, say.

Last edited 2 years ago by dimpase (previous) (diff)

comment:11 Changed 2 years ago by mkoeppe

  • Description modified (diff)
  • Status changed from needs_review to needs_work
  • Summary changed from Fix NTL spkg-configure.m4 so it rejects NTLs built with NTL_THREADS, without NTL_GMP_LIP, without NTL_GF2X_LIB to Fix NTL spkg-configure.m4 so it rejects NTLs built with NTL_THREADS

comment:12 Changed 2 years ago by git

  • Commit changed from 96553db01783fcd6c86e451b0b16b4bcb88140f7 to c3bc092ba9e7bea69092085b65231212f0367f6d

Branch pushed to git repo; I updated commit sha1. New commits:

c3bc092Remove tests for NTL_GMP_LIP, NTL_GF2X_LIB

comment:13 Changed 2 years ago by mkoeppe

  • Status changed from needs_work to needs_review

comment:14 in reply to: ↑ 10 Changed 2 years ago by mkoeppe

Replying to dimpase:

once again - some distros build NTL without gf2x support, and it results in some deterioration of performance, but this is not something Sage should worry too much. Print a warning if you must.

OK, I have removed the tests

comment:15 Changed 2 years ago by dimpase

IMHO also pari/gp is typically built with threads on by distros. This means that TLS is enabled for pari/gp.

We may end up with having to build all the libs...

comment:16 Changed 2 years ago by mkoeppe

I don't see such problems in my tests

comment:17 Changed 2 years ago by dimpase

do you test with options forcing the use of system packages?

comment:18 Changed 2 years ago by mkoeppe

Not recently, will do

comment:19 Changed 2 years ago by mkoeppe

However, you can look for the new messages configure: Hint: The following SPKGs did not find equivalent system packages such as (https://github.com/mkoeppe/sage/runs/512242876) for ubuntu-focal-standard:

configure: Hint: The following SPKGs did not find equivalent system packages:
configure:   arb cbc cmake eclib fflas_ffpack flint fplll isl libatomic_ops libsemigroups ninja_build ntl perl_term_readline_gnu
checking for the package system in use... debian
configure: Hint: Installing the following system packages is recommended and may avoid building some of the above SPKGs from source:
configure:   $ sudo apt-get install libflint-arb-dev coinor-cbc coinor-libcbc-dev cmake libec-dev libflint-dev libisl-dev ninja-build libntl-dev libterm-readline-gnu-perl
configure: After installation, re-run configure using:
configure:   $ ./config.status --recheck && ./config.status

comment:20 Changed 2 years ago by dimpase

I think that the threads issue should be handled on per platform basis. If threads are not working on MacOS it does not mean that Sage on Linux should be made single-thread.

comment:21 follow-up: Changed 2 years ago by mkoeppe

Do you know a platform that actually builds NTL with threads?

comment:22 in reply to: ↑ 21 Changed 2 years ago by fbissey

Replying to mkoeppe:

Do you know a platform that actually builds NTL with threads?

Gentoo.

comment:23 Changed 2 years ago by mkoeppe

Well, does sage-the-distribution work with Gentoo's NTL?

comment:24 Changed 2 years ago by dimpase

yes it does, I think.

comment:25 Changed 2 years ago by mkoeppe

Shouldn't there be a ticket that enables NTL threads for Sage-the-distribution then? The NTL upgrade ticket #20590 disabled threads explicitly. So I assume there was a difficulty -- and there is no evidence that it has been resolved.

comment:26 Changed 2 years ago by dimpase

4 years ago the Python's single-thread religion still had many followers :-)

comment:27 follow-up: Changed 2 years ago by mkoeppe

No, seriously, when our own installation scripts avoid NTL's threads, then I think it is quite appropriate that the spkg-configure checks for that.

comment:28 Changed 2 years ago by dimpase

by the way, Fedora 30 (and on newer Fedoras too) also provides multithreaded NTL, and Sage happily uses it, tests pass.

$ uname -a
Linux clpc171.cs.ox.ac.uk 5.2.18-200.fc30.x86_64 #1 SMP Tue Oct 1 13:14:07 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
$ ldd /lib64/libntl.so
        linux-vdso.so.1 (0x00007ffe9affd000)
        libgmp.so.10 => /lib64/libgmp.so.10 (0x00007fabe7899000)
        libgf2x.so.1 => /lib64/libgf2x.so.1 (0x00007fabe7888000)
        libpthread.so.0 => /lib64/libpthread.so.0 (0x00007fabe7867000)
        libstdc++.so.6 => /lib64/libstdc++.so.6 (0x00007fabe766d000)
        libm.so.6 => /lib64/libm.so.6 (0x00007fabe7527000)
        libc.so.6 => /lib64/libc.so.6 (0x00007fabe7361000)
        /lib64/ld-linux-x86-64.so.2 (0x00007fabe7c33000)
        libgcc_s.so.1 => /lib64/libgcc_s.so.1 (0x00007fabe7345000)
Last edited 2 years ago by dimpase (previous) (diff)

comment:29 in reply to: ↑ 27 ; follow-up: Changed 2 years ago by dimpase

Replying to mkoeppe:

No, seriously, when our own installation scripts avoid NTL's threads, then I think it is quite appropriate that the spkg-configure checks for that.

I don't see why. Anyhow, this problem (inability to use multithreaded NTL) seems to be MacOS-only, and probably related to #27764

comment:30 follow-ups: Changed 2 years ago by fbissey

We historically had an issue before defaulting to C++11 but we should be able to move ahead in my opinion. I am expecting distros to all be threaded at this stage. It is just a matter of testing the insane number of corner cases.

comment:31 in reply to: ↑ 30 Changed 2 years ago by dimpase

Replying to fbissey:

We historically had an issue before defaulting to C++11 but we should be able to move ahead in my opinion. I am expecting distros to all be threaded at this stage. It is just a matter of testing the insane number of corner cases.

it seems to be that Debian is not providing multithreaded NTL (and the related libs, such as Flint, are single-threaded too).

comment:32 Changed 2 years ago by gh-mwageringel

For what it is worth, on macOS 10.13.6, Homebrew's NTL works for me without any problems. I last checked with a clean build of 9.1.beta4 and ptestlong passed.

comment:33 in reply to: ↑ 30 Changed 2 years ago by mkoeppe

Replying to fbissey:

We historically had an issue before defaulting to C++11 but we should be able to move ahead in my opinion. I am expecting distros to all be threaded at this stage. It is just a matter of testing the insane number of corner cases.

I have created ticket #29340 (Enable threads in NTL) for that, in case someone would like to work on it.

comment:34 in reply to: ↑ 29 Changed 2 years ago by mkoeppe

Replying to dimpase:

Replying to mkoeppe:

No, seriously, when our own installation scripts avoid NTL's threads, then I think it is quite appropriate that the spkg-configure checks for that.

I don't see why. Anyhow, this problem (inability to use multithreaded NTL) seems to be MacOS-only, and probably related to #27764

#27764 does not fix the problem on macOS, I've already tested that

comment:35 Changed 2 years ago by mkoeppe

  • Status changed from needs_review to needs_info

See #29366 (arch)

comment:36 Changed 2 years ago by mkoeppe

  • Milestone changed from sage-9.1 to sage-9.2

comment:37 Changed 23 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:38 Changed 17 months ago by mkoeppe

  • Authors Matthias Koeppe deleted
  • Milestone changed from sage-9.3 to sage-duplicate/invalid/wontfix
  • Status changed from needs_info to needs_review

I think this is no longer needed

comment:39 Changed 17 months ago by dimpase

  • Reviewers set to Dima Pasechnik
  • Status changed from needs_review to positive_review

ok

comment:40 Changed 13 months ago by mkoeppe

  • Resolution set to invalid
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.