0
\$\begingroup\$

I have written a Python program to loop through a list of X files, open each one, read line by line, and write (append) to an output file. Being that these files are several GB each, it is taking very long.

I am looking for suggestions to improve the performance of this program. I have no formal CS training so it's likely I am missing the "obvious solution" to this problem; I have done some research but again, my limited knowledge (and other higher priority tasks) limits my ability to implement such.

for name in PR_files:
    with open(PR_path + name, 'r') as f:
        line = f.readline()
        while line:
            with open(PR_out_path, 'a') as g:
                g.write(line + '\n')
                line = f.readline()
    f.close()

The program will work but will have a blank line between each line in the output text file; this is because the first line of the next file began on the last line of the previous file (my solution to this problem was to add '\n' to each line being written to the output file. For that reason I wrote another block to remove all blank lines in the output file (yes I know, very inefficient).

# this removes all blank lines from out put file
with open(PR_out_path) as this, open(PR_out_path_fix, 'w') as that:
    for line in this:
        if not line.strip():
            continue
        that.write(line)

PLEASE NOTE: I attempted to do this, as oppose to reading line by line but I received a MemoryError.

with open(PR_out_path, 'a') as g:
    for name in PR_files:
        with open(PR_path + name, 'r') as f:
            g.write(f.read())
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Welcome to Code Review! The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How to Ask for examples, and revise the title accordingly. \$\endgroup\$
    – Vogel612
    Aug 24, 2018 at 17:40
  • 4
    \$\begingroup\$ Unrelatedly: I'm wondering why you're using python to do this. If you're on a linux system, you could just use cat <files> > <outpath> \$\endgroup\$
    – Vogel612
    Aug 24, 2018 at 17:42

2 Answers 2

3
\$\begingroup\$

Your original code:

with open(PR_out_path, 'a') as g:
    for name in PR_files:
        with open(PR_path + name, 'r') as f:
            g.write(f.read())

works but, as you found, has problems if the entire file can't be read into memory. The solution to that problem is to read the input file in chunks:

with open(PR_out_path, 'a') as g:
    for name in PR_files:
        with open(PR_path + name, 'r') as f:
            while True:
                data = f.read(ChunkSize)
                if not data:
                    break
                g.write(data)

where ChunkSize is something like 1GB.

But if speed is your only requirement why not use the tools offered by the operating system, as others have noted?

\$\endgroup\$
2
  • \$\begingroup\$ Chunk size should definitely be much smaller than 1GB. For example, Golang's io.Copy uses 32K chunks: golang.org/src/io/io.go?s=12784:12844#L353 \$\endgroup\$ Aug 25, 2018 at 17:57
  • \$\begingroup\$ I agree that ChunkSize is something that should be experimented with, but 1GB worked on MacOS during testing. YMMV. \$\endgroup\$
    – rzzzwilson
    Aug 26, 2018 at 23:53
0
\$\begingroup\$

@rzzzwilson has the right idea with what you should be doing, but let's break down your code to address some of the other performance and style issues:

In your first snippet you should be doing a few things:

  • Use pathlib instead of concatenating paths
  • You are using readline() strangely (we usually avoid while in Python). If you want to be reading by line (of course, by line is definitely not the approach you want here), you can just do for line in file:
  • You open and close PR_out_path a bunch of times. It should be opened first and remain open for the duration of for name in PR_files. Besides the obvious (that you need it for this long), this will also likely have an impact on caching as closing the file will force your writes to be flushed.
  • You get newlines between each line in the file because you do g.write(line + '\n') but line already ends in '\n'!
  • No need to do f.close() when you use a context manager (with open(...):). Read more about context managers
  • No need to do open('...', 'r'), 'r' is the default

So with that in mind, a rewrite of your first snippet would look like:

from pathlib import Path

pr_path = Path('/path/to/pr/files')
pr_files = (pr_path / name for name in ('pr_file_1', 'pr_file_2'))

with pr_out_path.open() as output:
    for pr_file in pr_files:
        with pr_file.open() as f:
            for line in pr_file:
                output.write(line)

I suspect this will behave slightly better than your approach, but it's still not optimal. Your second script is entirely unnecessary because I did output.write(line) instead of output.write(line + '\n'). But, if you did need to do some processing on each line, why do you use an intermediate file? Why not just do it in your loop above? For example:

with pr_out_path.open() as output:
    for pr_file in pr_files:
        with pr_file.open() as f:
            for line in pr_file:
                output.write(your_fancy_line_processing(line))

Your final snippet as was already pointed out has a problem that it reads the entirety of each file into memory (that's what f.read() does). This won't work for large files.

The solution that @rzzzwilson proposes is definitely the correct one. Buffered writing by using chunks (I'd choose a size that's in the kilobytes, perhaps the megabytes, but nothing too crazy) allows everything to fit into memory and composes nicely with some of the other buffering, cacheing, and prefetching already done by the userland IO library, operating system, and even harddisk itself.

However, there is something better if you are on a *nix. Python provides os.sendfile() which uses sendfile. This has the advantage that you don't have to do any copying yourself. All of the copying is done by the OS (which is much faster for many reasons, but most of them boiling down to context switching). You can use it like so:

from os import sendfile

with pr_out_path.open() as output:
    for pr_file in pr_files:
        with pr_file.open() as f:
            sendfile(output.fileno(), f.fileno())

Windows doesn't support sendfile though, so you'd likely want to use this and fall back to copying the file in chunks when not supported. When you do this make sure to open in binary mode ('rb' and 'wb') as this matters on windows.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.