Skip to content

bpo-35664: Optimize operator.itemgetter #11435

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rhettinger merged 6 commits into from
Jan 7, 2019
Merged

bpo-35664: Optimize operator.itemgetter #11435

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9f1e266
Operator argument parsing for the normal case
rhettinger Jan 4, 2019
c014a05
Fast path for tuples with non-negative integer indicies
rhettinger Jan 4, 2019
d00fd55
Add blurb
rhettinger Jan 5, 2019
e9e2fa9
PEP 7 nits
rhettinger Jan 6, 2019
e7c33f1
Add more tests
rhettinger Jan 6, 2019
d70baef
Expand comment since the review didn't understand previous comment
rhettinger Jan 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions Lib/test/test_operator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,19 @@ def __getitem__(self, name):
self.assertEqual(operator.itemgetter(2,10,5)(data), ('2', '10', '5'))
self.assertRaises(TypeError, operator.itemgetter(2, 'x', 5), data)

# interesting indices
t = tuple('abcde')
self.assertEqual(operator.itemgetter(-1)(t), 'e')
self.assertEqual(operator.itemgetter(slice(2, 4))(t), ('c', 'd'))

# interesting sequences
class T(tuple):
'Tuple subclass'
pass
self.assertEqual(operator.itemgetter(0)(T('abc')), 'a')
self.assertEqual(operator.itemgetter(0)(['a', 'b', 'c']), 'a')
self.assertEqual(operator.itemgetter(0)(range(100, 200)), 100)

def test_methodcaller(self):
operator = self.module
self.assertRaises(TypeError, operator.methodcaller)
Expand Down
4 changes: 4 additions & 0 deletions Misc/NEWS.d/next/Library/2019-01-04-22-18-25.bpo-35664.Z-Gyyj.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Improve operator.itemgetter() performance by 33% with optimized argument
handling and with adding a fast path for the common case of a single
non-negative integer index into a tuple (which is the typical use case in
the standard library).
42 changes: 37 additions & 5 deletions Modules/_operator.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -937,6 +937,7 @@ typedef struct {
PyObject_HEAD
Py_ssize_t nitems;
PyObject *item;
Py_ssize_t index; // -1 unless *item* is a single non-negative integer index
} itemgetterobject;

static PyTypeObject itemgetter_type;
Expand All @@ -948,6 +949,7 @@ itemgetter_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
itemgetterobject *ig;
PyObject *item;
Py_ssize_t nitems;
Py_ssize_t index;

if (!_PyArg_NoKeywords("itemgetter", kwds))
return NULL;
Expand All @@ -967,6 +969,21 @@ itemgetter_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
Py_INCREF(item);
ig->item = item;
ig->nitems = nitems;
ig->index = -1;
if (PyLong_CheckExact(item)) {
index = PyLong_AsSsize_t(item);
if (index < 0) {
/* If we get here, then either the index conversion failed
* due to being out of range, or the index was a negative
* integer. Either way, we clear any possible exception
* and fall back to the slow path, where ig->index is -1.
*/
PyErr_Clear();
}
else {
ig->index = index;
}
}

PyObject_GC_Track(ig);
return (PyObject *)ig;
Expand All @@ -993,12 +1010,27 @@ itemgetter_call(itemgetterobject *ig, PyObject *args, PyObject *kw)
PyObject *obj, *result;
Py_ssize_t i, nitems=ig->nitems;

if (!_PyArg_NoKeywords("itemgetter", kw))
return NULL;
if (!PyArg_UnpackTuple(args, "itemgetter", 1, 1, &obj))
return NULL;
if (nitems == 1)
assert(PyTuple_CheckExact(args));
if (kw == NULL && PyTuple_GET_SIZE(args) == 1) {
Copy link
Copy Markdown
Member

@serhiy-storchaka serhiy-storchaka Jan 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_PyArg_NoKeywords is a macro, no need to "optimize" it manually.

Copy link
Copy Markdown
Contributor Author

@rhettinger rhettinger Jan 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer this as-is. There is some tiny duplication which gets be compiled away, but to my eyes the kw==NULL check makes it clearer what the common path is and that no external calls are made. Also, I don't trust that _PyArg_NoKeywords will remain a macro.

Copy link
Copy Markdown
Member

@serhiy-storchaka serhiy-storchaka Jan 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_PyArg_NoKeywords() makes clear the purpose of code.

And please add comments why these special cases is checked.

obj = PyTuple_GET_ITEM(args, 0);
}
else {
if (!_PyArg_NoKeywords("itemgetter", kw))
return NULL;
if (!PyArg_UnpackTuple(args, "itemgetter", 1, 1, &obj))
return NULL;
}
if (nitems == 1) {
if (ig->index >= 0
&& PyTuple_CheckExact(obj)
&& ig->index < PyTuple_GET_SIZE(obj))
{
result = PyTuple_GET_ITEM(obj, ig->index);
Py_INCREF(result);
return result;
}
return PyObject_GetItem(obj, ig->item);
}

assert(PyTuple_Check(ig->item));
assert(PyTuple_GET_SIZE(ig->item) == nitems);
Expand Down