Skip to content

gh-116621: Specialize list.extend for dict keys/values #116816

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
merged 2 commits into from
Mar 15, 2024
Merged

gh-116621: Specialize list.extend for dict keys/values #116816

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7f22f58
gh-116621: Specialize list.extend for dict keys/values
corona10 Mar 14, 2024
22e889d
Address code review
corona10 Mar 15, 2024
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
16 changes: 12 additions & 4 deletions Lib/test/test_ordered_dict.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -568,14 +568,22 @@ def __hash__(self):
od[key] = i

# These should not crash.
with self.assertRaises(KeyError):
try:
Copy link
Member Author

@corona10 corona10 Mar 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@colesbury
I noticed that this change has some behavior changes.(in nogil 3.12, it has same behavior changes)
I'm not sure it's worth applying it.
See: colesbury/nogil-3.12@da071fa also

Let me check a microbenchmark instead.

Copy link
Member Author

@corona10 corona10 Mar 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

From my local benchmark, for the default build, it makes 2% slower.
(Or we can handle it as a noise)

(.venv) ➜  cpython git:(gh-116621-more) ✗ python -m pyperf compare_to base.json opt.json
Mean +- std dev: [base] 36.3 us +- 0.7 us -> [opt] 36.8 us +- 0.6 us: 1.02x slower

import pyperf

runner = pyperf.Runner()
runner.timeit(name="bench list.extend keys",
              stmt="""
keys = list(d.keys())
""",
              setup = """
d = dict()
for k in range(10000):
    d[k] = k
"""
              )

Copy link
Member Author

@corona10 corona10 Mar 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There are a few other cases in nogil-3.12 that we probably want to handle atomically as well, because I think we use the same pattern where the calling code assumes list(some_type) is atomic for certain built-in types:

When considering your suggestion, IMO, we should apply this technique to the free-threaded build only, WDYT?
And please let me know if this is not a fair benchmark.

Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding the OrderedDict changes: let's check for the exact type PyDictKeys_Type and PyDictValues_Type. I'm not sure using PyDict_Next is always appropriate for subclasses.

On the benchmark, I think that's just noise. I was not able to reproduce that locally. Avoiding the iterator creation in the specialized case is going to be faster when the dict has few items.

list(od.values())
with self.assertRaises(KeyError):
except KeyError:
pass
try:
list(od.items())
with self.assertRaises(KeyError):
except KeyError:
pass
try:
repr(od)
with self.assertRaises(KeyError):
except KeyError:
pass
try:
od.copy()
except KeyError:
pass

def test_issue24348(self):
OrderedDict = self.OrderedDict
Expand Down
37 changes: 37 additions & 0 deletions Objects/listobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#include "Python.h"
#include "pycore_abstract.h" // _PyIndex_Check()
#include "pycore_ceval.h" // _PyEval_GetBuiltin()
#include "pycore_dict.h" // _PyDictViewObject
#include "pycore_pyatomic_ft_wrappers.h"
#include "pycore_interp.h" // PyInterpreterState.list
#include "pycore_list.h" // struct _Py_list_freelist, _PyListIterObject
Expand Down Expand Up @@ -1295,6 +1296,30 @@ list_extend_set(PyListObject *self, PySetObject *other)
return 0;
}

static int
list_extend_dict(PyListObject *self, PyDictObject *dict, int which_item)
{
// which_item: 0 for keys and 1 for values
Py_ssize_t m = Py_SIZE(self);
Py_ssize_t n = PyDict_GET_SIZE(dict);
if (list_resize(self, m + n) < 0) {
return -1;
}

PyObject **dest = self->ob_item + m;
Py_ssize_t pos = 0;
PyObject *keyvalue[2];
while (PyDict_Next((PyObject *)dict, &pos, &keyvalue[0], &keyvalue[1])) {
PyObject *obj = keyvalue[which_item];
Py_INCREF(obj);
*dest = obj;
dest++;
}

Py_SET_SIZE(self, m + n);
return 0;
}

static int
_list_extend(PyListObject *self, PyObject *iterable)
{
Expand Down Expand Up @@ -1322,6 +1347,18 @@ _list_extend(PyListObject *self, PyObject *iterable)
res = list_extend_set(self, (PySetObject *)iterable);
Py_END_CRITICAL_SECTION2();
}
else if (PyDictKeys_Check(iterable)) {
PyDictObject *dict = ((_PyDictViewObject *)iterable)->dv_dict;
Py_BEGIN_CRITICAL_SECTION2(self, dict);
res = list_extend_dict(self, dict, 0 /*keys*/);
Py_END_CRITICAL_SECTION2();
}
else if (PyDictValues_Check(iterable)) {
PyDictObject *dict = ((_PyDictViewObject *)iterable)->dv_dict;
Py_BEGIN_CRITICAL_SECTION2(self, dict);
res = list_extend_dict(self, dict, 1 /*values*/);
Py_END_CRITICAL_SECTION2();
}
else {
Py_BEGIN_CRITICAL_SECTION(self);
res = list_extend_iter_lock_held(self, iterable);
Expand Down