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

defer primality and irreducibility testing in GF constructor until after caching #34281

Closed
yyyyx4 opened this issue Aug 5, 2022 · 14 comments
Closed

Comments

@yyyyx4
Copy link
Member

yyyyx4 commented Aug 5, 2022

Example:

sage: p = 2^521-1
sage: F = GF(p)
sage: GF(p) is F  # field is cached
True
sage: %timeit GF(p)
521 ms ± 6.46 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

Note that the constructor tests primality each time even though the field is already cached! This was pointed out here:

https://github.com/jack4818/Castryck-Decru-SageMath#speeding-sagemath-up-using-a-cache

In this patch, we move the primality and irreducibility testing from FiniteFieldFactory.create_key_and_extra_args() to FiniteFieldFactory.create_object(), so that it isn't performed again for fields already present in the cache.

The result is a massive speedup for repeated invocations of GF(p):

78.6 µs ± 870 ns per loop (mean ± std. dev. of 7 runs, 10000 loops each)

CC: @k3w1k0d3r

Component: performance

Author: Lorenz Panny

Branch/Commit: 0b9db49

Reviewer: Julien Grijalva

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

@yyyyx4 yyyyx4 added this to the sage-9.7 milestone Aug 5, 2022
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 5, 2022

Changed commit from 94af055 to b5f2eac

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 5, 2022

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

b5f2eacupdate doctest outputs

@k3w1k0d3r
Copy link

Reviewer: Julien Grijalva

@k3w1k0d3r
Copy link

comment:6

patchbot failed a test, segfaulting when it reached the p, n = order.perfect_power() you added. I'm currently checking if I reproduce this locally.

@yyyyx4
Copy link
Member Author

yyyyx4 commented Aug 6, 2022

comment:7

I suspect this is a consequence of the other doctest failures here (which I also see on the unpatched develop branch). I can't reproduce it in isolation.

@k3w1k0d3r
Copy link

comment:8

I am able to reproduce this behavior locally. It seems like a bug in the perfect_power method though. I'll investigate.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 8, 2022

Changed commit from b5f2eac to c6772dd

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 8, 2022

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

c6772ddremove incorrect(?) sig_on/sig_off

@yyyyx4
Copy link
Member Author

yyyyx4 commented Aug 8, 2022

comment:10

Setting to "needs review" to get the patchbot to run, but I suspect there are still problems.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 9, 2022

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

0b9db49avoid failing code path by passing tuple to GF constructor

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 9, 2022

Changed commit from c6772dd to 0b9db49

@yyyyx4
Copy link
Member Author

yyyyx4 commented Aug 9, 2022

comment:12

With the most recent commit, I no longer see any failures on my machine. Still not sure what causes these crashes and why things only go wrong in pbori.pyx, but the workaround seems to, well, work around it.

@yyyyx4
Copy link
Member Author

yyyyx4 commented Aug 9, 2022

comment:13

Patchbot is morally green; I've seen the remaining failure in other (totally unrelated) tickets before.

@vbraun
Copy link
Member

vbraun commented Aug 30, 2022

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

3 participants