5
\$\begingroup\$

I didn't use multi-threading so far as I didn't need to. But as far as I've read, implementing them will make my program slightly faster than it actually is.

from validate_email import validate_email
import os

# the program is reading each line from "emails.txt" and after it checks each email it will remove duplicates and sort the godd / bad emails

def verify_emails(all_emails_file, all_good_emails_file, all_bad_emails_file):
    with open(all_emails_file) as f: all_emails = f.readlines()

    rs_emails = [elem.strip('\n') for elem in all_emails]
    rs_emails_set = set(rs_emails)  # remove duplicates

    good_emails_file, bad_emails_file = open(all_good_emails_file, 'w+'), open(all_bad_emails_file, 'w+')

    for email in rs_emails_set:
        if validate_email(email, verify=True):
            print >> good_emails_file, email
        else:
            print >> bad_emails_file, email

if __name__ == "__main__":
    clear = lambda: os.system('cls')
    clear()
    try:
        verify_emails("emails.txt", "good_emails.txt", "bad_emails.txt")
    except:
        print "\n\nFile with emails could not be found. Please create emails.txt and run the program again\n\n"

My code is perfectly functional, but when it handles big files ( > 2k rows ) it runs really slow. I'd like to take the best out of it and make it as faster as possible using multi-threading or any other methods.

I'd like when answering, if possible, somebody to explain me whether using multi-threading will help me optimize the program or not. More, I'd like somebody to also explain from his / her experience how can I optimize my code

\$\endgroup\$
  • 3
    \$\begingroup\$ Without looking at your code: almost certainly no. Your problem is I/O bound, not CPU bound. multi-threading only helps when doing complex CPU operations (and even then it isn't guaranteed to). Multi-threading is not a silver bullet. Plus, unless you're using a specific version of Python, you have GIL, which basically makes multithreading useless. \$\endgroup\$ – Dan Pantry Oct 8 '15 at 8:43
  • \$\begingroup\$ Code Review is not the site to ask for help in fixing or changing what your code does. This is still a valid review, but it would be better if you weren't asking people to implement multithreading in your code, instead ask how you can improve the performance. \$\endgroup\$ – SuperBiasedMan Oct 8 '15 at 8:47
  • \$\begingroup\$ I think you might get better replies if you ask how to make your code more efficient, because that's what you are really asking. Multi-threading is just one vector to do that. \$\endgroup\$ – Dan Pantry Oct 8 '15 at 8:47
  • \$\begingroup\$ @SuperBiasedMan I've made some changes. I hope everything it's ok now \$\endgroup\$ – Cajuu' Oct 8 '15 at 8:55
  • 1
    \$\begingroup\$ I expect contacting SMTP servers to be the most expensive part by far. Using some form of parallel or asynchronous IO should speed this up a lot, since you can wait for multiple servers to respond instead of waiting sequentially. But I don't know enough python to help you with that. \$\endgroup\$ – CodesInChaos Oct 8 '15 at 12:45
5
\$\begingroup\$

Performance

As others have pointed out, reading the whole lot into memory is bad for performance. To elaborate a bit on this, it means that you're taking up much more memory than you need to and then performing expensive operations on large data sets. If you use for line in file then it's actually only taking one line at a time, which is much lighter on memory. You also have multiple loops even though you could easily perform all your operations in a single iteration, like this:

def verify_emails(email_path, good_filepath, bad_filepath):
    good_emails = open(good_filepath, 'w+')
    bad_emails = open(bad_filepath, 'w+')
    emails = set()
    with open(email_path) as f:
        for email in f:
            email = email.strip()

I'm using strip() here so that all whitespace (including \n) gets removed.

            if email in emails:
                continue

This tests whether email is already in set and if it is will continue to the next iteration of the loop, ie. the next line in the file. If that passes, then you can just add the email to the set and verify it as you did before.

            emails.add(email)
            if validate_email(email, verify=True):
                print >> good_emails, email
            else:
                print >> bad_emails, email

One important note is that you didn't manually close your files, you should always do this! Ideally you'd use with open(file) as f but it's not as easy to set up here. So instead just remember to close them after the loop.

    good_emails.close()
    bad_emails.close()

Style notes

all_emails_file, all_good_emails_file, all_bad_emails_file are bad names. For a start, if all of them have all_ and _file then what's the point of including those? Names should be clear identifiers of a variable's purpose but that doesn't mean you need to give all the details. Sometimes names just need to distinguish different data from each other. In this case it can be assumed they're all files, and all isn't a good description so it's better to have something like email_path, good_filepath, bad_filepath.

Using print >> good_emails, email is an odd construction. You should instead use good_emails.write(email + '\n'). It's more readable and commonly used.

Avoid using lines like with open(all_emails_file) as f: all_emails = f.readlines(). They're usually a bad idea because people might miss the fact that there's a statement at the end and it just becomes confusing.

I don't understand this:

clear = lambda: os.system('cls')
clear()

why not just call os.system('cls')? If there's a good reason, explain with a comment.

And don't use a bare except, that will catch any error at all that happens in your function. If you're going to do this at the very least catch except Exception as error, because then you can print information from error like error.message and type(error) to find out what actually happened. But really, you should know whether you're expecting a ValueError or an IOError and specifically handle what you expect to happen.

\$\endgroup\$
  • \$\begingroup\$ That was a perfect explanation. As for the lambda thing I didn't have a reason for using it. Just did it as it was. \$\endgroup\$ – Cajuu' Oct 8 '15 at 10:03
  • \$\begingroup\$ @SuperBiasedMan do you have an opinion about not writing the files at every loop cycle, but creating a structure and write them at once at the end? (with a join for the linebreaks) \$\endgroup\$ – oliverpool Oct 8 '15 at 11:02
  • 1
    \$\begingroup\$ @oliverpool It can be a good idea. It prevents possible issues with the unclosed files. But it means that you are taking up a lot of memory as you add to them. Writeable file objects don't increase in memory as you write to them, and since OP was concerned about performance I thought it'd be better to stick with them. \$\endgroup\$ – SuperBiasedMan Oct 8 '15 at 11:06
  • \$\begingroup\$ @SuperBiasedMan you're right. Even if oliverpools ideea is good, your solution is the one that best fit to this situation. More, what it would happen if you'd add threads ? The performance won't change at all ? Would you mind trying a second part of your answer involving those and explaining in detail why it won't be a good idea ? \$\endgroup\$ – Cajuu' Oct 8 '15 at 11:30
  • \$\begingroup\$ @SuperBiasedMan I didn't know that python already does a buffering when writing files line by line. But actually it might be possible to improve it by writing batch of emails (500 at once for instance) \$\endgroup\$ – oliverpool Oct 8 '15 at 13:48
5
\$\begingroup\$

As stated in the comments, multi-threading might not be what you are looking for.


I think there is room for improvement on the way you read your file. Currently:

  1. you read the whole file into a string all_emails = f.readlines()
  2. you remove duplicates rs_emails_set = set(rs_emails) # remove duplicates
  3. and you read every element of this array for email in rs_emails_set:

Reading this comment, I strongly recommend you to test the following:

processed_emails = set()
for email in f: 
  if email not in processed_emails:
    # validate email or not
    processed_emails.add(email)

Instead of immediately writing the good and bad emails files, you could store those into 2 lists and write them at once at the very end (removing some I/O with the filesystem as well)

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

Multithreading is not a silver bullet.

As it is your code is I/O bound and the only way to speed that up further is getting a faster harddrive.

There is one little thing you can do:

In your current code there are 3 distinct phases; reading the file into memory, removing duplicates and validation+output.

What you can do is interleave the phases so you can get to work with output while you are still reading in:

rs_emails_set = Set()

for email in f:
    if email not in rs_emails_set
        continue # we already processed this one
    else
        rs_emails_set.add(email)

    if validate_email(email, verify=True):
        print >> good_emails_file, email
    else:
        print >> bad_emails_file, email

This will mostly save you from copying the entire file into memory before processing but instead processes the emails one at a time as they come in.

\$\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.