1
\$\begingroup\$

This is my first attempt at splitting the download buffer into several threads by the number provided. I want to improve on performance and better implementation approach.

import os
import requests
import threading
import urllib2
import time

url = "http://www.nasa.gov/images/content/607800main_kepler1200_1600-1200.jpg"
# url = 'http://pymotw.com/2/urllib/index.html'
# url = 'http://megaboon.com/common/preview/track/786203.mp3' # size cannot be determined.
# url = 'http://broadcast.lds.org/churchmusic/MP3/1/2/nowords/271.mp3'


def buildRange(value, numsplits):
    lst = []
    for i in range(numsplits):
        if i == 0:
            lst.append('%s-%s' % (i, int(round(1 + i * value/(numsplits*1.0) + value/(numsplits*1.0)-1, 0))))
        else:
            lst.append('%s-%s' % (int(round(1 + i * value/(numsplits*1.0),0)), int(round(1 + i * value/(numsplits*1.0) + value/(numsplits*1.0)-1, 0))))
    return lst

class SplitBufferThreads(threading.Thread):
    """ Splits the buffer to ny number of threads
        thereby, concurrently downloading through
        ny number of threads.
    """
    def __init__(self, url, byteRange):
        super(SplitBufferThreads, self).__init__()
        self.__url = url
        self.__byteRange = byteRange
        self.req = None

    def run(self):
        self.req = urllib2.Request(self.__url,  headers={'Range': 'bytes=%s' % self.__byteRange})

    def getFileData(self):
        return urllib2.urlopen(self.req).read()


def main(url=None, splitBy=3):
    start_time = time.time()
    if not url:
        print "Please Enter some url to begin download."
        return

    fileName = url.split('/')[-1]
    sizeInBytes = requests.head(url, headers={'Accept-Encoding': 'identity'}).headers.get('content-length', None)
    print "%s bytes to download." % sizeInBytes
    if not sizeInBytes:
        print "Size cannot be determined."
        return

    dataLst = []
    for idx in range(splitBy):
        byteRange = buildRange(int(sizeInBytes), splitBy)[idx]
        bufTh = SplitBufferThreads(url, byteRange)
        bufTh.start()
        bufTh.join()
        dataLst.append(bufTh.getFileData())

    content = ''.join(dataLst)

    if dataLst:
        if os.path.exists(fileName):
            os.remove(fileName)
        print "--- %s seconds ---" % str(time.time() - start_time)
        with open(fileName, 'w') as fh:
            fh.write(content)
        print "Finished Writing file %s" % fileName

if __name__ == '__main__':
    main(url)

I hope this is not a challenging one to suggest or comment on improvement.

\$\endgroup\$
1
\$\begingroup\$

Utilizing Threads:

You are doing a number of things to slow down and serialize your code while the point of threads is to do things in parallel.

for idx in range(splitBy):
    byteRange = buildRange(int(sizeInBytes), splitBy)[idx]
    bufTh = SplitBufferThreads(url, byteRange)
    bufTh.start()
    bufTh.join()
    dataLst.append(bufTh.getFileData())

In the first line, you generate the full list of byte ranges, but then only use the the range for the specific index you are on. This only needs to be generated once, not every time you start a new chunk.

Then in the next three line, you create the thread, start it, and then immediately wait for it to finish. This means you are never running more than one thread at a time. This implementation will be slower than a single threaded implementation because of the over head involved.

On the last line you get the datafile and append all the bytes into your buffer. This is where the bottle-neck is. Request is just a class that contains the information describing what should be requested. urlopen is the function that actually makes the internet connection. Even if you weren't serializing your thread creation and execution, you still would not have a performance gain because the function that will take time is not part of the run method. You want to execute all the code in the run() method, then just access the resultant data after the thread has terminated.

The body of your main should be like this:

threads = []
byteRanges = buildRange(int(sizeInBytes), splitBy)
for idx in range(splitBy):
    t = SplitBufferThreads(url, byteRange)
    t.start()
    threads.append(t)

# all of the threads are downloading files at this point

for t in threads:
    t.join()

# file prep stuff ...

with open(fileName, 'w') as fh:
    for t in threads:
        fh.write(t.data) #where run() stores the data it read

Edit: It seems like you need a better understanding of how threads work. When you start a thread (in the main thread), run() is executed in the context of the thread that was started. The main thread can then continue to execute other tasks while the other thread is executing in the background. Once run() terminates, nothing else executes in the thread.

The other key point is that the call to urlopen() is when the download begins to happen. If this is not done in the context of a separate thread, it provides no performance gains. This means it must be called from run() or a method run() calls. Otherwise, it will execute in the main thread.

The thread class should be implemented like this:

class SplitBufferThreads(threading.Thread):
    """ Splits the buffer to ny number of threads
        thereby, concurrently downloading through
        ny number of threads.
    """
    def __init__(self, url, byteRange):
        super(SplitBufferThreads, self).__init__()
        self.__url = url
        self.__byteRange = byteRange

    def run(self):
        req = urllib2.Request(self.__url,  headers={'Range': 'bytes=%s' % self.__byteRange})
        # here is where the file download happens within the context of the thread
        self.data = getFileData(req)

    def getFileData(self, request):
        return urllib2.urlopen(request).read()

buildRange() is going too much on one line.

lst.append('%s-%s' % (i, int(round(1 + i * value/(numsplits*1.0) + value/(numsplits*1.0)-1, 0))))

Adding to a list, formating a string, and two mathematic equations. The equations are non-trivial and it is not immediately apparent what they are actually doing. Both equations should be broken out into separate functions.

Additionally, the if statement is deciding how the first and second values are calculated. It does not effect the string formating or how the list is mutated.

def buildRange(value, numsplits):
    lst = []
    for i in range(numsplits):
        if i == 0:
            first = i
            second = int(round(1 + i * value/(numsplits*1.0) + value/(numsplits*1.0)-1, 0))
        else:
            first = int(round(1 + i * value/(numsplits*1.0),0))
            second = int(round(1 + i * value/(numsplits*1.0) + value/(numsplits*1.0)-1, 0))
        lst.append("{}-{}".format(first, second))
    return lst

Once you do this, it becomes clear that second is not effected by the if statement. Now, we can make the function even simpler. Replacing the equations greatly improves the readability.

def buildRange(value, numsplits):
    lst = []
    for i in range(numsplits):
        first = i if i == 0 else start(i, value, numsplits)
        second = end(i, value, numsplits)
        lst.append("{}-{}".format(first, second))
    return lst

Naming:

lst is a bad variable name. We know it is a list from when the variable is initialized, but we don't know what it is meant to store.

buildRange() returns a list of things, yet the name makes it sound like it only makes one.

In my examples, first and second are also bad names, but I hadn't fully understood what the values represented until the code was cleaner.

SplitBufferThreads is a single thread, it should not be pluralized.

The first argument to buildRange() is value. The name of the variable that is passed in for this argument is sizeInBytes. Which one gives you a better idea about what the code is trying to do?

The Python convention for naming function is underscore_spaced, not camelCase.


You are mixing the meaning of a range and how it is used by the specific library you are using. If you changed the library you are using to make the specific request, you then need to change how you are constructing your ranges or parse out the values from the string.

\$\endgroup\$
  • \$\begingroup\$ well, everything seems fine, except for AttributeError: 'SplitBufferThread' object has no attribute 'data' in the run method !!! \$\endgroup\$ – Ciasto piekarz Jul 6 '14 at 4:41
  • \$\begingroup\$ Yes, you need to edit the run method so that it stores the result of urllib2.urlopen() to data. There is a comment in the re-written example saying this is required. \$\endgroup\$ – unholysampler Jul 6 '14 at 5:06
  • \$\begingroup\$ would this be better approach : ` @property def getFileData(cls): return urllib2.urlopen(cls.req).read()` and where I collect data I can just do for t in threads: fh.write(t.getFileData) \$\endgroup\$ – Ciasto piekarz Jul 6 '14 at 5:11
  • \$\begingroup\$ @san: No. See my edit for a more detailed explanation of why this is the case. \$\endgroup\$ – unholysampler Jul 6 '14 at 6:15
  • 1
    \$\begingroup\$ No, I meant a thread pool. If you have more questions, feel free to post you new code as a new question, as this is becoming more of a conversation instead of comments on my answer. \$\endgroup\$ – unholysampler Jul 7 '14 at 12:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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