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

configure threading in flint in sync with NTL #27764

Open
dimpase opened this issue May 3, 2019 · 30 comments
Open

configure threading in flint in sync with NTL #27764

dimpase opened this issue May 3, 2019 · 30 comments

Comments

@dimpase
Copy link
Member

dimpase commented May 3, 2019

By default, TLS (aka Thread-Local Storage) is enabled in flint.
However, it contradicts to NTL configured without threads (NTL_THREADS=off), and in particular non-GNU linkers might refuse to link flint
(actually happens on FreeBSD).

Also happens on macOS -- see for example https://groups.google.com/d/msg/sage-release/hobZzw74Rv0/VkAv7bG6DAAJ

See also:

CC: @embray @kiwifb @mkoeppe @dcoudert

Component: build

Author: Dima Pasechnik

Branch/Commit: u/dimpase/packages/flint-no-tls @ 2945e30

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

@dimpase dimpase added this to the sage-8.8 milestone May 3, 2019
@dimpase
Copy link
Member Author

dimpase commented May 3, 2019

Branch: u/dimpase/packages/flint-no-tls

@dimpase
Copy link
Member Author

dimpase commented May 3, 2019

New commits:

386dd3fdisable TLS in flint

@dimpase
Copy link
Member Author

dimpase commented May 3, 2019

Author: Dima Pasechnik

@dimpase
Copy link
Member Author

dimpase commented May 3, 2019

Commit: 386dd3f

@kiwifb
Copy link
Member

kiwifb commented May 3, 2019

comment:3

By default the latest ntl should use threads - when the compiler is C++11 capable. Is it disabled on some OS/archs?

@dimpase
Copy link
Member Author

dimpase commented May 3, 2019

@dimpase
Copy link
Member Author

dimpase commented May 3, 2019

comment:5

an intelligent setup would sync this between dependencies automatically, but this seems to be not so easy.

@kiwifb
Copy link
Member

kiwifb commented May 3, 2019

comment:6

Replying to @dimpase:

this is what Sage does, it disables threads in NTL.
https://github.com/sagemath/sage-prod/blob/5ba5a5a40cd33901c5f4307d59aa68a4359b0430/build/pkgs/ntl/spkg-install#L103

Well, we shouldn't anymore. I should have taken care of this during the ntl upgrade and things about C++11 standard. Unless there is a compelling reason to do so, I would recommend removing NTL_THREADS=off from ntl's spkg-install.

@kiwifb
Copy link
Member

kiwifb commented May 3, 2019

comment:7

On the other hand, flint's configuration system is un-impressive to me. And flint uses C++ for a grand total of compiling one file for the ntl interface. Which is optional. I have no idea if this is useful for anything.

@dimpase dimpase changed the title configure flint with --disable-tls configure threading in flint in sync with NTL May 3, 2019
@dimpase
Copy link
Member Author

dimpase commented May 3, 2019

comment:9

flint’s interface to NTL is used in sagelib. So it cannot just be switched off.

@kiwifb
Copy link
Member

kiwifb commented May 3, 2019

comment:10

Replying to @dimpase:

flint’s interface to NTL is used in sagelib. So it cannot just be switched off.

OK. That make something clearer to me.

@embray
Copy link
Contributor

embray commented Jul 3, 2019

comment:11

Moving tickets from the Sage 8.8 milestone that have been actively worked on in the last six months to the next release milestone (optimistically).

@embray embray modified the milestones: sage-8.8, sage-8.9 Jul 3, 2019
@kiwifb
Copy link
Member

kiwifb commented Jul 16, 2019

comment:12

Needs rebasing.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 17, 2019

Changed commit from 386dd3f to 2945e30

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 17, 2019

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

2945e30disable TLS in flint

@dimpase
Copy link
Member Author

dimpase commented Jul 17, 2019

comment:14

OK, done.

@kiwifb
Copy link
Member

kiwifb commented Jul 18, 2019

comment:15

OK now that I see again what was done and that I remember the conversation let's move on a bit. There are two options

  • enabling threads in NTL and letting flint do the same
  • leaving threads disabled in NTL and applying this ticket

Is there any reason or platforms preventing us from enabling threads in NTL? We historically disabled threads in NTL because it relied on C++11 and we weren't on board with it for all packages and that was causing problems on downstream packages where c++11 wasn't on. This should be solved now. So is there another reason we cannot enable threads in NTL?

@dimpase
Copy link
Member Author

dimpase commented Jul 18, 2019

comment:16

I suppose enabling threads in NTL and flint, and all its dependencies e.g. arb, needs testing on all platforms we claim to support.

@kiwifb
Copy link
Member

kiwifb commented Jul 18, 2019

comment:17

That's a bot job. Literally. We should make a threading branch and see how the patch bots likes it.

@embray
Copy link
Contributor

embray commented Dec 30, 2019

comment:18

Ticket retargeted after milestone closed

@embray embray modified the milestones: sage-8.9, sage-9.1 Dec 30, 2019
@mkoeppe mkoeppe modified the milestones: sage-9.2, sage-9.3 Aug 13, 2020
@dimpase
Copy link
Member Author

dimpase commented Aug 23, 2020

comment:25

yes, this happens on macOS 10.15 too, it seems

[sagelib-9.2.beta9] clang++ -bundle -undefined dynamic_lookup -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk -L/Users/dima/software/sagetrac-mirror/local/lib -Wl,-rpath,/Users/dima/software/sagetrac-mirror/local/lib build/temp.macosx-10.15-x86_64-3.7/build/cythonized/sage/matrix/matrix_integer_sparse.o -L/usr/local/Cellar/openblas/0.3.10_1/lib -L/Users/dima/software/sagetrac-mirror/local/lib -L/usr/local/lib -L/usr/local/opt/[email protected]/lib -L/usr/local/opt/sqlite/lib -llinbox -lntl -liml -lfflas -lffpack -lgivaro -lflint -lmpfr -lgmp -lgmpxx -lopenblas -o build/lib.macosx-10.15-x86_64-3.7/sage/matrix/matrix_integer_sparse.cpython-37m-darwin.so -lpari
[sagelib-9.2.beta9] ld: illegal thread local variable reference to regular symbol __ZN3NTL20ZZXFac_InitNumPrimesE for architecture x86_64

@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe modified the milestones: sage-9.3, sage-9.2 Sep 9, 2020
@mkoeppe

This comment has been minimized.

@dimpase
Copy link
Member Author

dimpase commented Oct 7, 2020

comment:28

from what I recall - I was able to build and test Sage 9.2.beta12 on Homebrew macOS 10.15.6
using Homebrew's Flint and NTL, which do come with threads enabled. (Unfortunately the machine is bricked now, so I can't verify).
Moving this to 9.3.

@dimpase dimpase modified the milestones: sage-9.2, sage-9.3 Oct 7, 2020
@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 24, 2021

comment:30

Sage development has entered the release candidate phase for 9.3. Setting a new milestone for this ticket based on a cursory review of ticket status, priority, and last modification date.

@mkoeppe mkoeppe modified the milestones: sage-9.3, sage-9.4 Mar 24, 2021
@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 19, 2021

comment:31

Setting a new milestone for this ticket based on a cursory review.

@mkoeppe mkoeppe modified the milestones: sage-9.4, sage-9.5 Jul 19, 2021
@mkoeppe
Copy link
Contributor

mkoeppe commented Dec 18, 2021

comment:32

Stalled in needs_review or needs_info; likely won't make it into Sage 9.5.

@mkoeppe mkoeppe modified the milestones: sage-9.5, sage-9.6 Dec 18, 2021
@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

4 participants