Opened 5 years ago

Closed 5 years ago

#16814 closed defect (fixed)

Add SAGE_PROFILE to enable profiler

Reported by: vbraun Owned by:
Priority: major Milestone: sage-6.4
Component: build Keywords:
Cc: mraum, jdemeyer, novoselt Merged in:
Authors: Volker Braun Reviewers: Martin Raum
Report Upstream: N/A Work issues:
Branch: 832f3ad (Commits) Commit: 832f3ada63dbf94c002ddc28b0244b9ee28ade31
Dependencies: Stopgaps:

Description (last modified by vbraun)

The new SAGE_PROFILE environment variable controls whether to build with profiling support. It defaults to off since this potentially is a big performance impact. Right now it only controls Cython profiling, but third-party packages should support it in the future if they can.

With SAGE_PROFILE=no:

 sage: %prun 1 + 1
         2 function calls in 0.000 seconds

   Ordered by: internal time

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
        1    0.000    0.000    0.000    0.000 <string>:1(<module>)
        1    0.000    0.000    0.000    0.000 {method 'disable' of '_lsprof.Profiler' objects}

With SAGE_PROFILE=yes:

 sage: %prun 1 + 1
         13 function calls in 0.000 seconds

   Ordered by: internal time

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
        1    0.000    0.000    0.000    0.000 <string>:1(<module>)
        1    0.000    0.000    0.000    0.000 element.pyx:1538(__add__)
        1    0.000    0.000    0.000    0.000 integer.pyx:1605(_add_)
        3    0.000    0.000    0.000    0.000 integer.pyx:6276(fast_tp_new)
        2    0.000    0.000    0.000    0.000 integer.pyx:512(__init__)
        3    0.000    0.000    0.000    0.000 integer.pyx:6362(fast_tp_dealloc)
        1    0.000    0.000    0.000    0.000 coerce.pxi:7(have_same_parent)
        1    0.000    0.000    0.000    0.000 {method 'disable' of '_lsprof.Profiler' objects}

Change History (16)

comment:1 Changed 5 years ago by vbraun

  • Description modified (diff)

comment:2 Changed 5 years ago by vbraun

  • Branch set to u/vbraun/cython_profile

comment:3 Changed 5 years ago by vbraun

  • Cc mraum jdemeyer added
  • Commit set to 832f3ada63dbf94c002ddc28b0244b9ee28ade31

New commits:

832f3adAdd SAGE_PROFILE environment variable

comment:4 Changed 5 years ago by vbraun

  • Status changed from new to needs_review

comment:5 Changed 5 years ago by vbraun

  • Description modified (diff)

comment:6 Changed 5 years ago by novoselt

  • Cc novoselt added

comment:7 Changed 5 years ago by mraum

This, it seems, gives only the filename, e.g. integer.pyx. Is it possible to modify the profiling so that we get the full path?

comment:8 Changed 5 years ago by vbraun

Its certainly possible, but I have no intention of modifying the way cProfile works.

comment:10 Changed 5 years ago by mraum

I investigated on this. It's not an issue of cProfile. Rather Cython must be changed. Line 688 of Cython/Compiler/ModuleNode?.py one has to put os.path.abspath. Such a change would be really useful for citation management. How big, do you think, are chances that we have a downstream patch or upstream accepts such a change?

comment:11 Changed 5 years ago by mmezzarobba

How about including a note on how to profile a single Cython module (perhaps a link to http://docs.cython.org/src/tutorial/profiling_tutorial.html)?

comment:12 Changed 5 years ago by mraum

That's a nice suggestion, and we should indeed include such a note.

As for citation management (see #16777), the burden to the user to obtain correct citations for the code that he or she uses should not be excessively high. That's why I'm looking for a nice way to get the full module path.

comment:13 Changed 5 years ago by vbraun

The link to the cython tutorial has no place in the installation instructions. But you are welcome to write more on Cython debugging and profiling in the developer guide. On another ticket, ideally.

comment:14 Changed 5 years ago by mraum

  • Reviewers set to Martin Raum
  • Status changed from needs_review to positive_review

You're absoultely right. The purpose of this ticket is to add an environment variable. And this change looks good.

comment:15 Changed 5 years ago by vbraun

For citation management (re)compiling Sage is probably too much of a burden. This will exclude everybody with a system-wide Sage installation, for starters.

Have you thought about inspecting memory? Any third-party interface will create objects, so thats an easy place to look at what changed. You could temporarily disable gc if you worry about them being collected too early.

comment:16 Changed 5 years ago by vbraun

  • Branch changed from u/vbraun/cython_profile to 832f3ada63dbf94c002ddc28b0244b9ee28ade31
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.