Opened 9 years ago

Closed 8 years ago

#7879 closed enhancement (fixed)

Remove unnecessary signal handling for low prec mpfr operations.

Reported by: robertwb Owned by: AlexGhitza
Priority: minor Milestone: sage-4.7.2
Component: basic arithmetic Keywords: sd32
Cc: Merged in: sage-4.7.2.alpha3
Authors: Robert Bradshaw Reviewers: Alex Ghitza, Mariah Lenox, William Stein
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by leif)

_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.

Attachments (4)

7879-RR-signal.patch (24.8 KB) - added by robertwb 9 years ago.
7879-RR-signal.2.patch (22.2 KB) - added by robertwb 8 years ago.
Rebased on 4.7
trac_7879-RR-signal.2.rebased_to_4.7.2.alpha2.patch (22.2 KB) - added by leif 8 years ago.
Sage library patch. Robert's patch rebased to Sage 4.7.2.alpha2, because of fuzz 2.
trac_7879-harmless_fuzz_2.diff (16.1 KB) - added by leif 8 years ago.
Do not apply. Fuzz against Sage 4.7.2.alpha2. For reference only.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 9 years ago by robertwb

  • Status changed from new to needs_review

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

comment:2 Changed 9 years ago by robertwb

  • Description modified (diff)

Changed 9 years ago by robertwb

comment:3 Changed 9 years ago by AlexGhitza

  • Authors set to Robert Bradshaw
  • Reviewers set to Alex Ghitza
  • Status changed from needs_review to 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 8 years ago by jdemeyer

  • Milestone changed from sage-4.6.2 to sage-duplicate/invalid/wontfix
  • Status changed from needs_info to 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 8 years ago by mariah

  • Status changed from needs_review to 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 8 years ago by jdemeyer

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: Changed 8 years ago by robertwb

  • Milestone changed from sage-duplicate/invalid/wontfix to 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 8 years ago by mariah

  • Status changed from positive_review to needs_work

comment:9 in reply to: ↑ 7 Changed 8 years ago by jdemeyer

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 8 years ago by jdemeyer

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

comment:11 Changed 8 years ago by robertwb

  • Status changed from needs_work to 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).

Changed 8 years ago by robertwb

Rebased on 4.7

comment:12 follow-up: Changed 8 years ago by mariah

  • Status changed from needs_review to 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 in reply to: ↑ 12 Changed 8 years ago by was

  • Reviewers changed from Alex Ghitza to 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 8 years ago by was

  • Status changed from needs_work to 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 8 years ago by was

  • Keywords sd32 added

comment:16 Changed 8 years ago by leif

  • Description modified (diff)
  • Reviewers changed from Alex Ghitza, William Stein to Alex Ghitza, Mariah Lenox, William Stein

Changed 8 years ago by leif

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

Changed 8 years ago by leif

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

comment:17 Changed 8 years ago by leif

  • Description modified (diff)

Attached a rebased patch because of fuzz.

comment:18 Changed 8 years ago by leif

  • Merged in set to sage-4.7.2.alpha3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.