Skip to content
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

Jump to bottom

OrderedDict iterator allocates di_result unnecessarily #90243

Closed
KevinShweh mannequin opened this issue Dec 15, 2021 · 7 comments
Closed

OrderedDict iterator allocates di_result unnecessarily #90243

KevinShweh mannequin opened this issue Dec 15, 2021 · 7 comments
Labels
3.9 only security fixes 3.10 only security fixes 3.11 only security fixes performance Performance or resource usage stdlib Python modules in the Lib dir

Comments

@KevinShweh
Copy link
Mannequin

KevinShweh mannequin commented Dec 15, 2021

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
    Copy link
    Mannequin Author

    KevinShweh mannequin commented Dec 15, 2021

    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.

    @KevinShweh KevinShweh mannequin added 3.8 only security fixes 3.10 only security fixes 3.11 only security fixes 3.9 only security fixes stdlib Python modules in the Lib dir performance Performance or resource usage labels Dec 15, 2021
    @AlexWaygood AlexWaygood removed 3.8 only security fixes labels Dec 15, 2021
    @methane
    Copy link
    Member

    methane commented Dec 16, 2021

    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
    Copy link
    Mannequin Author

    KevinShweh mannequin commented Dec 16, 2021

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

    if ((kind & _odict_ITER_ITEMS) == _odict_ITER_ITEMS)

    @corona10
    Copy link
    Member

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

    @miss-islington
    Copy link
    Contributor

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

    @miss-islington
    Copy link
    Contributor

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

    @corona10
    Copy link
    Member

    Thanks for reporting Kevin!

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    3.9 only security fixes 3.10 only security fixes 3.11 only security fixes performance Performance or resource usage stdlib Python modules in the Lib dir
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    4 participants
    @methane @corona10 @miss-islington @AlexWaygood