OrderedDict iterator allocates di_result unnecessarily

[KevinShweh]

BPO	46085
Nosy	@rhettinger, @methane, @ericsnowcurrently, @corona10, @miss-islington
PRs	bpo-46085: Fix iterator cache mechanism of OrderedDict. #30290 [3.10] bpo-46085: Fix iterator cache mechanism of OrderedDict. (GH-30290) #30299 [3.9] bpo-46085: Fix iterator cache mechanism of OrderedDict. (GH-30290) #30300

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2021-12-30.05:29:44.368>
created_at = <Date 2021-12-15.16:48:00.598>
labels = ['3.11', 'library', '3.9', '3.10', 'performance']
title = 'OrderedDict iterator allocates di_result unnecessarily'
updated_at = <Date 2021-12-30.05:29:44.367>
user = 'https://bugs.python.org/KevinShweh'

bugs.python.org fields:

activity = <Date 2021-12-30.05:29:44.367>
actor = 'corona10'
assignee = 'none'
closed = True
closed_date = <Date 2021-12-30.05:29:44.368>
closer = 'corona10'
components = ['Library (Lib)']
creation = <Date 2021-12-15.16:48:00.598>
creator = 'Kevin Shweh'
dependencies = []
files = []
hgrepos = []
issue_num = 46085
keywords = ['patch']
message_count = 7.0
messages = ['408616', '408670', '408728', '409345', '409347', '409348', '409349']
nosy_count = 6.0
nosy_names = ['rhettinger', 'methane', 'eric.snow', 'Kevin Shweh', 'corona10', 'miss-islington']
pr_nums = ['30290', '30299', '30300']
priority = 'normal'
resolution = None
stage = 'resolved'
status = 'closed'
superseder = None
type = 'resource usage'
url = 'https://bugs.python.org/issue46085'
versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

[KevinShweh]

The OrderedDict iterator caches a di_result tuple for use with iter(od.items()). It's *supposed* to only do that for the items() case, but the code does

if (kind & (_odict_ITER_KEYS | _odict_ITER_VALUES))

to test for this case. This is the wrong test. It should be

if ((kind & _odict_ITER_KEYS) && (kind &_odict_ITER_VALUES))

The current test allocates di_result for key and value iterators as well as items iterators.

[methane]

Nice catch.

if ((kind & \_odict_ITER_KEYS) && (kind &_odict_ITER_VALUES))

You can reduce one branch by

#define _odict_ITER_ITEMS (_odict_ITER_KEYS|_odict_ITER_VALUES)
...
     if (kind & _odict_ITER_ITEMS == _odict_ITER_ITEMS)

[KevinShweh]

Almost - C's weird bitwise operator precedence means it has to be parenthesized as

if ((kind & _odict_ITER_ITEMS) == _odict_ITER_ITEMS)

[corona10]

New changeset fb44d05 by Dong-hee Na in branch 'main':
bpo-46085: Fix iterator cache mechanism of OrderedDict. (GH-30290)
fb44d05

[miss-islington]

New changeset 1b37268 by Miss Islington (bot) in branch '3.10':
bpo-46085: Fix iterator cache mechanism of OrderedDict. (GH-30290)
1b37268

[miss-islington]

New changeset 2d4049d by Miss Islington (bot) in branch '3.9':
bpo-46085: Fix iterator cache mechanism of OrderedDict. (GH-30290)
2d4049d

[corona10]

Thanks for reporting Kevin!