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

Unintentional hard reference assignment in Python version of OrderedDict.move_to_end #73305

Closed
rhettinger opened this issue Dec 31, 2016 · 8 comments
Closed

Unintentional hard reference assignment in Python version of OrderedDict.move_to_end #73305

rhettinger opened this issue Dec 31, 2016 · 8 comments
Labels
3.7 (EOL) end of life performance Performance or resource usage stdlib Python modules in the Lib dir

Comments

@rhettinger
Copy link
Contributor

BPO 29119
Nosy @rhettinger, @serhiy-storchaka, @andrabogildea
Files
  • od_proxy.diff: Technique 1: Reuse existing weakref
  • od_proxy2.diff: Technique 2: Create a new proxy
  • od_proxy2a.diff: Revised version of Technique 2 patch
    		•

    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 2016-12-31.19:09:38.404>
created_at = <Date 2016-12-31.06:11:43.002>
labels = ['3.7', 'library', 'performance']
title = 'Unintentional hard reference assignment in Python version of OrderedDict.move_to_end'
updated_at = <Date 2016-12-31.19:09:38.403>
user = 'https://github.com/rhettinger'

    bugs.python.org fields:

    activity = <Date 2016-12-31.19:09:38.403>
actor = 'rhettinger'
assignee = 'none'
closed = True
closed_date = <Date 2016-12-31.19:09:38.404>
closer = 'rhettinger'
components = ['Library (Lib)']
creation = <Date 2016-12-31.06:11:43.002>
creator = 'rhettinger'
dependencies = []
files = ['46097', '46098', '46099']
hgrepos = []
issue_num = 29119
keywords = ['patch']
message_count = 8.0
messages = ['284367', '284368', '284375', '284377', '284389', '284410', '284411', '284412']
nosy_count = 4.0
nosy_names = ['rhettinger', 'python-dev', 'serhiy.storchaka', 'andrabogildea']
pr_nums = []
priority = 'low'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = 'resource usage'
url = 'https://bugs.python.org/issue29119'
versions = ['Python 3.5', 'Python 3.6', 'Python 3.7']
    @rhettinger
    Copy link
    Contributor Author

    The root.prev and first.prev assignments should use weak references rather than hard references. Spotted by Andra Bogildea.

    @rhettinger rhettinger added 3.7 (EOL) end of life stdlib Python modules in the Lib dir performance Performance or resource usage labels Dec 31, 2016
    @serhiy-storchaka
    Copy link
    Member

    Makes sense. Any chance to create tests?

    @rhettinger
    Copy link
    Contributor Author

    I'm leaving this for Andra Bogildea to work through and comment on.

    Testing is likely to be awkward at best. It would involve white box implementation specific steps like accessing the name managed variables and rotating through the doubly linked list to verify that all the prev-links have the type, <class 'weakproxy'>.

    @serhiy-storchaka
    Copy link
    Member

    od_proxy2a.diff LGTM.

    @andrabogildea
    Copy link
    Mannequin

    andrabogildea mannequin commented Dec 31, 2016

    od_proxy.diff and od_proxy2a.diff are the same. Did you meant soft_link = proxy(link) in od_proxy2a.diff?

    I believe Technique 1 (od_proxy.diff) is preferable, because it reuses the proxy object.

    Regarding automated tests, I agree with what’s been said. The usage of weak references is an internal detail (and name mangling makes it even more inconvenient).

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 31, 2016

    New changeset 19376765d7c3 by Raymond Hettinger in branch '3.5':
    Issue bpo-29119: Fix weakref in OrderedDict.move_to_end(). Work by Andra Bogildea.
    https://hg.python.org/cpython/rev/19376765d7c3

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 31, 2016

    New changeset cd811b867393 by Raymond Hettinger in branch '3.6':
    Issue bpo-29119: Merge in ACK from 3.5 branch
    https://hg.python.org/cpython/rev/cd811b867393

    @rhettinger
    Copy link
    Contributor Author

    Nice work Andra. Thank you.

    @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.7 (EOL) end of life performance Performance or resource usage stdlib Python modules in the Lib dir
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    2 participants
    @rhettinger @serhiy-storchaka