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

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

Closed
mkoeppe opened this issue Mar 15, 2020 · 41 comments
Closed

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

mkoeppe opened this issue Mar 15, 2020 · 41 comments

Comments

@mkoeppe
Copy link
Member

mkoeppe commented Mar 15, 2020

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

CC: @dimpase @kiwifb @mwageringel @jhpalmieri @jpflori

Component: build: configure

Branch/Commit: u/mkoeppe/fix_ntl_spkg_configure_m4_so_it_rejects_ntls_built_with_ntl_threads__without_ntl_gmp_lip__without_ntl_gf2x_lib @ c3bc092

Reviewer: Dima Pasechnik

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

@mkoeppe mkoeppe added this to the sage-9.1 milestone Mar 15, 2020
@dimpase
Copy link
Member

dimpase commented Mar 15, 2020

comment:1

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.

@dimpase
Copy link
Member

dimpase commented Mar 15, 2020

comment:2

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

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 15, 2020

comment:3

Do you mean NTL_TLS_HACK?

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 15, 2020

comment:4

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?

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 15, 2020

comment:5

Tests run at https://github.com/mkoeppe/sage/actions/runs/56215147

@mkoeppe

This comment has been minimized.

@dimpase
Copy link
Member

dimpase commented Mar 15, 2020

comment:7

see #27764

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 15, 2020

Commit: 96553db

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 15, 2020

New commits:

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

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 15, 2020

Author: Matthias Koeppe

@dimpase
Copy link
Member

dimpase commented Mar 16, 2020

comment:10

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.

@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe changed the title Fix NTL spkg-configure.m4 so it rejects NTLs built with NTL_THREADS, without NTL_GMP_LIP, without NTL_GF2X_LIB Fix NTL spkg-configure.m4 so it rejects NTLs built with NTL_THREADS Mar 16, 2020
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 16, 2020

Changed commit from 96553db to c3bc092

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 16, 2020

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

c3bc092Remove tests for NTL_GMP_LIP, NTL_GF2X_LIB

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 17, 2020

comment:14

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

@dimpase
Copy link
Member

dimpase commented Mar 17, 2020

comment:15

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

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 17, 2020

comment:16

I don't see such problems in my tests

@dimpase
Copy link
Member

dimpase commented Mar 17, 2020

comment:17

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

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 17, 2020

comment:18

Not recently, will do

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 18, 2020

comment:25

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.

@dimpase
Copy link
Member

dimpase commented Mar 18, 2020

comment:26

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

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 18, 2020

comment:27

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.

@dimpase
Copy link
Member

dimpase commented Mar 18, 2020

comment:28

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)

@dimpase
Copy link
Member

dimpase commented Mar 18, 2020

comment:29

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

@kiwifb
Copy link
Member

kiwifb commented Mar 18, 2020

comment:30

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.

@dimpase
Copy link
Member

dimpase commented Mar 18, 2020

comment:31

Replying to @kiwifb:

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

@mwageringel
Copy link

comment:32

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.

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 18, 2020

comment:33

Replying to @kiwifb:

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.

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 18, 2020

comment:34

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

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 19, 2020

comment:35

See #29366 (arch)

@mkoeppe mkoeppe modified the milestones: sage-9.1, sage-9.2 May 5, 2020
@mkoeppe mkoeppe modified the milestones: sage-9.2, sage-9.3 Aug 13, 2020
@mkoeppe
Copy link
Member Author

mkoeppe commented Feb 3, 2021

Changed author from Matthias Koeppe to none

@mkoeppe
Copy link
Member Author

mkoeppe commented Feb 3, 2021

comment:38

I think this is no longer needed

@mkoeppe mkoeppe removed this from the sage-9.3 milestone Feb 3, 2021
@dimpase
Copy link
Member

dimpase commented Feb 4, 2021

Reviewer: Dima Pasechnik

@dimpase
Copy link
Member

dimpase commented Feb 4, 2021

comment:39

ok

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

4 participants