16

I would like to improve the performance of a Python script and have been using cProfile to generate a performance report:

python -m cProfile -o chrX.prof ./bgchr.py ...args...

I opened this chrX.prof file with Python's pstats and printed out the statistics:

Python 2.7 (r27:82500, Oct  5 2010, 00:24:22) 
[GCC 4.1.2 20080704 (Red Hat 4.1.2-44)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import pstats
>>> p = pstats.Stats('chrX.prof')
>>> p.sort_stats('name')
>>> p.print_stats()                                                                                                                                                                                                                        
Sun Oct 10 00:37:30 2010    chrX.prof                                                                                                                                                                                                      

         8760583 function calls in 13.780 CPU seconds                                                                                                                                                                                      

   Ordered by: function name                                                                                                                                                                                                               

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)                                                                                                                                                                    
        1    0.000    0.000    0.000    0.000 {_locale.setlocale}                                                                                                                                                                          
        1    1.128    1.128    1.128    1.128 {bz2.decompress}                                                                                                                                                                             
        1    0.002    0.002   13.780   13.780 {execfile}                                                                                                                                                                                   
  1750678    0.300    0.000    0.300    0.000 {len}                                                                                                                                                                                        
       48    0.000    0.000    0.000    0.000 {method 'append' of 'list' objects}                                                                                                                                                          
        1    0.000    0.000    0.000    0.000 {method 'close' of 'file' objects}                                                                                                                                                           
        1    0.000    0.000    0.000    0.000 {method 'disable' of '_lsprof.Profiler' objects}                                                                                                                                             
  1750676    0.496    0.000    0.496    0.000 {method 'join' of 'str' objects}                                                                                                                                                             
        1    0.007    0.007    0.007    0.007 {method 'read' of 'file' objects}                                                                                                                                                            
        1    0.000    0.000    0.000    0.000 {method 'readlines' of 'file' objects}                                                                                                                                                       
        1    0.034    0.034    0.034    0.034 {method 'rstrip' of 'str' objects}                                                                                                                                                           
       23    0.000    0.000    0.000    0.000 {method 'seek' of 'file' objects}                                                                                                                                                            
  1757785    1.230    0.000    1.230    0.000 {method 'split' of 'str' objects}                                                                                                                                                            
        1    0.000    0.000    0.000    0.000 {method 'startswith' of 'str' objects}                                                                                                                                                       
  1750676    0.872    0.000    0.872    0.000 {method 'write' of 'file' objects}                                                                                                                                                           
        1    0.007    0.007   13.778   13.778 ./bgchr:3(<module>)                                                                                                                                                                          
        1    0.000    0.000   13.780   13.780 <string>:1(<module>)                                                                                                                                                                         
        1    0.001    0.001    0.001    0.001 {open}                                                                                                                                                                                       
        1    0.000    0.000    0.000    0.000 {sys.exit}                                                                                                                                                                                   
        1    0.000    0.000    0.000    0.000 ./bgchr:36(checkCommandLineInputs)                                                                                                                                                           
        1    0.000    0.000    0.000    0.000 ./bgchr:27(checkInstallation)                                                                                                                                                                
        1    1.131    1.131   13.701   13.701 ./bgchr:97(extractData)                                                                                                                                                                      
        1    0.003    0.003    0.007    0.007 ./bgchr:55(extractMetadata)                                                                                                                                                                  
        1    0.064    0.064   13.771   13.771 ./bgchr:5(main)                                                                                                                                                                              
  1750677    8.504    0.000   11.196    0.000 ./bgchr:122(parseJarchLine)                                                                                                                                                                  
        1    0.000    0.000    0.000    0.000 ./bgchr:72(parseMetadata)                                                                                                                                                                    
        1    0.000    0.000    0.000    0.000 /home/areynolds/proj/tools/lib/python2.7/locale.py:517(setlocale) 

Question: What can I do about join, split and write operations to reduce the apparent impact they have on the performance of this script?

If it is relevant, here is the full source code to the script in question:

#!/usr/bin/env python

import sys, os, time, bz2, locale

def main(*args):
    # Constants
    global metadataRequiredFileSize
    metadataRequiredFileSize = 8192
    requiredVersion = (2,5)

    # Prep
    global whichChromosome
    whichChromosome = "all"
    checkInstallation(requiredVersion)
    checkCommandLineInputs()
    extractMetadata()
    parseMetadata()
    if whichChromosome == "--list":
        listMetadata()
        sys.exit(0)

    # Extract
    extractData()   
    return 0

def checkInstallation(rv):
    currentVersion = sys.version_info
    if currentVersion[0] == rv[0] and currentVersion[1] >= rv[1]:
        pass
    else:
        sys.stderr.write( "\n\t[%s] - Error: Your Python interpreter must be %d.%d or greater (within major version %d)\n" % (sys.argv[0], rv[0], rv[1], rv[0]) )
        sys.exit(-1)
    return

def checkCommandLineInputs():
    cmdName = sys.argv[0]
    argvLength = len(sys.argv[1:])
    if (argvLength == 0) or (argvLength > 2):
        sys.stderr.write( "\n\t[%s] - Usage: %s [<chromosome> | --list] <bjarch-file>\n\n" % (cmdName, cmdName) )
        sys.exit(-1)
    else:   
        global inFile
        global whichChromosome
        if argvLength == 1:
            inFile = sys.argv[1]
        elif argvLength == 2:
            whichChromosome = sys.argv[1]
            inFile = sys.argv[2]
        if inFile == "-" or inFile == "--list":
            sys.stderr.write( "\n\t[%s] - Usage: %s [<chromosome> | --list] <bjarch-file>\n\n" % (cmdName, cmdName) )
            sys.exit(-1)
    return

def extractMetadata():
    global metadataList
    global dataHandle
    metadataList = []
    dataHandle = open(inFile, 'rb')
    try:
        for data in dataHandle.readlines(metadataRequiredFileSize):     
            metadataLine = data
            metadataLines = metadataLine.split('\n')
            for line in metadataLines:      
                if line:
                    metadataList.append(line)
    except IOError:
        sys.stderr.write( "\n\t[%s] - Error: Could not extract metadata from %s\n\n" % (sys.argv[0], inFile) )
        sys.exit(-1)
    return

def parseMetadata():
    global metadataList
    global metadata
    metadata = []
    if not metadataList: # equivalent to "if len(metadataList) > 0"
        sys.stderr.write( "\n\t[%s] - Error: No metadata in %s\n\n" % (sys.argv[0], inFile) )
        sys.exit(-1)
    for entryText in metadataList:
        if entryText: # equivalent to "if len(entryText) > 0"
            entry = entryText.split('\t')
            filename = entry[0]
            chromosome = entry[0].split('.')[0]
            size = entry[1]
            entryDict = { 'chromosome':chromosome, 'filename':filename, 'size':size }
            metadata.append(entryDict)
    return

def listMetadata():
    for index in metadata:
        chromosome = index['chromosome']
        filename = index['filename']
        size = long(index['size'])
        sys.stdout.write( "%s\t%s\t%ld" % (chromosome, filename, size) )
    return

def extractData():
    global dataHandle
    global pLength
    global lastEnd
    locale.setlocale(locale.LC_ALL, 'POSIX')
    dataHandle.seek(metadataRequiredFileSize, 0) # move cursor past metadata
    for index in metadata:
        chromosome = index['chromosome']
        size = long(index['size'])
        pLength = 0L
        lastEnd = ""
        if whichChromosome == "all" or whichChromosome == index['chromosome']:
            dataStream = dataHandle.read(size)
            uncompressedData = bz2.decompress(dataStream)
            lines = uncompressedData.rstrip().split('\n')
            for line in lines:
                parseJarchLine(chromosome, line)
            if whichChromosome == chromosome:
                break
        else:
            dataHandle.seek(size, 1) # move cursor past chromosome chunk

    dataHandle.close()
    return

def parseJarchLine(chromosome, line):
    global pLength
    global lastEnd
    elements = line.split('\t')
    if len(elements) > 1:
        if lastEnd:
            start = long(lastEnd) + long(elements[0])
            lastEnd = long(start + pLength)
            sys.stdout.write("%s\t%ld\t%ld\t%s\n" % (chromosome, start, lastEnd, '\t'.join(elements[1:])))
        else:
            lastEnd = long(elements[0]) + long(pLength)
            sys.stdout.write("%s\t%ld\t%ld\t%s\n" % (chromosome, long(elements[0]), lastEnd, '\t'.join(elements[1:])))
    else:
        if elements[0].startswith('p'):
            pLength = long(elements[0][1:])
        else:
            start = long(long(lastEnd) + long(elements[0]))
            lastEnd = long(start + pLength)
            sys.stdout.write("%s\t%ld\t%ld\n" % (chromosome, start, lastEnd))               
    return

if __name__ == '__main__':
    sys.exit(main(*sys.argv))

EDIT

If I comment out the sys.stdout.write statement in the first conditional of parseJarchLine(), then my runtime goes from 10.2 sec to 4.8 sec:

# with first conditional's "sys.stdout.write" enabled
$ time ./bgchr chrX test.bjarch > /dev/null
real    0m10.186s                                                                                                                                                                                        
user    0m9.917s                                                                                                                                                                                         
sys 0m0.160s  

# after first conditional's "sys.stdout.write" is commented out                                                                                                                                                                                           
$ time ./bgchr chrX test.bjarch > /dev/null
real    0m4.808s                                                                                                                                                                                         
user    0m4.561s                                                                                                                                                                                         
sys 0m0.156s

Is writing to stdout really that expensive in Python?

Alex Reynolds
  • 95,983
  • 54
  • 240
  • 345
  • 4
    Break the code to small functions. Python's cProfile is pretty much useless for code that is written as one big chunk, because it's a function-level profiler, not line-by-line profiler. In the meantime, you can gain some speed increase if you put everything in a main() function, since in Python accessing global variables is slower than accessing a local variable. – Lie Ryan Oct 09 '10 at 23:17
  • @Lie Ryan: Look at the numbers! These are detailed enough to show where optimization is needed. Access to global variables is not relevant here, and the times for bgchr:4() and :1() correspond to the total execution time. – Bernd Petersohn Oct 10 '10 at 02:35
  • @Bernd Petersohn: Please consider the possibility that you are mistaken. See my answer. – John Machin Oct 10 '10 at 04:18
  • I have replaced the source code and resulting cProfile analysis results. If you have a moment to take a look and offer suggestions, I would appreciate it. Thanks. – Alex Reynolds Oct 10 '10 at 17:35
  • @Alex Reynolds: In `parseJarchLine()`, it would be sufficient to split the line at the first tab only: `elements = line.split('\t', 1)`. This would make the following join obsolete: replace the expression `'\t'.join(elements[1:]` by `elements[1]`. Also, if that second part may become huge, performance could further be improved if you do not integrate it into the format string. Instead, use separate calls to `sys.stdout.write` to output the first part, the second part and the final newline. – Bernd Petersohn Oct 11 '10 at 00:22
  • I split out the four conditionals to separate functions and profiled each of them. Most of the time is spent exclusively in the first conditional, split equally between calculating `start` and `lastEnd`, and in writing to `stdout`. I split up the `sys.stdout.write` operations as suggested, but this actually added a second of runtime. – Alex Reynolds Oct 11 '10 at 00:58
  • regarding the expense of writing to stdout: I wouldn't go by the time command's output alone -- go back and look at the profiling again. My suspicion here is that disk cache has made accessing the records in the file much faster. That said, one of the changes I would make to this code is "import logging" :) – jonesy Oct 11 '10 at 14:01
  • @Alex Reynolds: As I said below, if you care to send me the chromosome data file that your script needs, I'll use the random interrupt method on it and tell you where the performance problems are and why. I'm at `"%s@%s.%s","mdunlavey","pharsight","com"` – Mike Dunlavey Oct 13 '10 at 13:02
  • @Mike Dunlavey: I decided to go with an application written in C, which took a bit of time to write but gives me significant performance improvement over the Python equivalent. Thank you for offering to look at this though! – Alex Reynolds Oct 14 '10 at 20:33
  • @Alex: Aw shucks. I was hoping to show off a little. I'm sure C can be positively zippy. Good Luck. – Mike Dunlavey Oct 14 '10 at 21:41
  • @AlexReynolds DO you know how to sum the `tot time` column? – Chuck Nov 03 '18 at 21:12

3 Answers3

29

ncalls is relevant only to the extent that comparing the numbers against other counts such as number of chars/fields/lines in a file may highligh anomalies; tottime and cumtime is what really matters. cumtime is the time spent in the function/method including the time spent in the functions/methods that it calls; tottime is the time spent in the function/method excluding the time spent in the functions/methods that it calls.

I find it helpful to sort the stats on tottime and again on cumtime, not on name.

bgchar definitely refers to the execution of the script and is not irrelevant as it takes up 8.9 seconds out of 13.5; that 8.9 seconds does NOT include time in the functions/methods that it calls! Read carefully what @Lie Ryan says about modularising your script into functions, and implement his advice. Likewise what @jonesy says.

string is mentioned because you import string and use it in only one place: string.find(elements[0], 'p'). On another line in the output you'll notice that string.find was called only once, so it's not a performance problem in this run of this script. HOWEVER: You use str methods everywhere else. string functions are deprecated nowadays and are implemented by calling the corresponding str method. You would be better writing elements[0].find('p') == 0 for an exact but faster equivalent, and might like to use elements[0].startswith('p') which would save readers wondering whether that == 0 should actually be == -1.

The four methods mentioned by @Bernd Petersohn take up only 3.7 seconds out of a total execution time of 13.541 seconds. Before worrying too much about those, modularise your script into functions, run cProfile again, and sort the stats by tottime.

Update after question revised with changed script:

"""Question: What can I do about join, split and write operations to reduce the apparent impact they have on the performance of this script?""

Huh? Those 3 together take 2.6 seconds out of the total of 13.8. Your parseJarchLine function is taking 8.5 seconds (which doesn't include time taken by functions/methods that it calls. assert(8.5 > 2.6)

Bernd has already pointed you at what you might consider doing with those. You are needlessly splitting the line completely only to join it up again when writing it out. You need to inspect only the first element. Instead of elements = line.split('\t') do elements = line.split('\t', 1) and replace '\t'.join(elements[1:]) by elements[1].

Now let's dive into the body of parseJarchLine. The number of uses in the source and manner of the uses of the long built-in function are astonishing. Also astonishing is the fact that long is not mentioned in the cProfile output.

Why do you need long at all? Files over 2 Gb? OK, then you need to consider that since Python 2.2, int overflow causes promotion to long instead of raising an exception. You can take advantage of faster execution of int arithmetic. You also need to consider that doing long(x) when x is already demonstrably a long is a waste of resources.

Here is the parseJarchLine function with removing-waste changes marked [1] and changing-to-int changes marked [2]. Good idea: make changes in small steps, re-test, re-profile.

def parseJarchLine(chromosome, line):
    global pLength
    global lastEnd
    elements = line.split('\t')
    if len(elements) > 1:
        if lastEnd != "":
            start = long(lastEnd) + long(elements[0])
            # [1] start = lastEnd + long(elements[0])
            # [2] start = lastEnd + int(elements[0])
            lastEnd = long(start + pLength)
            # [1] lastEnd = start + pLength
            sys.stdout.write("%s\t%ld\t%ld\t%s\n" % (chromosome, start, lastEnd, '\t'.join(elements[1:])))
        else:
            lastEnd = long(elements[0]) + long(pLength)
            # [1] lastEnd = long(elements[0]) + pLength
            # [2] lastEnd = int(elements[0]) + pLength
            sys.stdout.write("%s\t%ld\t%ld\t%s\n" % (chromosome, long(elements[0]), lastEnd, '\t'.join(elements[1:])))
    else:
        if elements[0].startswith('p'):
            pLength = long(elements[0][1:])
            # [2] pLength = int(elements[0][1:])
        else:
            start = long(long(lastEnd) + long(elements[0]))
            # [1] start = lastEnd + long(elements[0])
            # [2] start = lastEnd + int(elements[0])
            lastEnd = long(start + pLength)
            # [1] lastEnd = start + pLength
            sys.stdout.write("%s\t%ld\t%ld\n" % (chromosome, start, lastEnd))               
    return

Update after question about sys.stdout.write

If the statement that you commented out was anything like the original one:

sys.stdout.write("%s\t%ld\t%ld\t%s\n" % (chromosome, start, lastEnd, '\t'.join(elements[1:])))

Then your question is ... interesting. Try this:

payload = "%s\t%ld\t%ld\t%s\n" % (chromosome, start, lastEnd, '\t'.join(elements[1:]))
sys.stdout.write(payload)

Now comment out the sys.stdout.write statement ...

By the way, someone mentioned in a comment about breaking this into more than one write ... have you considered this? How many bytes on average in elements[1:] ? In chromosome?

=== change of topic: It worries me that you initialise lastEnd to "" rather than to zero, and that nobody has commented on it. Any way, you should fix this, which allows a rather drastic simplification plus adding in others' suggestions:

def parseJarchLine(chromosome, line):
    global pLength
    global lastEnd
    elements = line.split('\t', 1)
    if elements[0][0] == 'p':
        pLength = int(elements[0][1:])
        return
    start = lastEnd + int(elements[0])
    lastEnd = start + pLength
    sys.stdout.write("%s\t%ld\t%ld" % (chromosome, start, lastEnd))
    if elements[1:]:
        sys.stdout.write(elements[1])
    sys.stdout.write(\n)

Now I'm similarly worried about the two global variables lastEnd and pLength -- the parseJarchLine function is now so small that it can be folded back into the body of its sole caller, extractData, which saves two global variables, and a gazillion function calls. You could also save a gazillion lookups of sys.stdout.write by putting write = sys.stdout.write once up the front of extractData and using that instead.

BTW, the script tests for Python 2.5 or better; have you tried profiling on 2.5 and 2.6?

John Machin
  • 81,303
  • 11
  • 141
  • 189
  • I agree with the interpretation of numbers. But I also looked at the code: the main work is obviously done in the last section marked "Extract data". What I see there is a lot of string creations of what must be a massive amount of data. And this is the point which needs optimization in my eyes. You will probably not get much more informative numbers if you put that code into a function. And yes, personally I would have structured this code differently. – Bernd Petersohn Oct 10 '10 at 10:09
  • I have replaced the source code and resulting cProfile analysis results. If you have a moment to take a look and offer suggestions, I would appreciate it. Thanks. – Alex Reynolds Oct 10 '10 at 17:37
  • Thank you for your helpful advice. I'm not a Python expert and would have had no idea that `int` types are promoted to `long` automatically, based on work in other languages. However, the changes you suggested seem only to shave off 0.8 sec. My script is still taking about as twice as long as a `csh/awk` solution (one that doesn't allow use of `seek`-based random access, like Python, and should be slower). Unless there are other Python-language-specific optimizations and tricks (which still allow writing to standard output) I think I may have to look into a C-based solution at this point. – Alex Reynolds Oct 10 '10 at 22:39
  • I started out running this under Python 2.5.2 and it was just as slow — I went to Python 2.7 to try to get any speed enhancements. I have been writing a C-based equivalent of this script the last couple days, and the result is lightning fast, in comparison, albeit with some bzip difficulties I'm still working out. I may post code later for comparison purposes. – Alex Reynolds Oct 12 '10 at 06:33
  • @Alex Reynolds: Did you test the last suggestion of John Machin? I think one main problem of your script is that it maintains an unnecessarily large memory footprint. In `extractData()`, you first read the uncompressed data block for one chromosome. Then a second string of the decompressed data is created. It contains about 1.75 millions of lines. It is easy to reach 1 GB here. Then you create a list of those 1.75 million lines. It would be better to wrap the uncompressed data in StringIO and iterate over its lines. You should also release the compressed data early using `del`. – Bernd Petersohn Oct 12 '10 at 14:20
  • Thank you for the clarification between cumtime and tottime. AFAIK It's not mentioned in the python docs, and it's not completely obvious either. – Overdrivr Aug 25 '17 at 03:45
2

This output is going to be more useful if your code is more modular as Lie Ryan has stated. However, a couple of things you can pick up from the output and just looking at the source code:

You're doing a lot of comparisons that aren't actually necessary in Python. For example, instead of:

if len(entryText) > 0:

You can just write:

if entryText:

An empty list evaluates to False in Python. Same is true for an empty string, which you also test for in your code, and changing it would also make the code a bit shorter and more readable, so instead of this:

   for line in metadataLines:      
        if line == '':
            break
        else:
            metadataList.append(line)

You can just do:

for line in metadataLines:
    if line:
       metadataList.append(line)

There are several other issues with this code in terms of both organization and performance. You assign variables multiple times to the same thing instead of just creating an object instance once and doing all accesses on the object, for example. Doing this would reduce the number of assignments, and also the number of global variables. I don't want to sound overly critical, but this code doesn't appear to be written with performance in mind.

jonesy
  • 3,502
  • 17
  • 23
  • I have replaced the source code and resulting cProfile analysis results. If you have a moment to take a look and offer suggestions, I would appreciate it. Thanks. – Alex Reynolds Oct 10 '10 at 17:37
  • First, I think what's been suggested here by various people are generally good advice. Second, I'm still thinking that the rampant use of globals is going to have an impact on something that parses a large amount of data -- put the appropriate functions into a class, and that goes almost completely away. Also, wrapping things in a class makes it far easier to experiment with solutions like threading and multiprocessing (I favor multiprocessing over threading for this case, fwiw). Showing sample input would likely get you more feedback. – jonesy Oct 11 '10 at 12:17
  • 2
    `if line == '': break; else: metadataList.append(line)` is not the same as `if line: metadataList.append(line)` – warvariuc Dec 02 '14 at 06:37
-1

The entries relevant for possible optimization are those with high values for ncalls and tottime. bgchr:4(<module>) and <string>:1(<module>) probably refer to the execution of your module body and are not relevant here.

Obviously, your performance problem comes from string processing. This should perhaps be reduced. The hot spots are split, join and sys.stdout.write. bz2.decompress also seems to be costly.

I suggest you try the following:

  • Your main data seems to consist of tab separated CSV values. Try out, if CSV reader performs better.
  • sys.stdout is line buffered and flushed each time a newline is written. Consider writing to a file with a larger buffer size.
  • Instead of joining elements before writing them out, write them sequentially to the output file. You may also consider using CSV writer.
  • Instead of decompressing the data at once into a single string, use a BZ2File object and pass that to the CSV reader.

It seems that the loop body that actually uncompresses data is only invoked once. Perhaps you find a way to avoid the call dataHandle.read(size), which produces a huge string that is then decompressed, and to work with the file object directly.

Addendum: BZ2File is probably not applicable in your case, because it requires a filename argument. What you need is something like a file object view with integrated read limit, comparable to ZipExtFile but using BZ2Decompressor for decompression.

My main point here is that your code should be changed to perform a more iterative processing of your data instead of slurping it in as a whole and splitting it again afterwards.

Bernd Petersohn
  • 2,234
  • 1
  • 15
  • 7