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 — gh-100239: specialize bitwise logical binary ops on ints by iritkatriel · Pull Request #128927 · python/cpython

Conversation

@iritkatriel

@iritkatriel iritkatriel commented Jan 16, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

This adds specialisations for bitwise |, &, ^ on non-negative ints.

I'm not adding more in the same PR so we can more easily bisect in the future if we need to.

  • Issue: Specialize long tail of binary operations using a table. #100239

Sorry, something went wrong.

Uh oh!

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

All reactions
@iritkatriel
pythongh-100239: specialize bitwise logical binary ops on non-negativ…
c6f2deb
…e int
@bedevere-app bedevere-app Bot mentioned this pull request Jan 16, 2025
Specialize long tail of binary operations using a table. #100239
Open
@bedevere-app bedevere-app Bot added the awaiting core review label Jan 16, 2025
@iritkatriel
Merge branch 'main' into binaryops
c57fe46
@iritkatriel
iritkatriel requested review from markshannon and mdboom January 16, 2025 22:57
eendebakpt
eendebakpt reviewed Jan 16, 2025
View reviewed changes
Comment thread Python/specialize.c Outdated
return (is_nonnegative_compactlong(lhs) && is_nonnegative_compactlong(rhs));
}

#define NONNEGATIVE_LONGS_ACTION(NAME, OP) \

@eendebakpt eendebakpt Jan 16, 2025

Copy link
Copy Markdown
Contributor

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.

Why restrict to nonnegative longs here? If we do restrict, then we can replace the calls _PyLong_CompactValue with direct access to op->long_value.ob_digit[0]

Sorry, something went wrong.

Uh oh!

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

All reactions

@iritkatriel iritkatriel Jan 17, 2025

Copy link
Copy Markdown
Member Author

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.

Because negative ints need more work at runtime and I don't think they're common with bitwise logical ops.

Sorry, something went wrong.

Uh oh!

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

All reactions

@eendebakpt eendebakpt Jan 17, 2025

Copy link
Copy Markdown
Contributor

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.

The extra work is already done by _PyLong_CompactValue or am missing something? The output of ls OP rhs might not be a compact int, but there are no guards for the output type.

Sorry, something went wrong.

Uh oh!

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

All reactions

@iritkatriel iritkatriel Jan 17, 2025

Copy link
Copy Markdown
Member Author

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.

The extra work is already done by _PyLong_CompactValue or am missing something?

No, I think you're right. Good point.

The output of ls OP rhs might not be a compact int, but there are no guards for the output type.

For bitwise logical operators we should expect the results to have the same size as the inputs.

Sorry, something went wrong.

Uh oh!

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

All reactions
eendebakpt
eendebakpt reviewed Jan 17, 2025
View reviewed changes
Comment thread Misc/NEWS.d/next/Core_and_Builtins/2025-01-16-22-54-12.gh-issue-100239.7_HpBU.rst Resolved

Uh oh!

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

@iritkatriel
the globals thing
862bc67
@iritkatriel
iritkatriel requested a review from ericsnowcurrently as a code owner January 17, 2025 09:03
@iritkatriel @eendebakpt
Update Misc/NEWS.d/next/Core_and_Builtins/2025-01-16-22-54-12.gh-issu…
69e66f5
…e-100239.7_HpBU.rst

Co-authored-by: Pieter Eendebak <[email protected]>
@iritkatriel

iritkatriel commented Jan 17, 2025

Copy link
Copy Markdown
Member Author

The stats are weird with this PR, lot of misses: https://github.com/faster-cpython/benchmarking-public/blob/main/results/bm-20250116-3.14.0a4%2B-c57fe46/bm-20250116-azure-x86_64-iritkatriel-binaryops-3.14.0a4%2B-c57fe46-pystats-vs-base.md

image
All reactions

Sorry, something went wrong.

Uh oh!

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

iritkatriel added 4 commits January 17, 2025 18:09
@iritkatriel
add checks in test
0173413
@iritkatriel
no need to exclude negative
61d8bb3
@iritkatriel
EXIT_IF --> DEOPT_IF
1f48300
@iritkatriel
add specialization failure kind stats
e43ac58
kumaraditya303
kumaraditya303 reviewed Jan 18, 2025
View reviewed changes
Comment thread Python/specialize.c Resolved

Uh oh!

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

chris-eibl
chris-eibl reviewed Jan 18, 2025
View reviewed changes

@chris-eibl chris-eibl 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 think non-negative should be dropped in the news entry now?

Sorry, something went wrong.

Uh oh!

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

iritkatriel reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
iritkatriel
iritkatriel commented Jan 18, 2025
View reviewed changes
Comment thread Misc/NEWS.d/next/Core_and_Builtins/2025-01-16-22-54-12.gh-issue-100239.7_HpBU.rst Resolved

Uh oh!

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

@iritkatriel
remove non-negative
6916df4
chris-eibl
chris-eibl reviewed Jan 19, 2025
View reviewed changes
Comment thread Python/specialize.c Resolved

Uh oh!

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

@iritkatriel
add inplace ops
6476205
@chris-eibl

chris-eibl commented Jan 21, 2025

Copy link
Copy Markdown
Member

I think non-negative should be dropped in the news entry now?

And maybe in the title of this pull request, too?

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

@iritkatriel iritkatriel changed the title gh-100239: specialize bitwise logical binary ops on non-negative int gh-100239: specialize bitwise logical binary ops on ints Jan 21, 2025
iritkatriel added 2 commits January 21, 2025 17:10
@iritkatriel
Merge remote-tracking branch 'upstream/main' into binaryops
8448b4b
@iritkatriel
PyLong_FromLong --> PyLong_FromSsize_t
634b388
@chris-eibl chris-eibl mentioned this pull request Jan 21, 2025
Missing fast path in PyLong_From*() functions for compact integers #129149
Closed
markshannon
markshannon reviewed Jan 22, 2025
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.

It looks like we are seeing a high proportion of specialization failures for non-compact ints with the & operator.
I don't know why that would be. My guess is that some of the benchmarks are using ints as bit vectors and using more than one digit.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Python/specialize.c

LOOKUP_SPEC(compactlong_float_specs, oparg);
LOOKUP_SPEC(float_compactlong_specs, oparg);
LOOKUP_SPEC(compactlongs_specs, oparg);

@markshannon markshannon Jan 22, 2025

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.

Why three tables, rather than one?

Sorry, something went wrong.

Uh oh!

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

All reactions

@iritkatriel iritkatriel Jan 22, 2025

Copy link
Copy Markdown
Member Author

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.

A single table would need to have a list of (guard, action) pairs for each OP. So it's a table of tables. Same thing basically.

Sorry, something went wrong.

Uh oh!

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

All reactions
@chris-eibl chris-eibl mentioned this pull request Jan 22, 2025
gh-129149: Add Missing fast path in PYLONG_FROM_UINT macro for compact integers #129168
Merged
@chris-eibl

chris-eibl commented Jan 22, 2025

Copy link
Copy Markdown
Member

It looks like we are seeing a high proportion of specialization failures for non-compact ints with the & operator. I don't know why that would be. My guess is that some of the benchmarks are using ints as bit vectors and using more than one digit.

Just a wild guess: could those be from enums, which derive from int? Especially flag enums?

All reactions

Sorry, something went wrong.

Uh oh!

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

@iritkatriel

iritkatriel commented Jan 22, 2025

Copy link
Copy Markdown
Member Author

Just a wild guess: could those be from enums, which derive from int? Especially flag enums?

I don't think so. We check for int with PyLong_CheckExact.

chris-eibl 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.

@eendebakpt

eendebakpt commented Jan 22, 2025

Copy link
Copy Markdown
Contributor

I suspect the misses might be bm_pyflate:

https://github.com/python/pyperformance/blob/1d9261a7da8fcaa642a36181db8e7c4a306a1303/pyperformance/data-files/benchmarks/bm_pyflate/run_benchmark.py#L153-L163

Other python constructions that would causes misses (but are not in pyperformance afaics) are uuid.uuid4 (and variants) and bitwise logical operations on the hash of python objects.

All reactions

Sorry, something went wrong.

Uh oh!

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

@iritkatriel

iritkatriel commented Jan 22, 2025

Copy link
Copy Markdown
Member Author

I suspect the misses might be bm_pyflate:

https://github.com/python/pyperformance/blob/1d9261a7da8fcaa642a36181db8e7c4a306a1303/pyperformance/data-files/benchmarks/bm_pyflate/run_benchmark.py#L153-L163

Could be.

https://github.com/faster-cpython/benchmarking/blob/main/results/bm-20250121-3.14.0a4%2B-6476205/bm-20250121-azure-x86_64-iritkatriel-binaryops-3.14.0a4%2B-6476205-pystats-pyflate.md

image image image
All reactions

Sorry, something went wrong.

Uh oh!

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

@iritkatriel

iritkatriel commented Jan 22, 2025

Copy link
Copy Markdown
Member Author

(Would be nice if the report was rendered so that it's easy to see which benchmark contributed to a stat.)

All reactions

Sorry, something went wrong.

Uh oh!

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

@iritkatriel
Merge branch 'main' into binaryops
6041025
@markshannon

markshannon commented Jan 27, 2025

Copy link
Copy Markdown
Member

Do we have benchmarking numbers for this?

All reactions

Sorry, something went wrong.

Uh oh!

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

@iritkatriel

iritkatriel commented Jan 28, 2025 •
edited by mdboom
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

Do we have benchmarking numbers for this?

https://github.com/faster-cpython/benchmarking-public/tree/main/results/bm-20250121-3.14.0a4%2B-6476205

(mdboom edited URL to the public one)

All reactions

Sorry, something went wrong.

Uh oh!

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

@markshannon

markshannon commented Jan 28, 2025

Copy link
Copy Markdown
Member

Performance looks neutral within the noise.

Were we expecting a speedup on any particular benchmark, or is there a micro-benchmark that shows a speedup?

All reactions

Sorry, something went wrong.

Uh oh!

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

@iritkatriel

iritkatriel commented Jan 29, 2025

Copy link
Copy Markdown
Member Author

Performance looks neutral within the noise.

Were we expecting a speedup on any particular benchmark, or is there a micro-benchmark that shows a speedup?

Here are some microbenchmark numbers:

Old:

>>> for i in range(5):
...     timeit("for i in range(10000):\n\tb = a&i", number=10000, setup="a = 1")
...     
2.517115215305239
2.476386756170541
2.484560175333172
2.4904085099697113
2.4953759447671473

New:

>>> for i in range(5):
...     timeit("for i in range(10000):\n\tb = a&i", number=10000, setup="a = 1")
...     
2.2215038347058
2.1580611928366125
2.1611814140342176
2.154480631928891
2.1730910362675786
mdboom reacted with rocket emoji
All reactions
  • 🚀 1 reaction

Sorry, something went wrong.

Uh oh!

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

@markshannon
markshannon self-requested a review January 29, 2025 09:24
markshannon
markshannon approved these changes Jan 29, 2025
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.

My interpretation of the results is:

  • Shows a measurable speedup for the operators and classes it targets
  • Doesn't show an overall slowdown
  • This specialization's effectiveness is limited by the small range of compact ints. Something like faster-cpython/ideas#548 should mostly fix that.

@iritkatriel do you agree?

If so, let's merge this.

Sorry, something went wrong.

Uh oh!

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

iritkatriel reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Jan 29, 2025
@iritkatriel
iritkatriel merged commit 4815131 into python:main Jan 29, 2025
@bedevere-app bedevere-app Bot removed the awaiting merge label Jan 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eendebakpt eendebakpt eendebakpt left review comments
@kumaraditya303 kumaraditya303 kumaraditya303 left review comments
@chris-eibl chris-eibl chris-eibl left review comments
@markshannon markshannon markshannon approved these changes
@mdboom mdboom Awaiting requested review from mdboom
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

Uh oh!

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

Assignees

No one assigned

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.

5 participants

@iritkatriel @chris-eibl @eendebakpt @markshannon @kumaraditya303
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.