-
-
Notifications
You must be signed in to change notification settings - Fork 489
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
sage-loaded libraries are slowing down cypari2 a lot (on x86_64 glibc) #34850
Comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
comment:3
Could it be just a different memory layout as soon as |
comment:5
FWIW, I don't see the issue on my system:
(edit: I previously posted a version where I was running the test code under sage, not ipython, but the results above are with ipython) |
comment:6
Thanks Marc for running the test. What is your configuration? eg what is the system and does sage use system pari? |
comment:7
I was going to add more details after doit |
comment:8
on ubuntu 22.04.01 LTS:
|
comment:9
If you replace
with
you'll get the same slowdown. Bingo! Sympy probably changes FPU status, it seems, according to |
comment:10
Replying to Dima Pasechnik:
Did you accidentally look in I (obviously) don't see any slowdown here with |
comment:11
You know that the greatest discoveries are made by accident ;-) Indeed, I went looking for FPU in sage source repo, and found that
|
comment:12
I see this I see a bit of a speedup, by about 15%, with either |
comment:13
Replying to Dima Pasechnik:
Interesting.
|
comment:14
narrowing it down further, with taking all of Sage out: # p1.py
import timeit
prog="""
from cypari2 import Pari
pari = Pari(size={size})
pari("{cmd}")
"""
prog = prog.format(size=2**24, cmd="quadclassunit(1 - 2^100)")
print("python environment with cypari2: ",
timeit.timeit(prog, number=3))
setup="""
import sympy
"""
print("with sympy loaded: ",
timeit.timeit(prog, setup=setup, number=3)) On Gentoo with system
So this is down to interaction between python, sympy, and cypari2 (and Sage's venv?) |
comment:15
Matthias, there is something funny in Sage's venv, causing such a slowdown on Linux. |
comment:16
To narrow it down further, you can try installing the Sage-built wheels (from venv/var/lib/sage/wheels/) in a separate venv. Specifically, take cypari2 from the sage wheel rather than from pypi. |
comment:17
Replying to Matthias Köppe:
This way, no difference in timing, so the wheels themselves are OK. |
comment:18
Next you could check whether it makes a difference to run the separate venv inside or outside of the sage environment set up by |
comment:19
if I start
regardless,
|
comment:20
Replying to Dima Pasechnik:
Indeed. I've opened #34853 for this unrelated issue. |
comment:21
I'd suggest to look whether mpmath is somehow involved, see #25445 |
comment:22
Replying to Matthias Köppe:
In case, it's exactly the same wheels, So we can now test instead the following: # p1.py
import timeit
prog="""
from cypari2 import Pari
pari = Pari(size={size})
pari("{cmd}")
"""
prog = prog.format(size=2**24, cmd="quadclassunit(1 - 2^100)")
print("python environment with cypari2: ",
timeit.timeit(prog, number=3))
setup="""
import mpmath
"""
print("with mpmath loaded: ",
timeit.timeit(prog, setup=setup, number=3)) |
comment:23
Try if the various |
comment:24
Oops, we're back to if ('MPMATH_NOSAGE' not in os.environ and 'SAGE_ROOT' in os.environ or
'MPMATH_SAGE' in os.environ):
try:
import sage.all
import sage.libs.mpmath.utils as _sage_utils
sage = sage.all
sage_utils = _sage_utils
BACKEND = 'sage'
MPZ = sage.Integer
except:
pass and skipping it indeed cures the discrepancy.
sorry for the bogus lead :-) The question is now - can we import less than |
comment:25
Can you reproduce it if you replace |
comment:26
Replying to Matthias Köppe:
Success with
vs
for |
comment:27
and |
comment:103
I guess the glibc patch got lost because it was not sent at the proper place. I have put a comment on the bugzilla page. |
comment:104
I can confirm that the workaround provided in https://sourceware.org/bugzilla/show_bug.cgi?id=19924 |
comment:105
That's my experiment:
and hit |
comment:106
with the following simple change in
However you might have to do the same for any library that has TLS variables. |
comment:107
Replying to Paul Zimmermann:
It has been sent to libc-alpha too: |
comment:108
right, I will ask Szabolcs if he can post a new version as requested. |
comment:109
Hi Marc, Replying to Marc Mezzarobba:
I don't see what workaround MPFR could implement. If I understand correctly, there are 2 things to avoid the bug: either change the order of the library loading, but this is something that can be done only at the application or user level, or access an MPFR thread-local variable, but I don't think that this is possible at library load time on the MPFR side, i.e. this is also something that can be done only at the application level (see Paul's suggestion, consisting in calling |
comment:110
Replying to Paul Zimmermann:
alternatively, and certainly easier w.r.t. software complexity (vendors of libraries don't have to do anything), before starting any computations, one can dlopen a dummy library __thread int dummy_tls_for_lib = 23;
void dummy_access()
{
dummy_tls_for_lib++;
} and access a TLS variable, as in https://git.sr.ht/~dimpase/tls_workaround/tree/main/item/p.c#L34 void* handle = dlopen("./libdummy.so", RTLD_NOW);
void (*dummy_access)(void);
*(void**)(&dummy_access) = dlsym(handle, "dummy_access");
dummy_access(); |
comment:111
Replying to Paul Zimmermann:
frankly speaking, it looks as if glibc is stalling on this bug since, like, forever... |
comment:112
Replying to Vincent Lefèvre:
Okay, I assumed to could be done at load time, but you definitely know better :-) |
comment:113
Replying to Paul Zimmermann:
Linking with
Here the |
comment:114
Replying to Vincent Lefèvre:
Just for fun I rebuilt mpfr-4.1.1 with the following silly patch and voilà, the issue goes away: --- a/src/exceptions.c 2022-01-06 12:27:38.000000000 -0300
+++ b/src/exceptions.c 2023-01-06 10:32:49.845632284 -0300
@@ -34,6 +34,12 @@
return __gmpfr_emin;
}
+static void __attribute__ ((constructor))
+__dummy_access_tls (void)
+{
+ __gmpfr_emin = MPFR_EMIN_DEFAULT;
+}
+
#undef mpfr_set_emin
int |
comment:115
Replying to Gonzalo Tornaría:
But note that |
comment:116
Replying to Vincent Lefèvre:
Fortunately, the bug is also a GNU extension. Just guard the whole thing inside MPFR_HAVE_CONSTRUCTOR_ATTR and some GLIBC defined macro. |
comment:117
Replying to Gonzalo Tornaría:
GNU inaction, rather... Time for another
and it's |
comment:118
Replying to Gonzalo Tornaría:
Things are more complex. Such GNU extensions at compiler level are available on various systems, and it is not clear that they are working correctly on other systems. In theory, this should have no effect on such systems, but it is not impossible to trigger a bug yielding an incorrect behavior. In particular, this feature is probably used very little at library load time. So I'm wondering whether it has been fully tested on various systems. Enabling such a workaround via a configure option should be safer. |
comment:119
anyway, the issue is clearly on the glibc side, and it is not a good idea to try to workaround any compiler/library issue on the mpfr side. But of course Sage developers can patch their mpfr version if they like. |
comment:120
a new version of the patch: https://patchwork.ozlabs.org/project/glibc/patch/20221019111439.63501-1-szabolcs.nagy@arm.com/ and the corresponding discussion: https://sourceware.org/pipermail/libc-alpha/2023-January/144533.html If someone out there wants this to be included in glibc 2.37 (which will be released in February), feel free to review the patch on the libc-alpha list. |
comment:121
Thanks Paul. I tried the v3 version of the patch that was posted yesterday (https://sourceware.org/pipermail/libc-alpha/2023-January/144535.html). It does fix the slowdown:
|
So do you think we should do something like that at the end of |
for the record at today's glibc weekly review meeting I asked the v3 patch (https://patchwork.sourceware.org/project/glibc/patch/20230106185250.2936935-1-szabolcs.nagy@arm.com/) |
after several pings from myself, this issue should now be fixed in glibc development branch, thus should be in 2.39 |
@zimmermann6 @videlec
For comparision (the first one is just the same as the system glibc):
As for the original report:
|
thank you for checking, we can now close this ticket I guess. |
gives
See also
CC: @edgarcosta @mkoeppe @JohnCremona @fredrik-johansson @zimmermann6
Component: performance
Issue created by migration from https://trac.sagemath.org/ticket/34850
The text was updated successfully, but these errors were encountered: