-
-
Notifications
You must be signed in to change notification settings - Fork 33.5k
gh-141647: fix IP address/network comparison methods #141842
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
base: main
Are you sure you want to change the base?
Conversation
e88a7c0 to
bd5eeda
Compare
bd5eeda to
ca36ea4
Compare
serhiy-storchaka
left a comment
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.
Please make benchmarks for equality and order comparison.
| raise ValueError("Address negative or too large for IPv6") | ||
|
|
||
|
|
||
| def _check_ip_version(a, b): |
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.
Would not moving this code to a function add an overhead for fast operators?
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 will check how much overhead I'm getting.
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 the benchmark is faster its interesting...
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's likely noise. Benchmarks weren't run on an idle system.
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's likely noise. Benchmarks weren't run on an idle system.
thank you~
|
So I'm a bit surprised but here are the numbers: This benchmark uses the Benchmark scriptimport ipaddress
import itertools
import operator
import pyperf
seen = set()
def bench_eq(runner, label, x, y, seen):
name = f"{label}.__eq__[{x},{y}]"
if name not in seen:
runner.bench_func(name, operator.__eq__, x, y)
seen.add(name)
def bench_lt(runner, label, x, y, seen):
name = f"{label}.__lt__[{x},{y}]"
if name not in seen:
runner.bench_func(name, operator.__lt__, x, y)
seen.add(name)
if __name__ == "__main__":
runner = pyperf.Runner()
args = runner.parse_args()
seen = set()
for x, y in itertools.combinations_with_replacement(
[
ipaddress.IPv4Address("1.2.3.4"),
ipaddress.IPv4Address("5.6.7.8"),
], 2
):
bench_eq(runner, "address", x, y, seen)
bench_lt(runner, "address", x, y, seen)
for x, y in itertools.combinations_with_replacement(
[
ipaddress.IPv4Interface("192.0.2.0"),
ipaddress.IPv4Interface("192.0.8.0"),
ipaddress.IPv4Interface("192.0.2.0/24"),
ipaddress.IPv4Interface("192.0.8.0/32"),
], 2
):
bench_eq(runner, "interface", x, y, seen)
bench_lt(runner, "interface", x, y, seen)
for x, y in itertools.combinations_with_replacement(
[
ipaddress.IPv4Network("192.0.2.0"),
ipaddress.IPv4Network("192.0.8.0"),
ipaddress.IPv4Network("192.0.2.0/24"),
ipaddress.IPv4Network("192.0.8.0/32"),
], 2
):
bench_eq(runner, "network", x, y, seen)
bench_lt(runner, "network", x, y, seen)
for x, y in itertools.combinations_with_replacement(
[
ipaddress.IPv4Address("1.2.3.4"),
ipaddress.IPv4Address("5.6.7.8"),
ipaddress.IPv4Interface("1.2.3.4"),
ipaddress.IPv4Interface("5.6.7.8"),
ipaddress.IPv4Interface("1.2.3.4/24"),
ipaddress.IPv4Interface("5.6.7.8/32"),
], 2
):
bench_lt(runner, "mixed", x, y, seen) |
I think this patch should cover the cases. I'm unsure whether this will break more stuff or not but I do know that using
<and__lt__return different boolean results which can break many things.@serhiy-storchaka I really want a second eye here so PTAL whenever you've got time. TiA!
.versionattribute inipaddressclasses break many methods #141647