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.

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

Support key-sharing dictionaries in subclasses #64836

Closed
pingebretson mannequin opened this issue Feb 15, 2014 · 10 comments
Closed

Support key-sharing dictionaries in subclasses #64836

pingebretson mannequin opened this issue Feb 15, 2014 · 10 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage

Comments

@pingebretson
Copy link
Mannequin

pingebretson mannequin commented Feb 15, 2014

BPO 20637
Nosy @pitrou, @vstinner, @larryhastings, @benjaminp, @Trundle, @markshannon
Files
  • subclass-keys-pep-412.patch: Patch to support key-sharing dictionaries in subclasses
  • subclass-keys-pep-412.patch: Revised patch based on review feedback
  • 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 2014-02-23.15:52:07.847>
    created_at = <Date 2014-02-15.22:49:04.178>
    labels = ['interpreter-core', 'performance']
    title = 'Support key-sharing dictionaries in subclasses'
    updated_at = <Date 2014-03-17.21:00:36.811>
    user = 'https://bugs.python.org/pingebretson'

    bugs.python.org fields:

    activity = <Date 2014-03-17.21:00:36.811>
    actor = 'python-dev'
    assignee = 'none'
    closed = True
    closed_date = <Date 2014-02-23.15:52:07.847>
    closer = 'pitrou'
    components = ['Interpreter Core']
    creation = <Date 2014-02-15.22:49:04.178>
    creator = 'pingebretson'
    dependencies = []
    files = ['34096', '34106']
    hgrepos = []
    issue_num = 20637
    keywords = ['patch']
    message_count = 10.0
    messages = ['211302', '211315', '211320', '211358', '211376', '211378', '211385', '212002', '212003', '213895']
    nosy_count = 8.0
    nosy_names = ['pitrou', 'vstinner', 'larry', 'benjamin.peterson', 'Trundle', 'Mark.Shannon', 'pingebretson', 'python-dev']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'resource usage'
    url = 'https://bugs.python.org/issue20637'
    versions = ['Python 3.4', 'Python 3.5']

    @pingebretson
    Copy link
    Mannequin Author

    pingebretson mannequin commented Feb 15, 2014

    PEP-412 shared keys are not created for subclasses in Python 3.3 and 3.4:

    >>> import sys
    >>> class A:
    ...     pass
    ... 
    >>> class B(A):
    ...     pass
    ... 
    >>> a, b = A(), B()
    >>> sys.getsizeof(vars(a))
    96
    >>> sys.getsizeof(vars(b))
    288

    (Actual sizes depend on platform and configuration).

    This patch allows subclasses to share keys:

    >>> import sys
    >>> class A:
    ...     pass
    ... 
    >>> class B(A):
    ...     pass
    ... 
    >>> a, b = A(), B()
    >>> sys.getsizeof(vars(a))
    96
    >>> sys.getsizeof(vars(b))
    96

    @pingebretson pingebretson mannequin added interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage labels Feb 15, 2014
    @pitrou
    Copy link
    Member

    pitrou commented Feb 16, 2014

    Wow, really? Thanks for finding this!

    @markshannon
    Copy link
    Member

    Well spotted.

    I don't think that the line
    COPYVAL(tp_dictoffset);
    should be removed.

    inherit_special() is called from PyType_Ready
    which is used to initialise static TypeObjects.
    So builtin class B which doesn't set tp_dictoffset,
    but has builtin class A as its base, will not get its
    tp_dictoffset initialized from A.

    @pingebretson
    Copy link
    Mannequin Author

    pingebretson mannequin commented Feb 16, 2014

    Thanks Mark, I agree.

    I thought that assigning to tp_dictoffset in inherit_special was redundant with the assignment in inherit_slot and the assignment I added, but I see that COPYSLOT and COPYVAL are slightly different and extension types won't go through type_new.

    Here's an updated patch.

    @markshannon
    Copy link
    Member

    This looks good to me.

    Also, I think this should go into 3.4
    It is a tiny patch and could result in significant memory saving for OO programs with inheritance (e.g. Django)

    @vstinner
    Copy link
    Member

    Also, I think this should go into 3.4

    I think that it's too late for optimizations in Python 3.4 (RC1 is alread out). It can wait Python 3.5, or maybe 3.4.1.

    @larryhastings
    Copy link
    Contributor

    I agree: definitely not for 3.5, maybe for 3.4.1.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Feb 23, 2014

    New changeset 16229573e73e by Antoine Pitrou in branch 'default':
    Issue bpo-20637: Key-sharing now also works for instance dictionaries of subclasses. Patch by Peter Ingebretson.
    http://hg.python.org/cpython/rev/16229573e73e

    @pitrou
    Copy link
    Member

    pitrou commented Feb 23, 2014

    Thanks. I've committed a patch after augmenting the tests a bit, so it'll be in 3.4.1 as well as 3.5.

    @pitrou pitrou closed this as completed Feb 23, 2014
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 17, 2014

    New changeset afae24cb81d9 by Benjamin Peterson in branch '3.4':
    correct the fix for bpo-20637; allow slot descriptor inheritance to take place before creating cached keys
    http://hg.python.org/cpython/rev/afae24cb81d9

    @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
    Labels
    interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants