3
\$\begingroup\$

I've made a python script to get some statistics from a server log. I would like some guidelines to improve my code: make it run faster, be more pythonic, etc. I don't write python professionally (embedded developer) so please go easy on me.

Here's the code:

'''
Find the number of 'exceptions' and 'added' event's in the exception log
with respect to the device ID.

author: clmno
date: Dec 23 2018
'''

from time import time
import re

def timer(fn):
    ''' Used to time a function's execution'''
    def f(*args, **kwargs):
        before = time()
        rv = fn(*args, **kwargs)
        after = time()
        print("elapsed", after - before)
        return rv
    return f

def find_sql_guid(txt):
    ''' From the passed in txt, find the SQL guid using re'''
    re1 = '.*?'
    # SQL GUID 1
    re2='([A-Z0-9]{8}-[A-Z0-9]{4}-[A-Z0-9]{4}-[A-Z0-9]{4}-[A-Z0-9]{12})'
    rg = re.compile(re1+re2, re.IGNORECASE|re.DOTALL)
    m = rg.search(txt)
    if m:
        guid1 = m.group(1)
        return guid1
    else:
        print("ERROR: No SQL guid in line. Check the code")
        exit(-1)

def find_device_IDs(path, element):
    """ Find the element (type: str) within the file (file path is 
        provide as arg). Then find the SQL guid from the line at hand.
        (Each line has a SQL guid)
        Return a dict of {element: [<list of SQL guids>]}
    """
    lines = dict()
    lines[element] = []
    with open(path) as f:
        for line in f:
            if str(element) in line:
                #find the sql-guid from the line-str & append
                lines[element].append(find_sql_guid(line))
    return lines

# @timer
def find_num_occurences(path, key, search_val, unique_values):
    """ Find and append SQL guids that are in a line that contains a string
        that's in search_val into 'exception' and 'added'
        Return a dict of {'exception':set(<set of SQL guids>), 
                        'added': set(<set of SQL guids>)}
    """
    lines = {'exception':set(), 'added': set()}

    with open(path) as f:
        for line in f:
            for value in unique_values:
                if value in line:
                    if search_val[0] in line:
                        lines['exception'].add(value)
                    elif search_val[1] in line:
                        lines['added'].add(value)
    return lines

def print_stats(num_exceptions_dict):
    for key in num_exceptions_dict.keys():
        print("{} added ".format(key) + 
            str(len(list(num_exceptions_dict[key]["added"]))))
        print("{} exceptions ".format(key) + 
            str(len(list(num_exceptions_dict[key]["exception"]))))

if __name__ == "__main__":
    path  = 'log/server.log'
    search_list = ['3BAA5C42', '3BAA5B84', '3BAA5C57', '3BAA5B67']

    #find every occurance of device ID and find their corresponding SQL
    # guids (unique ID)
    unique_ids_dict = dict()
    for element in search_list:
        unique_ids_dict[element] = find_device_IDs(path, element)

    #Now for each unique ID find if string ["Exception occurred", 
    # "Packet record has been added"] is found in it's SQL guid list.
    search_with_in_deviceID = ["Exception occurred", 
                                "Packet record has been added"]

    num_exceptions_dict = dict()
    for elem in search_list:
        num_exceptions_dict[elem] = find_num_occurences(path, elem, 
            search_with_in_deviceID, list(unique_ids_dict[elem].values())[0])

    print_stats(num_exceptions_dict)

and here's a small server log for you to experiment on.
This code works but the time elapsed to run through a log file of 55000 lines is 42 seconds

$ time python findUniqueVal_log.py
 real   0m42.343s
 user   0m42.245s
 sys    0m0.100s

I would like guidelines on:

  • Optimizing in both coding and performance efficiency
  • Any better approach to this problem
  • I keep opening the file and closing it. Is that IO delay causing any noticeable delay?
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

Your timer function should probably go away, to use the built-in timeit instead; or at least timer should wrap timeit.

I'm unclear on why re1 and re2 are separate. Just have them in the same string. If you want to make it clear that they're separate parts of the regex, you could show the construction of the regex as a concatenation, i.e.

rg = re.compile('.*?'  # Prefix to do foo
                # This is a SQL GUID
                '([A-Z0-9]{8}-[A-Z0-9]{4}-[A-Z0-9]{4}-[A-Z0-9]{4}-[A-Z0-9]{12})',
                re.IGNORECASE | re.DOTALL)

Also, you should not have an outer group on that regex.

Your else after return guid1 is redundant, since the previous block has already returned.

lines = dict() should be lines = {}. There's a reason for this: generally, shorter code is considered more "Pythonic" (as long as it doesn't become arcane or difficult to understand). A dictionary literal is short, simple, and easy to understand.

Here:

if str(element) in line:

You've already stated in the function doc that element is assumed to be a string, so you don't need to convert it using str.

Here:

unique_ids_dict[element] = find_device_IDs(path, element)

Your data structure use is a little strange. You're constructing a dictionary by element of single-key dictionaries by element of GUIDs. It would probably make more sense to change find_device_IDs such that it doesn't return a dictionary, and instead just returns a set.

Speed tips:

  • Sort out your data structure strategy, mentioned above
  • Don't re-compile your regex every time an inner function is called. Compile your regex once, at the global level

This:

num_exceptions_dict = dict()
    for elem in search_list:
        num_exceptions_dict[elem] = find_num_occurences(path, elem, 
            search_with_in_deviceID, list(unique_ids_dict[elem].values())[0])

should probably be reduced to something like

num_exceptions_dict = {
    elem: find_num_occurences(path, elem, search_with_in_deviceID,
                              unique_ids_dict[elem].values())
    for elem in search_list
}

The performance-affecting change is to avoid re-constructing an inner list. But there's something more sinister going on there. A dictionary is unordered, but you're taking element [0] of an arbitrarily ordered list of values(). If you actually do intend to pull a non-deterministic value from the dictionary, then fine; but this is probably not your intention.

This:

search_with_in_deviceID = ["Exception occurred", "Packet record has been added"]

should be a tuple instead of a list, since you aren't changing it; so:

search_within_device_ID = ('Exception occurred', 'Packet record has been added')

I'm going to suggest that you attempt to address the issues that I've raised, above, in particular with respect to the structure of your dictionary and the non-deterministic behaviour of your values call; and then submit a new question so that further performance issues may be addressed.

\$\endgroup\$
7
  • \$\begingroup\$ Yes, about the last suggestion. That was something I missed. And about str(element), yes this too should have been updated. But about lines = {} instead of lines = dict() . I decided to use this because I thought it provided more clarity when one speed reads. Could you let me know how your's would be better. Same goes for re1 and re2. Better when speed reading. Thank you for suggesting timeit. Also, how can I make this run faster? \$\endgroup\$ Commented Dec 23, 2018 at 15:44
  • 1
    \$\begingroup\$ Even if re1 and re2 were to stay separate for the purposes of legibility, they have bad names, and you should rename them to something like re_prefix and re_guid - otherwise, legibility doesn't increase. \$\endgroup\$
    – Reinderien
    Commented Dec 23, 2018 at 15:46
  • \$\begingroup\$ Got it! Makes sense. Thank you. How about dict()? I'm guessing they are almost the same level of legibility. Again, any suggestion to make it go faster? \$\endgroup\$ Commented Dec 23, 2018 at 15:49
  • \$\begingroup\$ Please see edits. \$\endgroup\$
    – Reinderien
    Commented Dec 23, 2018 at 16:28
  • 1
    \$\begingroup\$ I've made improvements and reposted here codereview.stackexchange.com/questions/21040. \$\endgroup\$ Commented Dec 27, 2018 at 6:20

Your Answer

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

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