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

Jump to bottom

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

Merged
merged 18 commits into from Nov 11, 2021
Merged

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

merged 18 commits into from Nov 11, 2021

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:

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 requested a review from markshannon as a code owner Nov 9, 2021
@brandtbucher brandtbucher mentioned this pull request Nov 9, 2021
Closed
sweeneyde
sweeneyde reviewed Nov 9, 2021
View changes
Python/ceval.c Show resolved Hide resolved
Loading
markshannon
markshannon reviewed Nov 9, 2021
View changes
Copy link
Contributor

@markshannon markshannon left a comment

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

Loading

Python/ceval.c Outdated Show resolved Hide resolved
Loading
Python/ceval.c Outdated Show resolved Hide resolved
Loading
@markshannon markshannon self-assigned this Nov 10, 2021
@bedevere-bot
Copy link

@bedevere-bot 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 "🔨 test-with-buildbots" label again.

Loading

@markshannon
Copy link
Contributor

@markshannon 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 🙂

Loading

@markshannon markshannon mentioned this pull request Nov 10, 2021
Closed
@markshannon markshannon mentioned this pull request Nov 10, 2021
Closed
@markshannon markshannon mentioned this pull request Nov 10, 2021
Closed
Fidget-Spinner
Fidget-Spinner reviewed Nov 10, 2021
View changes
Copy link
Contributor

@Fidget-Spinner Fidget-Spinner left a comment

Awesome work!

Loading

Python/ceval.c Outdated Show resolved Hide resolved
Loading
@bedevere-bot
Copy link

@bedevere-bot 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 "🔨 test-with-buildbots" label again.

Loading

@brandtbucher
Copy link
Member Author

@brandtbucher brandtbucher commented Nov 11, 2021

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

Loading

@brandtbucher brandtbucher merged commit 9178f53 into python:main Nov 11, 2021
75 of 76 checks passed
Loading
@brandtbucher brandtbucher deleted the binary-op-unified branch Nov 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markshannon markshannon

@Fidget-Spinner Fidget-Spinner

@sweeneyde sweeneyde

Assignees

@markshannon markshannon

Labels
CLA signed
Projects
None yet
Milestone
No milestone
6 participants
@brandtbucher @bedevere-bot @markshannon @Fidget-Spinner @sweeneyde @the-knights-who-say-ni