Issue 2983 - Make sure that the new lastVersion format is recognized and exceptions don`t break eve…

sitescripts/stats/bin/logprocessor.py

 # coding: utf-8
 
 # This file is part of the Adblock Plus web scripts,
 # Copyright (C) 2006-2015 Eyeo GmbH
 #
 # Adblock Plus is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3 as
 # published by the Free Software Foundation.
 #
 # Adblock Plus is distributed in the hope that it will be useful,
@@ skipping 99 matching lines @@
 
   def wrapped(arg):
     if arg in results:
       result = results[arg]
       del results[arg]
     else:
       if results.entries_left > 0:
         results.entries_left -= 1
       else:
         results.popitem(last=False)
-      result = func(arg)
+      try:
+        result = func(arg)
+      except:

[Sebastian Noack, 2015/10/21 15:19:20]

You probably should explicitly catch Exception. Otherwise that code will catch
SystemExit and KeyboardInterrupt as well.

[Wladimir Palant, 2015/10/21 15:42:40]

Why shouldn't it? The exception is re-raised, the point is merely keeping the
cache in a consistent state. Whether some exception will cause the interpreter
to shut down is irrelevant for that.

[Sebastian Noack, 2015/10/21 15:49:13]

Acknowledged.

+        results.entries_left += 1
+        raise
     results[arg] = result
     return result
   return wrapped
 
 
 def cache_last(func):
   """
     Decorator that memoizes the last return value of a function in case it is
     called again with the same parameters.
   """
@@ skipping 129 matching lines @@
   except:
     path = urlparts.path
   return path[1:], urlparts.query
 
 @cache_lru
 def parse_query(query):
   return urlparse.parse_qs(query)
 
 @cache_lru
 def parse_lastversion(last_version):
+  if '-' in last_version:
+    last_version = last_version.split('-', 1)[0]

[Sebastian Noack, 2015/10/21 15:19:20]

Nit: last_version.split('-')[0] would do as will and is slightly simpler.

[Wladimir Palant, 2015/10/21 15:42:40]

Sure, but the point is doing as little work as possible here.

[Sebastian Noack, 2015/10/21 15:49:13]

More than one dash given is a rather theoretical scenario, we don't have to
optimize for, right? Because if there is only one dash given, split() is even
faster when omitting the second argument:

In [3]: %timeit '0-1'.split('-')
1000000 loops, best of 3: 180 ns per loop

In [4]: %timeit '0-1'.split('-', 1)
1000000 loops, best of 3: 192 ns per loop

[Sebastian Noack, 2015/10/21 16:10:34]

I did some more benchmarking, and confirmed that the |'-' in last_version| check
is indeed giving some notable speedups if there is no dash. However, if the
common case is a dash being given, omitting that check is obviously faster (and
also simpler):

In [10]: x = '0-1'

In [11]: %timeit x.split(x, 1)[0] if '-' in x else x
1000000 loops, best of 3: 243 ns per loop

In [12]: %timeit x.split(x)[0]
1000000 loops, best of 3: 204 ns per loop

In [13]: x = '0'

In [14]: %timeit x.split(x, 1)[0] if '-' in x else x
10000000 loops, best of 3: 38.1 ns per loop

In [15]: %timeit x.split(x)[0]
1000000 loops, best of 3: 197 ns per loop

[Wladimir Palant, 2015/10/21 16:16:03]

No, it's a very real scenario if more than one notification is active at a time.
As is the "no dash" scenario (no notifications active).

Either way, should we really argue about microseconds? The point is that we only
need one split here.

[Sebastian Noack, 2015/10/21 16:19:51]

Well, I'm not arguing. I just try to understand whether the added complexity is
worth the few microseconds you save in these scenarios. LGTM.

   return datetime.strptime(last_version, "%Y%m%d%H%M")
 
 @cache_lru
 def get_week(date):
   return date.isocalendar()[0:2]
 
 def parse_downloader_query(info):
   params = parse_query(info["query"])
   for param in ("addonName", "addonVersion", "application", "applicationVersion", "platform", "platformVersion"):
     info[param] = params.get(param, ["unknown"])[0]
@@ skipping 284 matching lines @@
   parser.add_argument("mirror_name", nargs="?", help="Name of the mirror server that the file belongs to")
   parser.add_argument("server_type", nargs="?", help="Server type like download, update or subscription")
   parser.add_argument("log_file", nargs="?", help="Log file path, can be a local file path, http:// or ssh:// URL")
   args = parser.parse_args()
 
   if args.mirror_name and args.server_type and args.log_file:
     sources = [(args.mirror_name, args.server_type, args.log_file)]
   else:
     sources = get_stats_files()
   parse_sources(sources, args.factor, args.verbose)