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

SETUP_WITH #50351

Closed
benjaminp opened this issue May 24, 2009 · 14 comments
Closed

SETUP_WITH #50351

benjaminp opened this issue May 24, 2009 · 14 comments
Assignees
@benjaminp
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage

Comments

@benjaminp
Copy link
Contributor

BPO 6101
Nosy @rhettinger, @pitrou, @benjaminp
Files
  • SETUP_WITH.patch
  • SETUP_WITH2.patch
  • SETUP_WITH3.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/benjaminp'
closed_at = <Date 2009-05-25.13:14:09.650>
created_at = <Date 2009-05-24.23:31:35.288>
labels = ['interpreter-core', 'performance']
title = 'SETUP_WITH'
updated_at = <Date 2010-12-04.20:01:17.394>
user = 'https://github.com/benjaminp'

    bugs.python.org fields:

    activity = <Date 2010-12-04.20:01:17.394>
actor = 'rhettinger'
assignee = 'benjamin.peterson'
closed = True
closed_date = <Date 2009-05-25.13:14:09.650>
closer = 'benjamin.peterson'
components = ['Interpreter Core']
creation = <Date 2009-05-24.23:31:35.288>
creator = 'benjamin.peterson'
dependencies = []
files = ['14060', '14061', '14062']
hgrepos = []
issue_num = 6101
keywords = ['patch']
message_count = 14.0
messages = ['88293', '88294', '88295', '88296', '88297', '88315', '88317', '123384', '123385', '123389', '123390', '123393', '123394', '123395']
nosy_count = 4.0
nosy_names = ['rhettinger', 'pitrou', 'thomasvs', 'benjamin.peterson']
pr_nums = []
priority = 'normal'
resolution = 'accepted'
stage = 'commit review'
status = 'closed'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue6101'
versions = ['Python 2.7', 'Python 3.2']
    @benjaminp
    Copy link
    Contributor Author

    This patch condenses the many current opcodes used to start a with
    statement into one, SETUP_WITH. I originally did this to properly lookup
    __enter__ and __exit__ as special methods. However, the patch also has
    the nice side effect of removing the need for a temporary variable.

    @benjaminp benjaminp self-assigned this May 24, 2009
    @benjaminp benjaminp added interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage labels May 24, 2009
    @benjaminp
    Copy link
    Contributor Author

    Performance numbers:

    With patch:

    $ ./python.exe -m timeit -s 'import thread; l = thread.allocate_lock()'
'with l: pass'
1000000 loops, best of 3: 1.99 usec per loop

    Without:
    100000 loops, best of 3: 2.15 usec per loop

    @pitrou
    Copy link
    Member

    pitrou commented May 24, 2009

    I'm not sure I understand the point of PyInstance_Check() in
    lookup_special().

    You should bump the bytecode version in import.c.

    By the way, there are some "with" tests in pybench (you can run them
    using "-t With").

    @benjaminp
    Copy link
    Contributor Author

    2009/5/24 Antoine Pitrou <report@bugs.python.org>:

    Antoine Pitrou <pitrou@free.fr> added the comment:

    I'm not sure I understand the point of PyInstance_Check() in
    lookup_special().

    _PyObject_LookupSpecial doesn't understand classic classes.

    You should bump the bytecode version in import.c.

    Ok

    Thanks.

    @pitrou
    Copy link
    Member

    pitrou commented May 25, 2009

    Endly, in compile.c, it seems the stack effect of SETUP_WITH should be
    1, not 3 (only one value is pushed onto the stack).

    @pitrou
    Copy link
    Member

    pitrou commented May 25, 2009

    SETUP_WITH3.patch looks good to me.

    @benjaminp
    Copy link
    Contributor Author

    Applied in r72912.

    @thomasvs
    Copy link
    Mannequin

    thomasvs mannequin commented Dec 4, 2010

    Maybe I am missing something, but why was it ok for this patch to move EXTENDED_ARGS from 143 to 145 ? I thought the numbers for opcodes were part of the ABI ?

    @benjaminp
    Copy link
    Contributor Author

    2010/12/4 Thomas Vander Stichele <report@bugs.python.org>:

    Thomas Vander Stichele <thomasvs@users.sourceforge.net> added the comment:

    Maybe I am missing something, but why was it ok for this patch to move EXTENDED_ARGS from 143 to 145 ? I thought the numbers for opcodes were part of the ABI ?

    Very much not.

    @thomasvs
    Copy link
    Mannequin

    thomasvs mannequin commented Dec 4, 2010

    Really ? Is this documented somewhere ? Do you know of any other case where a number for an existing opcode was changed ? I can't find any so far.

    @pitrou
    Copy link
    Member

    pitrou commented Dec 4, 2010

    Really ? Is this documented somewhere ? Do you know of any other case
    where a number for an existing opcode was changed ? I can't find any
    so far.

    Opcodes are an implementation detail. If you are fiddling with opcodes,
    how will your code work under Jython, IronPython or even PyPy?
    Besides, not only opcode numbers may change, but the semantics of a
    given opcode can change too (even if its number stays the same). So
    there's nothing stable you can rely on here.

    @thomasvs
    Copy link
    Mannequin

    thomasvs mannequin commented Dec 4, 2010

    Well, I just checked, and from 2.3 to 2.6 opcodes were only added, existing ones were never renumbered.

    2.7 however reshuffled a bunch of them, for no apparent reason at all:

    $ diff -au opcodes-2.6 opcodes-2.7
--- opcodes-2.6	2010-12-04 20:47:19.110031279 +0100
+++ opcodes-2.7	2010-12-04 20:47:06.770611299 +0100
@@ -10,7 +10,6 @@
   12 UNARY_NOT
   13 UNARY_CONVERT
   15 UNARY_INVERT
-  18 LIST_APPEND
   19 BINARY_POWER
   20 BINARY_MULTIPLY
   21 BINARY_DIVIDE
@@ -73,6 +72,7 @@
   91 DELETE_NAME
   92 UNPACK_SEQUENCE
   93 FOR_ITER
+  94 LIST_APPEND
   95 STORE_ATTR
   96 DELETE_ATTR
   97 STORE_GLOBAL
@@ -82,15 +82,18 @@
  101 LOAD_NAME
  102 BUILD_TUPLE
  103 BUILD_LIST
- 104 BUILD_MAP
- 105 LOAD_ATTR
- 106 COMPARE_OP
- 107 IMPORT_NAME
- 108 IMPORT_FROM
+ 104 BUILD_SET
+ 105 BUILD_MAP
+ 106 LOAD_ATTR
+ 107 COMPARE_OP
+ 108 IMPORT_NAME
+ 109 IMPORT_FROM
  110 JUMP_FORWARD
- 111 JUMP_IF_FALSE
- 112 JUMP_IF_TRUE
+ 111 JUMP_IF_FALSE_OR_POP
+ 112 JUMP_IF_TRUE_OR_POP
  113 JUMP_ABSOLUTE
+ 114 POP_JUMP_IF_FALSE
+ 115 POP_JUMP_IF_TRUE
  116 LOAD_GLOBAL
  119 CONTINUE_LOOP
  120 SETUP_LOOP
@@ -110,4 +113,7 @@
  140 CALL_FUNCTION_VAR
  141 CALL_FUNCTION_KW
  142 CALL_FUNCTION_VAR_KW
- 143 EXTENDED_ARG
+ 143 SETUP_WITH
+ 145 EXTENDED_ARG
+ 146 SET_ADD
+ 147 MAP_ADD

    @pitrou
    Copy link
    Member

    pitrou commented Dec 4, 2010

    Well, I just checked, and from 2.3 to 2.6 opcodes were only added,
    existing ones were never renumbered.

    2.7 however reshuffled a bunch of them, for no apparent reason at all:

    LIST_APPEND was renumbered because it gained an argument.
    There are also new opcodes in this list.
    Regardless, there is no guarantee about opcode numbering stability. You
    cannot infer such an expectation from previous behaviour.

    @rhettinger
    Copy link
    Contributor

    Yes, someone went nuts with renumbering. That is allowed but was probably unnecessary.

    That being said, users of opcodes should really use the names in opcode.py instead of the numbers themselves.

    @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

    @benjaminp benjaminp

    Labels
    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
    3 participants
    @benjaminp @rhettinger @pitrou