GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-45636: `BINARY_OP` (third time's the charm) by brandtbucher · Pull Request #29482 · python/cpython

Conversation

@brandtbucher

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

Uh oh!

There was an error while loading. Please reload this page.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
brandtbucher added 14 commits October 27, 2021 13:16
@brandtbucher
Merge BINARY_*/INPLACE_* into BINARY_OP/INPLACE_OP
4b7125f
@brandtbucher
Clean things up a bit
3a87083
@brandtbucher
Improve the new _PyNumber_*Op implementations
a2df29a
@brandtbucher
Add docs
1b8934e
@brandtbucher
blurb add
3137973
@brandtbucher
Fix API leak
9905549
@brandtbucher
Clean up the nb_info struct
f697036
@brandtbucher
Clean up some comments
33f6e7c
@brandtbucher
Truly unify all binary ops
f9e388b
@brandtbucher
Fix BINARY_OP stats
f5b1923
@brandtbucher
Don't defer operators that can't be specialized
2a8072c
@brandtbucher
Remove some old changes
6c6b78f
@brandtbucher
Clarify behavior for non-specializing operators
51c4b5f
@brandtbucher
Clean up dis.py a bit
f0d2056
@brandtbucher
brandtbucher requested a review from markshannon as a code owner November 9, 2021 01:07
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Nov 9, 2021
@bedevere-bot bedevere-bot added the awaiting core review label Nov 9, 2021
@brandtbucher brandtbucher mentioned this pull request Nov 9, 2021
bpo-45636: Simpler design for BINARY_OP/INPLACE_OP #29418
Closed
@brandtbucher
Remove trailing whitespace
45e8502
sweeneyde
sweeneyde reviewed Nov 9, 2021
View reviewed changes
Comment thread Python/ceval.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

markshannon
markshannon reviewed Nov 9, 2021
View reviewed changes

@markshannon markshannon left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Python/ceval.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Python/ceval.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

brandtbucher added 2 commits November 9, 2021 17:27
@brandtbucher
Apply feedback from code review
8d23dad
@brandtbucher
Merge branch 'binary-op-unified' of github.com:brandtbucher/cpython i…
490b8da
…nto binary-op-unified
@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 10, 2021
@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 🙂

tekknolagi reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Nov 10, 2021
bpo-45508: Specialize INPLACE_ADD #29024
Closed
Experimental variant of BINARY_ADD specialization. #29059
Closed
@markshannon markshannon mentioned this pull request Nov 10, 2021
bpo-45510: Specialize BINARY_SUBTRACT #29010
Closed
Fidget-Spinner
Fidget-Spinner reviewed Nov 10, 2021
View reviewed changes

@Fidget-Spinner Fidget-Spinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Awesome work!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Python/ceval.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@brandtbucher
Fix a refleak during error handling
bea2cff
@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

sweeneyde reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@brandtbucher
brandtbucher merged commit 9178f53 into python:main Nov 11, 2021
@bedevere-bot bedevere-bot removed the awaiting core review label 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
Unify BINARY_OP bytecodes RustPython/RustPython#6317
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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@markshannon markshannon

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@brandtbucher @bedevere-bot @markshannon @Fidget-Spinner @sweeneyde @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.