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

Break up COMPARE_OP into logically distinct operations. #83337

Closed
markshannon opened this issue Dec 29, 2019 · 7 comments
Closed

Break up COMPARE_OP into logically distinct operations. #83337

markshannon opened this issue Dec 29, 2019 · 7 comments
Labels
3.9 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage

Comments

@markshannon
Copy link
Member

BPO 39156
Nosy @rhettinger, @markshannon, @serhiy-storchaka, @pablogsal
PRs
  • bpo-39156: Break up COMPARE_OP into four logically distinct opcodes. #17754
  • 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 2020-01-14.10:13:33.812>
    created_at = <Date 2019-12-29.19:44:42.502>
    labels = ['interpreter-core', '3.9', 'performance']
    title = 'Break up COMPARE_OP into logically distinct operations.'
    updated_at = <Date 2020-01-14.10:13:33.812>
    user = 'https://github.com/markshannon'

    bugs.python.org fields:

    activity = <Date 2020-01-14.10:13:33.812>
    actor = 'Mark.Shannon'
    assignee = 'none'
    closed = True
    closed_date = <Date 2020-01-14.10:13:33.812>
    closer = 'Mark.Shannon'
    components = ['Interpreter Core']
    creation = <Date 2019-12-29.19:44:42.502>
    creator = 'Mark.Shannon'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 39156
    keywords = ['patch']
    message_count = 7.0
    messages = ['359002', '359003', '359005', '359009', '359013', '359032', '359963']
    nosy_count = 4.0
    nosy_names = ['rhettinger', 'Mark.Shannon', 'serhiy.storchaka', 'pablogsal']
    pr_nums = ['17754']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'performance'
    url = 'https://bugs.python.org/issue39156'
    versions = ['Python 3.9']

    @markshannon
    Copy link
    Member Author

    Currently the COMPARE_OP instruction performs one of four different tasks.
    We should break it up into four different instructions, that each performs only one of those tasks.

    The four tasks are:
    Rich comparison (>, <, ==, !=, >=, <=)
    Identity comparison (is, is not)
    Contains test (in, not in)
    Exception matching

    The current implementation involves an unnecessary extra dispatch to determine which task to perform.
    Comparisons are common operations, so this extra call and unpredictable branch has a cost.

    In addition, testing for exception matching is always followed by a branch, so the test and branch can be combined.

    I propose adding three new instructions and changing the meaning of COMPARE_OP.

    COMPARE_OP should only perform rich comparisons, and should call PyObject_RichCompare directly.
    IS_OP performs identity tests, performs no calls and cannot fail.
    CONTAINS_OP tests for 'in and 'not in' and should call PySequence_Contains directly.
    JUMP_IF_NOT_EXC_MATCH Tests whether the exception matches and jumps if it does not.

    @markshannon markshannon added 3.9 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage labels Dec 29, 2019
    @pablogsal
    Copy link
    Member

    I think is a good idea, being the only problem that I see that as the opcode targets are limited we would be burning 3 more. I specially like the proposal for JUMP_IF_NOT_EXC_MATCH as PyCmp_EXC_MATCH is only used in the code for the try-except and is always followed by a POP_JUMP_IF_FALSE.

    As a curiosity, it would be good to have an idea on performance gains of specializing the comparison.

    @rhettinger
    Copy link
    Contributor

    When COMPARE_OP occurs in a loop, the dispatch tends to be branch predictable. So there may not be real-world performance benefit to splitting the opcodes.

    @serhiy-storchaka
    Copy link
    Member

    Few years ago I experimented with a special opcode for exception matching. It could make the bytecode a tiny bit smaller and faster, but since catching an exception in Python is relatively expensive, it would not have significant performance benefit.

    As for splitting COMPARE_OP for comparison, identity and containment tests, all these operations look the same from the compiler side, they correspond the same AST node. Introducing new opcodes will complicate the compiler.

    @rhettinger
    Copy link
    Contributor

    Introducing new opcodes will complicate the compiler.

    And it will complicate opcode.py and peephole.c and anything else that touches the word codes.

    @markshannon
    Copy link
    Member Author

    Moving work from the interpreter to the compiler is always a good idea.

    Performance: The compiler is run once per code unit, the interpreter thousands or millions of times.

    The compiler is easier to test. Just match the expected bytecode with the actual bytecode.
    The output can be sanity checked by visual inspection.

    Although I expect a performance boost, I think this is a worthwhile improvement whether or not it helps
    performance as it makes the instructions better focused.

    Pablo, currently there are 117 opcodes, increasing that to 120 is not a problem.
    Also there is no reason why we are limited to 256 opcodes in the long term.
    Plus, I'm 4 opcodes in credit, thanks to https://bugs.python.org/issue33387 :)

    Raymond, regarding the performance of COMPARE_OP, it is not just branch prediction that matters. With this change, the number of (hardware) instructions executed is always reduced, even if branch prediction is no better.

    Serhiy, the benefit of having a special opcode for exception matching is not really to speed up exception matching, but to avoid slowing down other tests.

    @markshannon
    Copy link
    Member Author

    New changeset 9af0e47 by Mark Shannon in branch 'master':
    bpo-39156: Break up COMPARE_OP into four logically distinct opcodes. (GH-17754)
    9af0e47

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

    No branches or pull requests

    4 participants