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

Merge BINARY_*/INPLACE_* into BINARY_OP #89799

Closed
brandtbucher opened this issue Oct 27, 2021 · 7 comments
Closed

Merge BINARY_*/INPLACE_* into BINARY_OP #89799

brandtbucher opened this issue Oct 27, 2021 · 7 comments
Assignees
Labels
3.11 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage

Comments

@brandtbucher
Copy link
Member

BPO 45636
Nosy @gvanrossum, @markshannon, @brandtbucher, @iritkatriel
PRs
  • bpo-45636: Merge BINARY_*/INPLACE_* into BINARY_OP/INPLACE_OP #29255
  • bpo-45636: Simpler design for BINARY_OP/INPLACE_OP #29418
  • bpo-45636: BINARY_OP (third time's the charm) #29482
  • bpo-45636: Remove the old %-formatting fast-path #29532
  • bpo-45636: Un-switch BINARY_OP #29565
  • bpo-45635: Do not suppress errors in functions called from _PyErr_Display #30073
  • 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/brandtbucher'
    closed_at = <Date 2021-11-19.10:53:23.973>
    created_at = <Date 2021-10-27.22:13:52.199>
    labels = ['interpreter-core', '3.11', 'performance']
    title = 'Merge BINARY_*/INPLACE_* into BINARY_OP'
    updated_at = <Date 2021-12-12.15:47:51.087>
    user = 'https://github.com/brandtbucher'

    bugs.python.org fields:

    activity = <Date 2021-12-12.15:47:51.087>
    actor = 'iritkatriel'
    assignee = 'brandtbucher'
    closed = True
    closed_date = <Date 2021-11-19.10:53:23.973>
    closer = 'Mark.Shannon'
    components = ['Interpreter Core']
    creation = <Date 2021-10-27.22:13:52.199>
    creator = 'brandtbucher'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 45636
    keywords = ['patch']
    message_count = 7.0
    messages = ['405136', '405179', '405221', '406147', '406148', '406355', '406403']
    nosy_count = 4.0
    nosy_names = ['gvanrossum', 'Mark.Shannon', 'brandtbucher', 'iritkatriel']
    pr_nums = ['29255', '29418', '29482', '29532', '29565', '30073']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'performance'
    url = 'https://bugs.python.org/issue45636'
    versions = ['Python 3.11']

    @brandtbucher
    Copy link
    Member Author

    ...as discussed in faster-cpython/ideas#101.

    This change merges all BINARY_/INPLACE_ instructions, except for a few special cases:

    • BINARY_ADD/INPLACE_ADD, which interact with sq_concat/sq_inplace_concat and already have their own specialization family.
    • BINARY_MULTIPLY/INPLACE_MULTIPLY, which interact with sq_repeat/sq_inplace_repeat and already have their own specialization family.
    • BINARY_POWER/INPLACE_POWER, which are technically ternary operators under-the-hood.
    • BINARY_MODULO/INPLACE_MODULO, which contain a special fast path for string formatting (but likely can be rolled in later as a specialization).

    It has no mean impact on pyperformance, shrinks the eval loop, and makes it much simpler to implement operator specializations.

    @brandtbucher brandtbucher added the 3.11 only security fixes label Oct 27, 2021
    @brandtbucher brandtbucher self-assigned this Oct 27, 2021
    @brandtbucher brandtbucher added interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage 3.11 only security fixes labels Oct 27, 2021
    @brandtbucher brandtbucher self-assigned this Oct 27, 2021
    @brandtbucher brandtbucher added interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage labels Oct 27, 2021
    @markshannon
    Copy link
    Member

    Do you have results for pyperformance?

    @brandtbucher
    Copy link
    Member Author

    Slower (29):

    • unpack_sequence: 43.7 ns +- 0.9 ns -> 45.7 ns +- 1.1 ns: 1.04x slower
    • float: 80.5 ms +- 0.9 ms -> 83.5 ms +- 1.3 ms: 1.04x slower
    • regex_effbot: 3.15 ms +- 0.03 ms -> 3.26 ms +- 0.04 ms: 1.04x slower
    • go: 165 ms +- 1 ms -> 171 ms +- 3 ms: 1.03x slower
    • pickle_dict: 26.8 us +- 0.1 us -> 27.5 us +- 0.1 us: 1.03x slower
    • scimark_monte_carlo: 77.5 ms +- 0.8 ms -> 79.3 ms +- 1.3 ms: 1.02x slower
    • sqlalchemy_imperative: 18.6 ms +- 0.5 ms -> 18.9 ms +- 0.9 ms: 1.02x slower
    • chaos: 76.7 ms +- 0.7 ms -> 78.1 ms +- 0.8 ms: 1.02x slower
    • logging_format: 6.73 us +- 0.09 us -> 6.84 us +- 0.08 us: 1.02x slower
    • richards: 56.9 ms +- 0.9 ms -> 57.7 ms +- 1.0 ms: 1.01x slower
    • chameleon: 7.48 ms +- 0.10 ms -> 7.58 ms +- 0.12 ms: 1.01x slower
    • json_loads: 25.4 us +- 0.2 us -> 25.7 us +- 0.2 us: 1.01x slower
    • sympy_expand: 501 ms +- 5 ms -> 507 ms +- 4 ms: 1.01x slower
    • logging_silent: 116 ns +- 3 ns -> 117 ns +- 2 ns: 1.01x slower
    • django_template: 37.2 ms +- 0.5 ms -> 37.7 ms +- 0.4 ms: 1.01x slower
    • regex_v8: 23.2 ms +- 0.1 ms -> 23.4 ms +- 0.3 ms: 1.01x slower
    • regex_dna: 212 ms +- 1 ms -> 214 ms +- 1 ms: 1.01x slower
    • xml_etree_process: 59.1 ms +- 0.6 ms -> 59.6 ms +- 0.6 ms: 1.01x slower
    • xml_etree_generate: 80.4 ms +- 0.7 ms -> 81.2 ms +- 0.8 ms: 1.01x slower
    • scimark_lu: 138 ms +- 3 ms -> 140 ms +- 3 ms: 1.01x slower
    • logging_simple: 6.15 us +- 0.08 us -> 6.20 us +- 0.09 us: 1.01x slower
    • regex_compile: 144 ms +- 2 ms -> 145 ms +- 1 ms: 1.01x slower
    • spectral_norm: 107 ms +- 1 ms -> 108 ms +- 2 ms: 1.01x slower
    • 2to3: 271 ms +- 1 ms -> 272 ms +- 1 ms: 1.00x slower
    • sympy_integrate: 22.0 ms +- 0.2 ms -> 22.1 ms +- 0.1 ms: 1.00x slower
    • sympy_str: 303 ms +- 4 ms -> 304 ms +- 3 ms: 1.00x slower
    • dulwich_log: 67.6 ms +- 0.3 ms -> 67.8 ms +- 0.3 ms: 1.00x slower
    • python_startup_no_site: 5.90 ms +- 0.00 ms -> 5.91 ms +- 0.01 ms: 1.00x slower
    • python_startup: 8.51 ms +- 0.01 ms -> 8.52 ms +- 0.01 ms: 1.00x slower

    Faster (13):

    • pickle_list: 4.48 us +- 0.04 us -> 4.31 us +- 0.03 us: 1.04x faster
    • scimark_fft: 355 ms +- 10 ms -> 348 ms +- 3 ms: 1.02x faster
    • nqueens: 90.1 ms +- 0.7 ms -> 88.3 ms +- 0.8 ms: 1.02x faster
    • xml_etree_iterparse: 107 ms +- 3 ms -> 105 ms +- 2 ms: 1.02x faster
    • nbody: 115 ms +- 2 ms -> 113 ms +- 3 ms: 1.02x faster
    • fannkuch: 427 ms +- 4 ms -> 420 ms +- 4 ms: 1.02x faster
    • unpickle_list: 5.05 us +- 0.04 us -> 4.96 us +- 0.06 us: 1.02x faster
    • telco: 6.38 ms +- 0.25 ms -> 6.29 ms +- 0.13 ms: 1.01x faster
    • json_dumps: 12.6 ms +- 0.1 ms -> 12.5 ms +- 0.1 ms: 1.01x faster
    • pyflate: 539 ms +- 9 ms -> 533 ms +- 3 ms: 1.01x faster
    • crypto_pyaes: 87.5 ms +- 0.6 ms -> 86.6 ms +- 1.2 ms: 1.01x faster
    • raytrace: 331 ms +- 2 ms -> 330 ms +- 2 ms: 1.00x faster
    • mako: 11.9 ms +- 0.1 ms -> 11.9 ms +- 0.1 ms: 1.00x faster

    Benchmark hidden because not significant (16): deltablue, hexiom, meteor_contest, pathlib, pickle, pickle_pure_python, pidigits, scimark_sor, scimark_sparse_mat_mult, sqlalchemy_declarative, sqlite_synth, sympy_sum, tornado_http, unpickle, unpickle_pure_python, xml_etree_parse

    Geometric mean: 1.00x slower

    @brandtbucher
    Copy link
    Member Author

    New changeset 9178f53 by Brandt Bucher in branch 'main':
    bpo-45636: Merge all numeric operators (GH-29482)
    9178f53

    @brandtbucher brandtbucher changed the title Merge BINARY_*/INPLACE_* into BINARY_OP/INPLACE_OP Merge BINARY_*/INPLACE_* into BINARY_OP Nov 11, 2021
    @brandtbucher brandtbucher changed the title Merge BINARY_*/INPLACE_* into BINARY_OP/INPLACE_OP Merge BINARY_*/INPLACE_* into BINARY_OP Nov 11, 2021
    @brandtbucher
    Copy link
    Member Author

    Tasks for tomorrow:

    • Reimplement the string formatting fast-path as a proper specialization.
    • Try indexing into an array of function pointers instead of switching.
    • Experiment more generic specializations for all operators (for instance, when lhs.__class__ is rhs.__class__, or when only one valid implementation exists).

    @brandtbucher
    Copy link
    Member Author

    New changeset ec382fa by Brandt Bucher in branch 'main':
    bpo-45636: Remove the old %-formatting fast-path (GH-29532)
    ec382fa

    @brandtbucher
    Copy link
    Member Author

    New changeset 6a84d61 by Brandt Bucher in branch 'main':
    bpo-45636: Simplify BINARY_OP (GH-29565)
    6a84d61

    @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.11 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

    2 participants