Skip to content

bpo-45636: BINARY_OP (third time's the charm)#29482

Merged
brandtbucher merged 18 commits intopython:mainfrom
brandtbucher:binary-op-unified
Nov 11, 2021
Merged

bpo-45636: BINARY_OP (third time's the charm)#29482
brandtbucher merged 18 commits intopython:mainfrom
brandtbucher:binary-op-unified

Conversation

@brandtbucher
Copy link
Member

@brandtbucher brandtbucher commented Nov 9, 2021 •
edited by bedevere-bot
Loading

This merges all numeric BINARY_* and INPLACE_* instructions into one BINARY_OP instruction. As a consequence, the BINARY_ADD and BINARY_MULTIPLY specialization families are merged into a new BINARY_OP family too.

I prefer this new approach over the more complicated one taken in #29418. It simplifies the specialization logic for all operators (for example, += and *= now get specialized the same as + and *, basically for free).

Performance is unchanged vs main:

Details 
Slower (28):
- unpack_sequence: 43.7 ns +- 0.9 ns -> 46.8 ns +- 0.7 ns: 1.07x slower
- regex_v8: 23.2 ms +- 0.1 ms -> 24.1 ms +- 0.3 ms: 1.04x slower
- go: 165 ms +- 1 ms -> 171 ms +- 1 ms: 1.03x slower
- chaos: 76.7 ms +- 0.7 ms -> 79.3 ms +- 1.0 ms: 1.03x slower
- pickle_dict: 26.8 us +- 0.1 us -> 27.7 us +- 0.1 us: 1.03x slower
- pidigits: 190 ms +- 0 ms -> 195 ms +- 0 ms: 1.03x slower
- regex_dna: 212 ms +- 1 ms -> 217 ms +- 1 ms: 1.02x slower
- spectral_norm: 107 ms +- 1 ms -> 109 ms +- 3 ms: 1.02x slower
- regex_compile: 144 ms +- 2 ms -> 147 ms +- 1 ms: 1.02x slower
- sympy_expand: 501 ms +- 5 ms -> 510 ms +- 6 ms: 1.02x slower
- meteor_contest: 106 ms +- 2 ms -> 108 ms +- 3 ms: 1.02x slower
- json_loads: 25.4 us +- 0.2 us -> 25.8 us +- 0.2 us: 1.02x slower
- crypto_pyaes: 87.5 ms +- 0.6 ms -> 88.9 ms +- 0.5 ms: 1.02x slower
- sqlalchemy_imperative: 18.6 ms +- 0.5 ms -> 18.8 ms +- 0.9 ms: 1.02x slower
- telco: 6.38 ms +- 0.25 ms -> 6.47 ms +- 0.09 ms: 1.01x slower
- scimark_monte_carlo: 77.5 ms +- 0.8 ms -> 78.6 ms +- 1.4 ms: 1.01x slower
- richards: 56.9 ms +- 0.9 ms -> 57.5 ms +- 0.7 ms: 1.01x slower
- sympy_str: 303 ms +- 4 ms -> 306 ms +- 4 ms: 1.01x slower
- pyflate: 539 ms +- 9 ms -> 543 ms +- 5 ms: 1.01x slower
- float: 80.5 ms +- 0.9 ms -> 81.1 ms +- 1.2 ms: 1.01x slower
- sympy_integrate: 22.0 ms +- 0.2 ms -> 22.2 ms +- 0.2 ms: 1.01x slower
- unpickle_pure_python: 272 us +- 3 us -> 274 us +- 5 us: 1.01x slower
- scimark_sor: 154 ms +- 2 ms -> 155 ms +- 3 ms: 1.01x slower
- pathlib: 19.0 ms +- 0.3 ms -> 19.1 ms +- 0.3 ms: 1.01x slower
- raytrace: 331 ms +- 2 ms -> 333 ms +- 3 ms: 1.01x slower
- xml_etree_process: 59.1 ms +- 0.6 ms -> 59.4 ms +- 0.7 ms: 1.01x slower
- python_startup: 8.51 ms +- 0.01 ms -> 8.53 ms +- 0.01 ms: 1.00x slower
- python_startup_no_site: 5.90 ms +- 0.00 ms -> 5.90 ms +- 0.00 ms: 1.00x slower

Faster (17):
- scimark_sparse_mat_mult: 4.84 ms +- 0.08 ms -> 4.51 ms +- 0.03 ms: 1.07x faster
- unpickle: 13.9 us +- 1.3 us -> 13.3 us +- 0.2 us: 1.04x faster
- fannkuch: 427 ms +- 4 ms -> 410 ms +- 4 ms: 1.04x faster
- scimark_fft: 355 ms +- 10 ms -> 344 ms +- 5 ms: 1.03x faster
- pickle_list: 4.48 us +- 0.04 us -> 4.39 us +- 0.04 us: 1.02x faster
- pickle: 9.97 us +- 0.16 us -> 9.78 us +- 0.07 us: 1.02x faster
- json_dumps: 12.6 ms +- 0.1 ms -> 12.4 ms +- 0.1 ms: 1.02x faster
- pickle_pure_python: 367 us +- 3 us -> 363 us +- 4 us: 1.01x faster
- nbody: 115 ms +- 2 ms -> 114 ms +- 3 ms: 1.01x faster
- unpickle_list: 5.05 us +- 0.04 us -> 4.99 us +- 0.07 us: 1.01x faster
- hexiom: 7.53 ms +- 0.12 ms -> 7.45 ms +- 0.06 ms: 1.01x faster
- logging_silent: 116 ns +- 3 ns -> 115 ns +- 2 ns: 1.01x faster
- mako: 11.9 ms +- 0.1 ms -> 11.8 ms +- 0.1 ms: 1.01x faster
- nqueens: 90.1 ms +- 0.7 ms -> 89.3 ms +- 1.1 ms: 1.01x faster
- dulwich_log: 67.6 ms +- 0.3 ms -> 67.2 ms +- 0.4 ms: 1.01x faster
- logging_format: 6.73 us +- 0.09 us -> 6.69 us +- 0.08 us: 1.01x faster
- django_template: 37.2 ms +- 0.5 ms -> 37.1 ms +- 0.4 ms: 1.00x faster

Benchmark hidden because not significant (13): 2to3, chameleon, deltablue, logging_simple, regex_effbot, scimark_lu, sqlalchemy_declarative, sqlite_synth, sympy_sum, tornado_http, xml_etree_parse, xml_etree_iterparse, xml_etree_generate

Geometric mean: 1.00x slower

https://bugs.python.org/issue45636

@brandtbucher brandtbucher mentioned this pull request Nov 9, 2021
Closed
sweeneyde
sweeneyde reviewed Nov 9, 2021
View reviewed changes
markshannon
markshannon reviewed Nov 9, 2021
View reviewed changes
Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this approach. Performance is acceptable and should improve with specialization.

@markshannon markshannon self-assigned this Nov 10, 2021
@markshannon markshannon added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 10, 2021
@bedevere-bot
Copy link

bedevere-bot commented Nov 10, 2021

🤖 New build scheduled with the buildbot fleet by @markshannon for commit 490b8da 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 10, 2021
@markshannon
Copy link
Member

markshannon commented Nov 10, 2021

👍
It's good to be rid of the ad-hoc specialization of unicode_concatenate, not to mention freeing up 25 instructions for other purposes 🙂

This was referenced Nov 10, 2021
Closed
Closed
@markshannon markshannon mentioned this pull request Nov 10, 2021
Closed
Fidget-Spinner
Fidget-Spinner reviewed Nov 10, 2021
View reviewed changes
Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work!

@brandtbucher brandtbucher added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 10, 2021
@bedevere-bot
Copy link

bedevere-bot commented Nov 10, 2021

🤖 New build scheduled with the buildbot fleet by @brandtbucher for commit bea2cff 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 10, 2021
@brandtbucher
Copy link
Member Author

brandtbucher commented Nov 11, 2021

The one buildbot failure looks unrelated, and seems to be affecting other PRs as well.

@brandtbucher brandtbucher merged commit 9178f53 into python:main Nov 11, 2021
@brandtbucher brandtbucher deleted the binary-op-unified branch November 11, 2021 18:24
remykarem pushed a commit to remykarem/cpython that referenced this pull request Dec 7, 2021
@brandtbucher @remykarem
bpo-45636: Merge all numeric operators (pythonGH-29482)
1622deb
remykarem pushed a commit to remykarem/cpython that referenced this pull request Jan 30, 2022
@brandtbucher @remykarem
bpo-45636: Merge all numeric operators (pythonGH-29482)
ca7cae6
@ShaharNaveh ShaharNaveh mentioned this pull request Dec 1, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@markshannon markshannon markshannon left review comments

@Fidget-Spinner Fidget-Spinner Fidget-Spinner left review comments

@sweeneyde sweeneyde sweeneyde left review comments

Assignees

@markshannon markshannon

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@brandtbucher @bedevere-bot @markshannon @Fidget-Spinner @sweeneyde @the-knights-who-say-ni