◐ Shell
reader mode source ↗
Skip to content

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

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

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

Conversation

@brandtbucher

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

Copy link
Copy Markdown
Member

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
markshannon
markshannon reviewed Nov 9, 2021
View reviewed changes

@markshannon markshannon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

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

bedevere-bot commented Nov 10, 2021

Copy link
Copy Markdown

🤖 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.

@markshannon

markshannon commented Nov 10, 2021

Copy link
Copy Markdown
Member

👍
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

@Fidget-Spinner Fidget-Spinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

Awesome work!

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

bedevere-bot commented Nov 10, 2021

Copy link
Copy Markdown

🤖 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

brandtbucher commented Nov 11, 2021

Copy link
Copy Markdown
Member Author

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