4
\$\begingroup\$

I am trying to create a program to download a long list of websites using Python 3.7. By using multiprocessing for each request, my code runs much faster than when it's run synchronously.

I've added a timer to my script so I can know the execution time for the script. The one I wrote below takes on average approximately 3.5 seconds to run. However, when I use this script for my long list of URL's, it takes approximately 8 minutes to execute.

I am wondering: is it taking this long because my internet is slow? Or is my script just bad?

My internet speed is pretty low (3 mb/s download, 1 mb/s upload). If anyone has faster internet, could you post your execution time for this script along with your internet speed? This way, I can see if the bottleneck is internet speed or not.

If the bottleneck isn't internet speed, but simply that my script is bad, can anyone offer any suggestions for improving the script so that the execution time is better? Thanks coders!

from urllib.request import Request, urlopen
from multiprocessing.dummy import Pool as ThreadPool
import datetime
import time
import requests

start_time = time.time()

urls = [
  'https://en.wikipedia.org/wiki/Python_(programming_language)',
  'https://www.python.org/',
  'https://stackoverflow.com/questions/tagged/python',
  'https://github.com/python',
  'https://realpython.com/python-beginner-tips/',
  'https://pythonprogramming.net/introduction-to-python-programming/',

  ]

mylist = []

def my_function(url_to_parse):
  req = Request(url_to_parse, headers={'User-Agent': 'Mozilla/5.0'})
  resp = urlopen(req).read()
  mylist.append(resp)

#------------------------------------------------------------------------------------------

pool = ThreadPool(8)
results = pool.map(my_function, urls)
pool.close()
pool.join()

print("--- %s seconds --- REQUESTS" % (time.time() - start_time))
```
\$\endgroup\$
2
  • \$\begingroup\$ Welcome to CR! How many cores do you have on your machine and how long is your long list? \$\endgroup\$ – ggorlen Jul 14 '19 at 23:40
  • \$\begingroup\$ @ggorlen Thanks! I have 4 cores on my PC (and 8 processors, at 3.4 GHz). There are about 500 URL's on my list. \$\endgroup\$ – George Orwell Jul 15 '19 at 0:11
1
\$\begingroup\$

Firstly, it's a good idea to pythonize your code before we begin.

When I ran your code, I saw the time to run calculation as the first thing. Looking into your code, I found that line at the end, then had search your entire program before I understood where start_time was defined. An important thing with programming is - other programmers shouldn't have to do any hunting to understand your code. They will get frustrated if they have to keep searching through spaghetti to find what they're looking for.

So, to fix that, and because we're writing in Python, it's important to have the standard entry point in your code. There are two reasons for this - first it tells your readers where your code starts "here", and second - if your code employs automated documentors, like Sphinx, it instantiates all the objects in your code before performing reflection to document your code. As is, your code would be executed immediately, which would break that intended functionality.

if __name__ == "__main__":
    start_time = time.time()
    mylist = []
    pool = ThreadPool(8)
    results = pool.map(my_function, urls())
    pool.close()
    pool.join()
    print_results() 

Also, all your program flow is in one place. I can read this, and know what your code does without having to read all the different functions. I moved the print results into it's own function - as you will likely make changes to the print function as time progresses. If you make a change, and you create an error, the trace-back will point clearly to this function as being the error, instead of the main routine as being the error. This makes it easier and faster to fix bugs when you code. Keep everything in it's own function, this is the Single Responsibility Principle (SOLID).

def print_results():
    print("--- %s seconds --- REQUESTS" % (time.time() - start_time))

You will notice I've changed results = pool.map(my_function, urls()) - it's important to reduce variables into what they are - as your url variable returns a list of URLs, why not make it a simple function so it is called in one place without the overhead of maintaining memory space? (... for display)

def urls():
    return [
        'https://en.wikipedia.org/wiki/Python_(programming_language)',
        ... 
        'https://pythonprogramming.net/introduction-to-python-programming/',
    ]

Another comment with that - as time goes on, you'll add and remove URLs, won't you? Every time you open your code up to make a change, there's a chance you'll inadvertently change the behavior of the code or accidentally break the code. We call this the Open Close Principle (SOLID) - your code should be open for enhancement, but closed for modification. To complete the point - it's best you export your URL list into a text/ini file, and when you need to add/remove URLs, you make the changes there, without the code being modified.

Furthermore, think if your module was part of a large compiled program. If you changed a URL inside the code, it would need to be recompiled. Anything that used that library would need to be recompiled too, making your simple change quite laborious. See the point?

Looking further into your code, we can see that mylist[] is created, but you don't really use it - because everything comes back from the pool.map into results. So let's change your code a little to improve that (... for display):

def fetch_url(url_to_parse):
    req = Request(url_to_parse, headers={'User-Agent': 'Mozilla/5.0'})
    resp = urlopen(req).read()
    return resp

def print_results(results):
    print(f"--- {time.time() - start_time} Requests: {len(results)}")

if __name__ == "__main__":
    ...
    print_results(results)

Notice I renamed my_function to fetch_url - my_function doesn't mean anything, but fetch_url does. Make sure you name your functions to match what they do, otherwise other programmers need to read the entire function to understand what it does first. I've also used f-strings (Python 3.6+) which make print statements much cleaner. Take some time to learn them, they're good.

Now that we've fixed your code, here it is:

from urllib.request import Request, urlopen
from multiprocessing.dummy import Pool as ThreadPool
import time


def urls():
    return [
        'https://en.wikipedia.org/wiki/Python_(programming_language)',
        'https://www.python.org/',
        'https://stackoverflow.com/questions/tagged/python',
        'https://github.com/python',
        'https://realpython.com/python-beginner-tips/',
        'https://pythonprogramming.net/introduction-to-python-programming/',
    ]


def read_url(url_to_parse):
    req = Request(url_to_parse, headers={'User-Agent': 'Mozilla/5.0'})
    return urlopen(req).read()


def print_results(results):
    print(f"--- {time.time() - start_time} Requests: {len(results)}")


if __name__ == "__main__":
    start_time = time.time()
    pool = ThreadPool(8)
    results = pool.map(read_url, urls())
    pool.close()
    pool.join()
    print_results(results)

If you want to make your code faster, it's clear the fetching is the slow part. I notice you're mixing threading concepts and multiprocessor concepts, you should spend some time on understanding the differences. Finally, now that your code is modular, it's easy to drop-in a replacement for the fetch_url function. Here is an example which I tested with your code:

https://stackoverflow.com/questions/3472515/python-urllib2-urlopen-is-slow-need-a-better-way-to-read-several-urls

Hope this helps, Good Luck!

\$\endgroup\$
0
2
\$\begingroup\$

serializing

You are serializing all HTML text and sending it back to the parent. That can be time consuming.

Consider having the child (my_function()) write HTML text to a file in /tmp, and pass back the temporary filename to the parent. Better, also pass back some timestamps: (start, end, filename)

stragglers

If you can predict that a page will be "big" or a site will be "slow", sort those to the front of your list of 500 URLs. That way you won't have a handful of sluggish stragglers impacting your overall time, while most cores are idle and waiting for them to complete.

\$\endgroup\$
4
  • \$\begingroup\$ Hi, thanks for the reply! Regarding stragglers, almost all of the sites are about the same size, so I think it's more simple to not take this into account. However, for serializing, do you know how I can do this? I just looked it up but I'm a beginner to Python so I'm having trouble understanding. \$\endgroup\$ – George Orwell Jul 15 '19 at 0:53
  • \$\begingroup\$ I advised you not to serialize many kilobytes of HTML text coming back to a single-threaded parent. Instead, prefer to serialize the HTML text out to the filesystem in each of N children, and pass just a tiny filename back to the parent. \$\endgroup\$ – J_H Jul 15 '19 at 2:08
  • \$\begingroup\$ I might remember wrongly, but isn't writing to a file subject to the global interpreter lock, meaning that you would lose out anyway, especially as writing to a file can be expensive. \$\endgroup\$ – Ninetails Jul 15 '19 at 7:06
  • \$\begingroup\$ That's not relevant here. It could be, in a threaded setting, but this is multiprocessing. Each child is its own process, running its own copy of the python interpreter. So for N children we have N non-interfering GILs. \$\endgroup\$ – J_H Jul 15 '19 at 15:40
1
\$\begingroup\$

The main thing that tends to take time in acting over the net is the wait time between sending a request and getting a response, as opposed to the actual computations you have to do during the processing. It is therefore much more important to have a highly concurrent implementation than optimizing for parralel processing. In your case I would either increase the amount of threads in the pool to be the same length as as the number of pages to load, or use a concurrency library instead of a paralization library, especially if the thread library starts to have some kind of problems with such a high amount of threads (as they might have higher overhead).

\$\endgroup\$

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.