Opened 6 years ago

Closed 6 years ago

Last modified 4 years ago

#16148 closed defect (fixed)

Really enable cython caching

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-6.2
Component: build Keywords:
Cc: robertwb, vbraun Merged in:
Authors: Jeroen Demeyer Reviewers: Volker Braun
Report Upstream: N/A Work issues:
Branch: dfc4bf9 (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

In order to speed up Cythonization, we should use a cache. Ticket #15430 tried to make this the default, but due to some bug, it never actually worked.

Besides this, this ticket also fixes the silly precision in timing output (time = 2.35585808754 seconds) and disables stdout buffering such that Cython's messages actually appear in the right order.

Change History (19)

comment:1 Changed 6 years ago by jdemeyer

  • Branch set to u/jdemeyer/ticket/16148
  • Created changed from 04/12/14 11:19:28 to 04/12/14 11:19:28
  • Modified changed from 04/12/14 11:19:28 to 04/12/14 11:19:28

comment:2 Changed 6 years ago by jdemeyer

  • Commit set to dfc4bf95f2aa6ee5e69d6754a594d3332a11e35f
  • Description modified (diff)
  • Status changed from new to needs_review

New commits:

dfc4bf9Enable Cython caching by default

comment:3 follow-up: Changed 6 years ago by vbraun

The cycache fix looks good to me.

I don't understand your comment about order, if cython messages are printed to stdout then flushing stdout does not change their order. If they are printed to stderr then unbuffered stdout is wrong, it should be line buffered to avoid half-written lines.

comment:4 Changed 6 years ago by vbraun

  • Reviewers set to Volker Braun

comment:5 in reply to: ↑ 3 Changed 6 years ago by jdemeyer

Replying to vbraun:

I don't understand your comment about order, if cython messages are printed to stdout then flushing stdout does not change their order.

Cython mixes print statements with shell commands. Usually, if you do

print "Python message"
os.system("echo hello")

then the "hello" will usually be output before the "Python message" if output is not to a terminal (output to a terminal is unbuffered by default).

comment:6 follow-up: Changed 6 years ago by vbraun

Ok, I see we are using os.system to launch. Still, we should be using line buffering and not unbuffered mode to avoid truncated lines on the Python stream level. Though the lower-level libc buffering is line buffered by default and that probably saves us.

comment:7 in reply to: ↑ 6 ; follow-up: Changed 6 years ago by jdemeyer

Replying to vbraun:

Ok, I see we are using os.system to launch.

Where "we" is also Cython, not just Sage.

Still, we should be using line buffering and not unbuffered mode to avoid truncated lines on the Python stream level.

How could unbuffered mode lead to truncated lines?

Though the lower-level libc buffering is line buffered by default and that probably saves us.

I think it's the same level, Python doesn't do it own buffering.

comment:8 in reply to: ↑ 7 ; follow-ups: Changed 6 years ago by vbraun

Replying to jdemeyer:

How could unbuffered mode lead to truncated lines?

Not truncated, but different processe's output would not necessarily be separated by newlines 1111222222111111\n222\n vs. 111111111\n2222222\n

Python doesn't do it own buffering.

To be precise, it did until your patch turned it off. But os.fdopen(,,0) does not call libc setvbuf.

Last edited 6 years ago by jdemeyer (previous) (diff)

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

Replying to vbraun:

But os.fdopen(,,0) does not call libc setvbuf.

I admit I have not read Python's sources, but why do you think so?

comment:10 in reply to: ↑ 8 Changed 6 years ago by jdemeyer

Replying to vbraun:

Replying to jdemeyer:

How could unbuffered mode lead to truncated lines?

Not truncated, but different processe's output would not necessarily be separated by newlines 1111222222111111\n222\n vs. 111111111\n2222222\n

That can happen with or without buffering. This is an OS-level issue, which libc's buffering doesn't solve.

comment:11 follow-up: Changed 6 years ago by vbraun

Ok, os.fdopen does call setvbuf. Cool.

comment:12 in reply to: ↑ 11 Changed 6 years ago by jdemeyer

Replying to vbraun:

Ok, os.fdopen does call setvbuf. Cool.

Side comment: this is something which changed in Python 3, where the libc FILE I/O is no longer used.

comment:13 Changed 6 years ago by jdemeyer

  • Description modified (diff)

comment:14 Changed 6 years ago by jdemeyer

  • Description modified (diff)

comment:15 Changed 6 years ago by vbraun

  • Branch changed from u/jdemeyer/ticket/16148 to dfc4bf95f2aa6ee5e69d6754a594d3332a11e35f
  • Resolution set to fixed
  • Status changed from needs_review to closed

comment:16 in reply to: ↑ description Changed 5 years ago by leif

  • Commit dfc4bf95f2aa6ee5e69d6754a594d3332a11e35f deleted

Replying to jdemeyer:

this ticket also fixes the silly precision in timing output (time = 2.35585808754 seconds)

\o/ Thank you very much, it has been annoying me for years.

comment:17 Changed 5 years ago by leif

Cython.Compiler.Main.default_options['cache'] = True

is probably the wrong thing; it should be passed in options to cythonize().

But essentially cythonize() appears to be broken w.r.t. this, since it does:

    ...
    c_options = CompilationOptions(**options)
    ...
    options = c_options    
    ...
    if hasattr(options, 'cache'):
        if not os.path.exists(options.cache):
            os.makedirs(options.cache)

and one may end up with

File "/scratch/sage/local/lib/python2.7/site-packages/Cython/Build/Dependencies.py", line 739, in cythonize
if not os.path.exists(options.cache):
File "/scratch/sage/local/lib/python/genericpath.py", line 18, in exists
os.stat(path)
TypeError: coercing to Unicode: need string or buffer, bool found

as reported on sage-devel.

comment:18 Changed 4 years ago by saraedum

Is it possible that cython caching is broken again? At least it does not work for me anymore (i.e., when switching back and forth between branches I do not see a speedup.) Do I have to do anything (create cache directories?) to make it work or should it work out of the box?

comment:19 Changed 4 years ago by vbraun

See #17851

Note: See TracTickets for help on using tickets.