Opened 13 years ago

Last modified 11 years ago

#7879 closed enhancement

Remove unnecessary signal handling for low prec mpfr operations. — at Version 17

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:

Status badges

Description (last modified by Leif Leonhardy)

_sig_on and _sig_off are more expensive than computing the result itself.


Apply only trac_7879-RR-signal.2.rebased_to_4.7.2.alpha2.patch to the Sage library.

Change History (21)

comment:1 Changed 13 years ago by Robert Bradshaw

Status: newneeds_review

Signal handling is omitted for functions/precisions that were clearly and uniformly in the millisecond range or less.

comment:2 Changed 13 years ago by Robert Bradshaw

Description: modified (diff)

Changed 13 years ago by Robert Bradshaw

Attachment: 7879-RR-signal.patch added

comment:3 Changed 13 years ago by Alex Ghitza

Authors: Robert Bradshaw
Reviewers: Alex Ghitza
Status: needs_reviewneeds_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 Jeroen Demeyer

Milestone: sage-4.6.2sage-duplicate/invalid/wontfix
Status: needs_infoneeds_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 Mariah Lennox

Status: needs_reviewpositive_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 Jeroen Demeyer

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 Changed 12 years ago by Robert Bradshaw

Milestone: sage-duplicate/invalid/wontfixsage-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 Mariah Lennox

Status: positive_reviewneeds_work

comment:9 in reply to:  7 Changed 12 years ago by Jeroen Demeyer

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 Jeroen Demeyer

The patch adds one unrelated doctest. This doctest has been moved to #11344.

comment:11 Changed 12 years ago by Robert Bradshaw

Status: needs_workneeds_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).

Changed 12 years ago by Robert Bradshaw

Attachment: 7879-RR-signal.2.patch added

Rebased on 4.7

comment:12 Changed 12 years ago by Mariah Lennox

Status: needs_reviewneeds_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 in reply to:  12 Changed 11 years ago by William Stein

Reviewers: Alex GhitzaAlex 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 William Stein

Status: needs_workpositive_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 William Stein

Keywords: sd32 added

comment:16 Changed 11 years ago by Leif Leonhardy

Description: modified (diff)
Reviewers: Alex Ghitza, William SteinAlex Ghitza, Mariah Lenox, William Stein

Changed 11 years ago by Leif Leonhardy

Sage library patch. Robert's patch rebased to Sage 4.7.2.alpha2, because of fuzz 2.

Changed 11 years ago by Leif Leonhardy

Do not apply. Fuzz against Sage 4.7.2.alpha2. For reference only.

comment:17 Changed 11 years ago by Leif Leonhardy

Description: modified (diff)

Attached a rebased patch because of fuzz.

Note: See TracTickets for help on using tickets.