Opened 11 years ago

Closed 10 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:

Status badges

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 11 years ago.
7879-RR-signal.2.patch (22.2 KB) - added by robertwb 10 years ago.
Rebased on 4.7
trac_7879-RR-signal.2.rebased_to_4.7.2.alpha2.patch (22.2 KB) - added by leif 10 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 10 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 11 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 11 years ago by robertwb

  • Description modified (diff)

Changed 11 years ago by robertwb

comment:3 Changed 11 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 10 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 10 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 10 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 10 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 10 years ago by mariah

  • Status changed from positive_review to needs_work

comment:9 in reply to: ↑ 7 Changed 10 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 10 years ago by jdemeyer

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

comment:11 Changed 10 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 10 years ago by robertwb

Rebased on 4.7

comment:12 follow-up: Changed 10 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 10 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 10 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 10 years ago by was

  • Keywords sd32 added

comment:16 Changed 10 years ago by leif

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

Changed 10 years ago by leif

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

Changed 10 years ago by leif

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

comment:17 Changed 10 years ago by leif

  • Description modified (diff)

Attached a rebased patch because of fuzz.

comment:18 Changed 10 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.