Issue 29329315: Issue 2983 - Make sure that the new lastVersion format is recognized and exceptions don`t break eve… (Closed)
|
Created:
Oct. 21, 2015, 9:53 a.m. by Wladimir Palant Modified:
Oct. 22, 2015, 1 p.m. Visibility:
Public. |
DescriptionIssue 2983 - Make sure that the new lastVersion format is recognized and exceptions don`t break eve…
Patch Set 1 #
Total comments: 9
MessagesTotal messages: 8
https://codereview.adblockplus.org/29329315/diff/29329316/sitescripts/stats/b... File sitescripts/stats/bin/logprocessor.py (right): https://codereview.adblockplus.org/29329315/diff/29329316/sitescripts/stats/b... sitescripts/stats/bin/logprocessor.py:122: except: You probably should explicitly catch Exception. Otherwise that code will catch SystemExit and KeyboardInterrupt as well. https://codereview.adblockplus.org/29329315/diff/29329316/sitescripts/stats/b... sitescripts/stats/bin/logprocessor.py:275: last_version = last_version.split('-', 1)[0] Nit: last_version.split('-')[0] would do as will and is slightly simpler.
https://codereview.adblockplus.org/29329315/diff/29329316/sitescripts/stats/b... File sitescripts/stats/bin/logprocessor.py (right): https://codereview.adblockplus.org/29329315/diff/29329316/sitescripts/stats/b... sitescripts/stats/bin/logprocessor.py:122: except: On 2015/10/21 15:19:20, Sebastian Noack wrote: > You probably should explicitly catch Exception. Otherwise that code will catch > SystemExit and KeyboardInterrupt as well. 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. https://codereview.adblockplus.org/29329315/diff/29329316/sitescripts/stats/b... sitescripts/stats/bin/logprocessor.py:275: last_version = last_version.split('-', 1)[0] On 2015/10/21 15:19:20, Sebastian Noack wrote: > Nit: last_version.split('-')[0] would do as will and is slightly simpler. Sure, but the point is doing as little work as possible here.
https://codereview.adblockplus.org/29329315/diff/29329316/sitescripts/stats/b... File sitescripts/stats/bin/logprocessor.py (right): https://codereview.adblockplus.org/29329315/diff/29329316/sitescripts/stats/b... sitescripts/stats/bin/logprocessor.py:122: except: On 2015/10/21 15:42:40, Wladimir Palant wrote: > On 2015/10/21 15:19:20, Sebastian Noack wrote: > > You probably should explicitly catch Exception. Otherwise that code will catch > > SystemExit and KeyboardInterrupt as well. > > 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. Acknowledged. https://codereview.adblockplus.org/29329315/diff/29329316/sitescripts/stats/b... sitescripts/stats/bin/logprocessor.py:275: last_version = last_version.split('-', 1)[0] On 2015/10/21 15:42:40, Wladimir Palant wrote: > On 2015/10/21 15:19:20, Sebastian Noack wrote: > > Nit: last_version.split('-')[0] would do as will and is slightly simpler. > > Sure, but the point is doing as little work as possible here. 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
https://codereview.adblockplus.org/29329315/diff/29329316/sitescripts/stats/b... File sitescripts/stats/bin/logprocessor.py (right): https://codereview.adblockplus.org/29329315/diff/29329316/sitescripts/stats/b... sitescripts/stats/bin/logprocessor.py:275: last_version = last_version.split('-', 1)[0] On 2015/10/21 15:49:13, Sebastian Noack wrote: > On 2015/10/21 15:42:40, Wladimir Palant wrote: > > On 2015/10/21 15:19:20, Sebastian Noack wrote: > > > Nit: last_version.split('-')[0] would do as will and is slightly simpler. > > > > Sure, but the point is doing as little work as possible here. > > 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 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
https://codereview.adblockplus.org/29329315/diff/29329316/sitescripts/stats/b... File sitescripts/stats/bin/logprocessor.py (right): https://codereview.adblockplus.org/29329315/diff/29329316/sitescripts/stats/b... sitescripts/stats/bin/logprocessor.py:275: last_version = last_version.split('-', 1)[0] On 2015/10/21 15:49:13, Sebastian Noack wrote: > More than one dash given is a rather theoretical scenario, we don't have to > optimize for, right? 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.
https://codereview.adblockplus.org/29329315/diff/29329316/sitescripts/stats/b... File sitescripts/stats/bin/logprocessor.py (right): https://codereview.adblockplus.org/29329315/diff/29329316/sitescripts/stats/b... sitescripts/stats/bin/logprocessor.py:275: last_version = last_version.split('-', 1)[0] On 2015/10/21 16:16:03, Wladimir Palant wrote: > On 2015/10/21 15:49:13, Sebastian Noack wrote: > > More than one dash given is a rather theoretical scenario, we don't have to > > optimize for, right? > > 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. 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.
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
