CODEWITHSUNDEEP
pythonoptimizationpython-3.xpyqtpyqt4
warvariuc
warvariuc

Reputation: 59664

Optimizing `in`

I am using such constructs to test whether a needed key is pressed:

def eventFilter(self, tableView, event):
    if event.type() == QtCore.QEvent.KeyPress:
        key = event.key()
        if event.modifiers() in (QtCore.Qt.NoModifier, QtCore.Qt.KeypadModifier):
            if key in (QtCore.Qt.Key_Enter, QtCore.Qt.Key_Return):
                self.menu.editItem.trigger()
                return True

I know that 'Premature optimization is the root of all evil', but i think eventFilter is called quite often to think about its optimization.

My concerns:

  1. if key in (QtCore.Qt.Key_Enter, QtCore.Qt.Key_Return) upon each run makes double lookup: 1. Find Qt attribute in QtCore module; 2. Find Key_Enter attribute in Qt module.
  2. if key in (QtCore.Qt.Key_Enter, QtCore.Qt.Key_Return) this constructs the tuple upon each run. The search in the tuple is sequential - better to use frozenset?

How do you deal with such cases? Don't care?

Upvotes: 2

Views: 299

Answers (4)

PaulMcG
PaulMcG

Reputation: 63747

Your code:

def eventFilter(self, tableView, event): 
    if event.type() == QtCore.QEvent.KeyPress: 
        key = event.key() 
        if event.modifiers() in (QtCore.Qt.NoModifier, QtCore.Qt.KeypadModifier): 
            if key in (QtCore.Qt.Key_Enter, QtCore.Qt.Key_Return): 
                self.menu.editItem.trigger() 
                return True

As I mentioned in a comment to @interjay, there may be a multitude of calls to this function for any kind of UI event, and if you have many of these filters, they could make for a sluggish UI. If you wanted to optimize this at least as far as the first if test, then move the local definition of QtCore.QEvent.KeyPress into a default argument value:

def eventFilter(self, tableView, event,
    FILTER_EVENT_TYPE=QtCore.QEvent.KeyPress
    ): 
    if event.type() == FILTER_EVENT_TYPE: 
        if event.modifiers() in (QtCore.Qt.NoModifier, QtCore.Qt.KeypadModifier): 
            key = event.key() 
            if key in (QtCore.Qt.Key_Enter, QtCore.Qt.Key_Return): 
                self.menu.editItem.trigger() 
                return True

(I also moved the function call to event.key() to after the test on event.modifiers().)

Default arguments like this are evaluated once at function compile time when the module is imported, not once per call, so your lookups for QtCore.QEvent.KeyPress will be accelerated. You could of course take this to the extreme of:

def eventFilter(self, tableView, event,
    FILTER_EVENT_TYPE=QtCore.QEvent.KeyPress
    FILTER_MODIFIERS=(QtCore.Qt.NoModifier, QtCore.Qt.KeypadModifier),
    FILTER_KEYS=(QtCore.Qt.Key_Enter, QtCore.Qt.Key_Return)
    ): 
    if (event.type() == FILTER_EVENT_TYPE and
        event.modifiers() in FILTER_MODIFIERS and 
        event.key() in FILTER_KEYS): 
            self.menu.editItem.trigger() 
            return True

Now you have optimized not only the module-object-attribute lookups, but also the tuple constructions, and as @AndrewDalke mentions, my testing of in shows it is faster for tuples than sets up to a size of about 3 or 4 elements. The single condition will still short-circuit when any part of the condition fails, so you won't get calls to event.modifiers or event.key if the type is not a keypress.

EDIT: I like @ekhumoro's coupled testing of key and modifier, here's how it would look merged into my code:

def eventFilter(self, tableView, event,
    FILTER_EVENT_TYPE=QtCore.QEvent.KeyPress
    FILTER_KEY_MODIFIERS=((QtCore.Qt.Key_Return, QtCore.Qt.NoModifier),
                          (QtCore.Qt.Key_Enter, QtCore.Qt.KeypadModifier),
                          )
    ): 
    if (event.type() == FILTER_EVENT_TYPE and
        (event.key(), event.modifiers()) in FILTER_KEY_MODIFIERS): 
            self.menu.editItem.trigger() 
            return True

Upvotes: 3

Tim Delaney
Tim Delaney

Reputation: 5605

Echo the "don't bother". But there are a couple of things you should be aware of.

Firstly, and most importantly, tuple literals in a function are constructed once, when the function itself is compiled, and stored as a constant. So you do not have the case you're asking about - constantly reconstructing the tuple (and accompanying lookups).

Secondly, binding QtCore.QEvent.KeyPress to a module-level global would benefit you (a small amount). Currently you have a module-level lookup for QtCore, followed by a module-level lookup for QEvent, followed (I think) by a module-level lookup for KeyPress (it might be class-level, but that's similar cost in most classes). By doing something like:

QtKeyPress = QtCore.QEvent.KeyPress

or

from QtCore.QEvent import KeyPress as QtKeyPress

you will reduce that to a single module-level lookup. You could reduce it to a local lookup using the default parameter trick, but that has disadvantages as well - ugliness of code, and the possibility of someone overriding the binding at function call time.

Upvotes: 0

ekhumoro
ekhumoro

Reputation: 120678

Whilst it's true that multiple attribute lookups will be slower, since we're talking about less than a millionth of a second per lookup, such differences are going to be swamped by other much larger factors (such as the cost of simply calling a method). So this would be more a case of pointless optimization than premature optimization.

However, if you're really concerned about it, a lot of the lookups can be avoided by simply changing your import statements.

So instead of doing:

from PyQt4 import QtCore

you can do:

from PyQt4.QtCore import QEvent

and avoid the extra attribute lookup for every QEvent reference within the module (and of course the same thing goes for any other class imported in this way).

Personally, I would also avoid all the in tests, and instead test each possibility individually. This saves the cost of creating a tuple every time the test is run, and also takes advantage of short-circuit evaluation.

So I would re-write your example code as:

from PyQt4.QtCore import Qt, QEvent

def eventFilter(self, tableView, event):
    if event.type() == QEvent.KeyPress:
        key = event.key()
        modifiers = event.modifiers()
        if ((modifiers == Qt.NoModifier and key == Qt.Key_Return) or 
            (modifiers == Qt.KeypadModifier and key == Qt.Key_Enter)):
            self.menu.editItem.trigger()
            return True

Upvotes: 1

Martin Geisler
Martin Geisler

Reputation: 73788

I agree with the comments that it wont matter in this case.

But if you want to save time here, then the canonical way to remove the cost of repeated lookups in Python is to cache the objects in the local variable namespace:

NoModifier = QtCore.Qt.NoModifier
KeypadModifier = QtCore.Qt.KeypadModifier
if event.modifiers() in (NoModifier, KeypadModifier):
    ...

Python will lookup variables in the local variables namespace, the module namespace, and finally in the global namespace — you can thus win something by putting things in the local variable namespace.

However, this does not make sense in your case: there lookups are done once per function call. The above optimization stragegy applies if you have a loop where you do many, many lookups of the same attributes:

for event in huge_pile_of_accumulated_events:
    if event.modifiers() in (NoModifier, KeypadModifier):
        ...

There you might save something — remember to profile your code first to actually prove that this matters! For a handler that's run once per keyboard event, the lookup time wont matter.

Upvotes: 2

Related Questions