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

Switch dict and set structures to PyVarObject #73174

Closed
serhiy-storchaka opened this issue Dec 16, 2016 · 7 comments
Closed

Switch dict and set structures to PyVarObject #73174

serhiy-storchaka opened this issue Dec 16, 2016 · 7 comments
Assignees
@rhettinger
Labels
3.7 (EOL) end of life extension-modules C modules in the Modules dir type-feature A feature request or enhancement

Comments

@serhiy-storchaka
Copy link
Member

BPO 28988
Nosy @rhettinger, @vstinner, @methane, @serhiy-storchaka
Files
  • dict-set-var-object.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 = 'https://github.com/rhettinger'
closed_at = <Date 2019-08-22.09:17:24.091>
created_at = <Date 2016-12-16.15:01:05.774>
labels = ['extension-modules', 'type-feature', '3.7']
title = 'Switch dict and set structures to PyVarObject'
updated_at = <Date 2019-08-22.09:17:24.088>
user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2019-08-22.09:17:24.088>
actor = 'rhettinger'
assignee = 'rhettinger'
closed = True
closed_date = <Date 2019-08-22.09:17:24.091>
closer = 'rhettinger'
components = ['Extension Modules']
creation = <Date 2016-12-16.15:01:05.774>
creator = 'serhiy.storchaka'
dependencies = []
files = ['45925']
hgrepos = []
issue_num = 28988
keywords = ['patch']
message_count = 7.0
messages = ['283404', '283405', '283679', '283719', '283733', '283735', '350175']
nosy_count = 4.0
nosy_names = ['rhettinger', 'vstinner', 'methane', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'rejected'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue28988'
versions = ['Python 3.7']
    @serhiy-storchaka
    Copy link
    Member Author

    In bpo-28959 Raymond said that a dict structure can be switched to PyVarObject. Victor guessed that it makes sense to switch a set structure too. Proposed patch implements this. The layout of a dict structure is not changed. The number of used and filled entries in a set structure are swapped, therefore extensions that use PySet_GET_SIZE or access fields of PySetObject directly (both are not in a stable API) should be recompiled.

    I don't see a benefit from this patch except some unification.

    @serhiy-storchaka serhiy-storchaka added the 3.7 (EOL) end of life label Dec 16, 2016
    @serhiy-storchaka serhiy-storchaka added extension-modules C modules in the Modules dir type-feature A feature request or enhancement labels Dec 16, 2016
    @vstinner
    Copy link
    Member

    Copy/paste of my msg283176:

    dict doesn't end with array.

    Right, but...

    Recently I looked at dict internals. As a newcomer, I have to confess that it's currently really hard to understand the purpose of each dict field: they are many "sizes": size of the hash table, number of usable entries, number of used entries, number of items in the dictionary, etc.

    I like the idea of using the standard ob_size field (PyVarObject) to make it more explicitl that this field is the expected result of len(obj).

    Technically, I don't know any function inside Python core which rely on the fact that object data is at the end of the main memory block.

    Other builtin Python types:

    • tuple: PyVarObject
    • list: PyVarObject
    • bytes: PyVarObject
    • bytearray: PyVarObject
    • memoryview: PyVarObject
    • set: "used" field
    • str: "length" field

    The str type is super optimized for memory footprint, there are technical reasons for not used PyVarObject here, like backward compatibility.

    It may make sense to modify PySetObject to use PyVarObject as well, but that's a different topic :-)

    @methane
    Copy link
    Member

    methane commented Dec 20, 2016

    LGTM

    @rhettinger
    Copy link
    Contributor

    Please leave the set object patch for me. I would like to do it a bit differently.

    @vstinner
    Copy link
    Member

    Raymond Hettinger:

    Please leave the set object patch for me. I would like to do it a bit differently.

    Differently? How?

    @serhiy-storchaka
    Copy link
    Member Author

    Since I don't see a benefit from this change I leave both patches to you Raymond.

    @rhettinger
    Copy link
    Contributor

    Since I don't see a benefit from this change I leave both patches
    to you Raymond.

    After more reflection, I also don't see any point of doing this. While there is a minor whiff of consistency when using Py_SIZE(), it makes the assignment look weird and inconsistent between filled and used. I now favor local consistency and clarity (with in setobject.c) over trying to make it look exactly like some other modules.

    @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

    @rhettinger rhettinger

    Labels
    3.7 (EOL) end of life extension-modules C modules in the Modules dir type-feature A feature request or enhancement
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    4 participants
    @vstinner @methane @rhettinger @serhiy-storchaka