Opened 13 years ago
Last modified 11 years ago
#7879 closed enhancement
Remove unnecessary signal handling for low prec mpfr operations. — at Version 16
Reported by: | Robert Bradshaw | Owned by: | Alex Ghitza |
---|---|---|---|
Priority: | minor | Milestone: | sage-4.7.2 |
Component: | basic arithmetic | Keywords: | sd32 |
Cc: | Merged in: | ||
Authors: | Robert Bradshaw | Reviewers: | Alex Ghitza, Mariah Lenox, William Stein |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
_sig_on
and _sig_off
are more expensive than computing the result itself.
Apply only 7879-RR-signal.2.patch to the Sage library.
Change History (20)
comment:1 Changed 13 years ago by
Status: | new → needs_review |
---|
comment:2 Changed 13 years ago by
Description: | modified (diff) |
---|
Changed 13 years ago by
Attachment: | 7879-RR-signal.patch added |
---|
comment:3 Changed 13 years ago by
Authors: | → Robert Bradshaw |
---|---|
Reviewers: | → Alex Ghitza |
Status: | needs_review → needs_info |
This looks good. I have one question: you are hardcoding threshold precisions after which the signals should be used, mostly 1000, but also 10000 in a couple of places. Would it be better to have a global variable PREC_THRESHOLD set to 1000 at the top of the file, and then use it or 10*PREC_THRESHOLD where needed? It would make it easier to change this in the future, since a search for it would be all that's needed.
I'm marking this as needs_info. If you think it's not worth the trouble, I'll put a positive review.
comment:4 Changed 12 years ago by
Milestone: | sage-4.6.2 → sage-duplicate/invalid/wontfix |
---|---|
Status: | needs_info → needs_review |
I am proposing to close this ticket as invalid since #9678 makes sig_on()
and sig_off()
a lot faster. In any case, if you want to keep the patch, it needs rebasing (_sig_on
and _sig_off
must be replaced by sig_on()
and sig_off()
but there are more patch conflicts).
comment:5 Changed 12 years ago by
Status: | needs_review → positive_review |
---|
Since this ticket is marked sage-duplicate/invalid/wontfix plus has not been active for 4 months, I believe this ticket can be closed.
comment:6 Changed 12 years ago by
Robert, what is your opinion? sig_on()
and sig_off()
became a lot faster since #9678, but they still take up some cycles.
comment:7 follow-up: 9 Changed 12 years ago by
Milestone: | sage-duplicate/invalid/wontfix → sage-4.7.1 |
---|
Thanks for taking a look at this. I've been waiting for 4.7 to be released to re-base this, and think it'd still worth having. Perhaps I'll put PREC_THRESHOLD in there while I'm at it, and maybe do some benchmarking.
comment:8 Changed 12 years ago by
Status: | positive_review → needs_work |
---|
comment:9 Changed 12 years ago by
Replying to robertwb:
and maybe do some benchmarking.
If you find useful ways to do benchmarks, please let me know. I might re-use those benchmarks to optimize sig_on()
and sig_off()
.
comment:10 Changed 12 years ago by
The patch adds one unrelated doctest. This doctest has been moved to #11344.
comment:11 Changed 12 years ago by
Status: | needs_work → needs_review |
---|
---------------------------------------------------------------------- | Sage Version 4.7, Release Date: 2011-05-23 | | Type notebook() for the GUI, and license() for information. | ---------------------------------------------------------------------- Loading Sage library. Current Mercurial branch is: bitrot sage: L = [RR(k) for k in range(1000)] sage: %timeit [a.sqrt() for a in L] 625 loops, best of 3: 569 µs per loop sage: %timeit [a.sqrt() for a in L] 625 loops, best of 3: 509 µs per loop sage: %timeit [a.sqrt() for a in L] 625 loops, best of 3: 513 µs per loop sage: timeit("[a.sqrt() for a in L]", number=10000) 10000 loops, best of 3: 519 µs per loop sage: Exiting Sage (CPU time 0m19.94s, Wall time 1m9.75s). robertwb@sage:/scratch/robertwb/sage-4.7/devel/sage-bitrot$ hg qpush applying 7879-RR-signal.patch now at: 7879-RR-signal.patch robertwb@sage:/scratch/robertwb/sage-4.7/devel/sage-bitrot$ ../../sage -br [...] ---------------------------------------------------------------------- | Sage Version 4.7, Release Date: 2011-05-23 | | Type notebook() for the GUI, and license() for information. | ---------------------------------------------------------------------- Loading Sage library. Current Mercurial branch is: bitrot sage: L = [RR(k) for k in range(1000)] sage: %timeit [a.sqrt() for a in L] 625 loops, best of 3: 498 µs per loop sage: %timeit [a.sqrt() for a in L] 625 loops, best of 3: 498 µs per loop sage: %timeit [a.sqrt() for a in L] 625 loops, best of 3: 496 µs per loop sage: timeit("[a.sqrt() for a in L]", number=10000) 10000 loops, best of 3: 499 µs per loop
Looks like about a 5% win (much smaller than I remember it being before).
comment:12 follow-up: 13 Changed 12 years ago by
Status: | needs_review → needs_work |
---|
patch 7879-RR-signal.2.patch applied to sage-4.7.1.alpha1. Then did 'sage -b' followed by 'make testlong'. The following test failed:
eno% ./sage -t -long -force_lib "devel/sage/sage/rings/residue_field.pyx" sage -t -long -force_lib "devel/sage/sage/rings/residue_field.pyx" ********************************************************************** File "/home/mariah/sage/sage-4.7.1.alpha1-x86_64-Linux-core2-fc-review-7879/devel/sage/sage/rings/residue_field.pyx", line 670: sage: I = QQ[3^(1/3)].factor(5)[1][0]; I Exception raised: Traceback (most recent call last): File "/home/mariah/sage/sage-4.7.1.alpha1-x86_64-Linux-core2-fc-review-7879/local/bin/ncadoctest.py", line 1231, in run_one_test self.run_one_example(test, example, filename, compileflags) File "/home/mariah/sage/sage-4.7.1.alpha1-x86_64-Linux-core2-fc-review-7879/local/bin/sagedoctest.py", line 38, in run_one_example OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags) File "/home/mariah/sage/sage-4.7.1.alpha1-x86_64-Linux-core2-fc-review-7879/local/bin/ncadoctest.py", line 1172, in run_one_example compileflags, 1) in test.globs File "<doctest __main__.example_10[2]>", line 1, in <module> I = QQ[Integer(3)**(Integer(1)/Integer(3))].factor(Integer(5))[Integer(1)][Integer(0)]; I###line 670: sage: I = QQ[3^(1/3)].factor(5)[1][0]; I File "ring.pyx", line 167, in sage.rings.ring.Ring.__getitem__ (sage/rings/ring.c:2007) File "expression.pyx", line 8287, in sage.symbolic.expression.Expression.minpoly (sage/symbolic/expression.cpp:31950) File "/home/mariah/sage/sage-4.7.1.alpha1-x86_64-Linux-core2-fc-review-7879/local/lib/python/site-packages/sage/calculus/calculus.py", line 925, in minpoly if g(ex).simplify_trig().simplify_radical() == 0: File "expression.pyx", line 6546, in sage.symbolic.expression.Expression.simplify_trig (sage/symbolic/expression.cpp:24657) File "expression.pyx", line 460, in sage.symbolic.expression.Expression._maxima_ (sage/symbolic/expression.cpp:3674) File "sage_object.pyx", line 429, in sage.structure.sage_object.SageObject._interface_ (sage/structure/sage_object.c:3451) File "lazy_import.pyx", line 167, in sage.misc.lazy_import.LazyImport.__getattr__ (sage/misc/lazy_import.c:1353) File "lazy_import.pyx", line 110, in sage.misc.lazy_import.LazyImport._get_object (sage/misc/lazy_import.c:1023) File "/home/mariah/sage/sage-4.7.1.alpha1-x86_64-Linux-core2-fc-review-7879/local/lib/python/site-packages/sage/interfaces/maxima_lib.py", line 152, in <module> ecl_eval("#$%s$"%l) File "ecl.pyx", line 1224, in sage.libs.ecl.ecl_eval (sage/libs/ecl.c:6301) File "ecl.pyx", line 1239, in sage.libs.ecl.ecl_eval (sage/libs/ecl.c:6252) File "ecl.pyx", line 252, in sage.libs.ecl.ecl_safe_eval (sage/libs/ecl.c:2544) RuntimeError: ECL says: THROW: The catch MACSYMA-QUIT is undefined. ********************************************************************** File "/home/mariah/sage/sage-4.7.1.alpha1-x86_64-Linux-core2-fc-review-7879/devel/sage/sage/rings/residue_field.pyx", line 672: sage: k = I.residue_field(); k Exception raised: Traceback (most recent call last): File "/home/mariah/sage/sage-4.7.1.alpha1-x86_64-Linux-core2-fc-review-7879/local/bin/ncadoctest.py", line 1231, in run_one_test self.run_one_example(test, example, filename, compileflags) File "/home/mariah/sage/sage-4.7.1.alpha1-x86_64-Linux-core2-fc-review-7879/local/bin/sagedoctest.py", line 38, in run_one_example OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags) File "/home/mariah/sage/sage-4.7.1.alpha1-x86_64-Linux-core2-fc-review-7879/local/bin/ncadoctest.py", line 1172, in run_one_example compileflags, 1) in test.globs File "<doctest __main__.example_10[3]>", line 1, in <module> k = I.residue_field(); k###line 672: sage: k = I.residue_field(); k File "element.pyx", line 328, in sage.structure.element.Element.__getattr__ (sage/structure/element.c:2792) File "parent.pyx", line 277, in sage.structure.parent.getattr_from_other_class (sage/structure/parent.c:2940) File "parent.pyx", line 175, in sage.structure.parent.raise_attribute_error (sage/structure/parent.c:2709) AttributeError: 'sage.symbolic.expression.Expression' object has no attribute 'residue_field' **********************************************************************
etc.
comment:13 Changed 11 years ago by
Reviewers: | Alex Ghitza → Alex Ghitza, William Stein |
---|
Replying to mariah:
patch 7879-RR-signal.2.patch applied to sage-4.7.1.alpha1. Then did 'sage -b' followed by 'make testlong'. The following test failed:
eno% ./sage -t -long -force_lib "devel/sage/sage/rings/residue_field.pyx" sage -t -long -force_lib "devel/sage/sage/rings/residue_field.pyx" ********************************************************************** File "/home/mariah/sage/sage-4.7.1.alpha1-x86_64-Linux-core2-fc-review-7879/devel/sage/sage/rings/residue_field.pyx", line 670: sage: I = QQ[3^(1/3)].factor(5)[1][0]; I Exception raised:
These errors don't occur for me with sage-4.7.2.alpha2.
comment:14 Changed 11 years ago by
Status: | needs_work → positive_review |
---|
I did a full test of Sage on another machine too with 4.7.2.alpha2, and everything works. Code looks good and addresses other people's issues...
comment:15 Changed 11 years ago by
Keywords: | sd32 added |
---|
comment:16 Changed 11 years ago by
Description: | modified (diff) |
---|---|
Reviewers: | Alex Ghitza, William Stein → Alex Ghitza, Mariah Lenox, William Stein |
Changed 11 years ago by
Attachment: | trac_7879-RR-signal.2.rebased_to_4.7.2.alpha2.patch added |
---|
Sage library patch. Robert's patch rebased to Sage 4.7.2.alpha2, because of fuzz 2.
Changed 11 years ago by
Attachment: | trac_7879-harmless_fuzz_2.diff added |
---|
Do not apply. Fuzz against Sage 4.7.2.alpha2. For reference only.
Signal handling is omitted for functions/precisions that were clearly and uniformly in the millisecond range or less.