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

Harmonize STORE_DEREF with STORE_FAST and LOAD_DEREF #72851

Closed
rhettinger opened this issue Nov 11, 2016 · 11 comments
Closed

Harmonize STORE_DEREF with STORE_FAST and LOAD_DEREF #72851

rhettinger opened this issue Nov 11, 2016 · 11 comments
Assignees
@rhettinger
Labels
3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage

Comments

@rhettinger
Copy link
Contributor

BPO 28665
Nosy @rhettinger, @serhiy-storchaka
PRs
  • [Do Not Merge] Convert Misc/NEWS so that it is managed by towncrier #552
    		•
    Files
  • fastcell.diff
  • delete_deref.diff: Fix-up DELETE_DEREF
  • concat_deref.diff: Fix-up unicode_concat
  • issue28665.py
    		•

    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 2016-11-12.09:12:16.093>
created_at = <Date 2016-11-11.12:10:41.742>
labels = ['interpreter-core', '3.7', 'performance']
title = 'Harmonize STORE_DEREF with STORE_FAST and LOAD_DEREF'
updated_at = <Date 2017-03-31.16:36:26.072>
user = 'https://github.com/rhettinger'

    bugs.python.org fields:

    activity = <Date 2017-03-31.16:36:26.072>
actor = 'dstufft'
assignee = 'rhettinger'
closed = True
closed_date = <Date 2016-11-12.09:12:16.093>
closer = 'rhettinger'
components = ['Interpreter Core']
creation = <Date 2016-11-11.12:10:41.742>
creator = 'rhettinger'
dependencies = []
files = ['45442', '45454', '45455', '45457']
hgrepos = []
issue_num = 28665
keywords = ['patch']
message_count = 11.0
messages = ['280574', '280576', '280577', '280578', '280584', '280585', '280635', '280642', '280643', '280645', '280646']
nosy_count = 3.0
nosy_names = ['rhettinger', 'python-dev', 'serhiy.storchaka']
pr_nums = ['552']
priority = 'low'
resolution = 'fixed'
stage = 'commit review'
status = 'closed'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue28665'
versions = ['Python 3.7']
    @rhettinger
    Copy link
    Contributor Author

    The STORE_FAST, LOAD_FAST, and LOAD_DEREF opcodes all use fast macros for variable access. This patch harmonizes STORE_DEREF to follow the same pattern.

    Both the C code and the generated assembly look nicer. Gives an approx 40% speed-up (using both Clang and GCC-6) on the "store_nonlocal" portion of the variable access benchmark at http://code.activestate.com/recipes/577834
    The eliminates the nonlocal speed penalty, making cell variable updates run nearly as fast as updates to locals.

    @rhettinger rhettinger added 3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage labels Nov 11, 2016
    @serhiy-storchaka
    Copy link
    Member

    LGTM. This saves function call and INCREF/DECREF pair.

    What about DELETE_DEREF? PyCell_Set() also is used in unicode_concatenate().

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 11, 2016

    New changeset d78d45436753 by Raymond Hettinger in branch '3.6':
    Issue bpo-28665: Harmonize STORE_DEREF with STORE_FAST and LOAD_DEREF giving a 40% speedup.
    https://hg.python.org/cpython/rev/d78d45436753

    @rhettinger
    Copy link
    Contributor Author

    Thanks for the quick review. I'll look at the other two cases when I get a chance.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 11, 2016

    New changeset 7ec45e7d2194 by Serhiy Storchaka in branch 'default':
    Merge from 3.6 (issue bpo-28665).
    https://hg.python.org/cpython/rev/7ec45e7d2194

    @serhiy-storchaka
    Copy link
    Member

    You forgot to merge and created new head.

    @serhiy-storchaka
    Copy link
    Member

    Could you please measure the performance effect of these changes?

    @rhettinger
    Copy link
    Contributor Author

    I think the speed benefit for the last two patches is likely too modest to care about. The main reason I did the work was because you suggested it and because it seemed like a reasonable idea (the patched code looks nice, it does only work that is necessary, and it is more consistent with the other opcodes).

    Let me know if you want to go forward with it or leave it in the current state (the current juxtaposition of PyCell_GET with PyCell_Set looks a little weird but it does get the job done).

    @serhiy-storchaka
    Copy link
    Member

    The argument about "harmonizing" doesn't look strong to me. Opcodes for locals use the SETLOCAL() macro which decrefs
    old value, while opcodes for nonlocals with your patches use the PyCell_SET() macro which doesn't.

    But performance arguments look more weighty. I made benchmarks. fastcell.diff speeds up STORE_FAST by 40%, delete_deref.diff speeds up DELETE_DEREF by 50%. and concat_deref.diff speeds up string concatenating up to 15%. All these operations are rare in comparison with operations with locals or LOAD_DEREF, but the cognitive cost of the optimization is pretty low. All patches LGTM.

    I only have doubts that such changes could be pushed in 3.6 at this stage. This is not bug fix and isn't tweaking new 3.6 feature.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 12, 2016

    New changeset 5f3b7ceb394c by Raymond Hettinger in branch 'default':
    Issue bpo-28665: Use macro form of PyCell_GET/SET
    https://hg.python.org/cpython/rev/5f3b7ceb394c

    @rhettinger
    Copy link
    Contributor Author

    I think small changes are fine while there is still another beta ahead but would rather just push it to 3.7 than spend more time talking about it.

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

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