Skip to content

bpo-27860: clean up ipaddress#12774

Closed
methane wants to merge 2 commits intopython:masterfrom
methane:bpo-27860
Closed

bpo-27860: clean up ipaddress#12774
methane wants to merge 2 commits intopython:masterfrom
methane:bpo-27860

Conversation

@methane
Copy link
Copy Markdown
Member

@methane methane commented Apr 11, 2019

Patch by Moritz Sichert.

https://bugs.python.org/issue27860

Lib/ipaddress.py Outdated
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.

What is faster, _BaseNetwork.__init__() or super().__init__()?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In my initial patch I changed this to the super().__init__() on purpose from the version where the base class is mentioned explicitly. Not using the super() version here makes it harder to write subclasses for those classes. Also, I personally think it's better style to use super().

I have not measured it but I don't think that this is a significant speed improvement.

Copy link
Copy Markdown
Member Author

@methane methane Apr 13, 2019

Choose a reason for hiding this comment

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

Not using the super() version here makes it harder to write subclasses for those classes.

Simple subclass is OK without super(). It helps only complicated subclassing like:

class A(IPv4Network):
    def __init__(self, address):
        super().__init__(address)

class B(_BaseNetwork):
    def __init__(self, address)
        super().__init__(address)

class C(A, B):
    def __init__(self, address):
        super().__init__(address)

In this case, if we don't use super() in IPv4Network, __init__ is called for C -> A -> IPv4Network -> _BaseNetwork (B is skipped)
if we use super(), C -> A -> IPv4Network -> B -> _BaseNetwork.

But note that _BaseNetwork is private class.
Not using super() here doesn't make write subclass of IPv4Network harder.

Also, I personally think it's better style to use super().

I'm +0.1 on super in general.

I have not measured it but I don't think that this is a significant speed improvement.

It seems 3% slowdown.

$ ./python -m perf timeit -s 'from ipaddress import IPv4Network' 'IPv4Network("10.10.1.0/24")'
...
Mean +- std dev: [legacy] 8.96 us +- 0.09 us -> [super] 9.23 us +- 0.07 us: 1.03x slower (+3%)

I don't have any strong opinion here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Now there is no __init__ call.

Lib/ipaddress.py Outdated
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.

This seems like a new property as per current docs at https://docs.python.org/3.8/library/ipaddress.html#ipaddress.IPv4Interface . Should this be documented?

@tirkarthi
Copy link
Copy Markdown
Member

@methane This might increase the scope of the PR but there are functions where int(self) is used that can directly use self._ip for noticeable speed boost in https://bugs.python.org/issue25430#msg331931 in case you want to adopt this in the PR or if it's worthy enough I can make a separate PR. There is a PR for optimizing __contains__ check at #1785 that reduces time by 70% .

Explanation from Serhiy on the optimization : https://bugs.python.org/issue25430#msg294442

@methane methane closed this Apr 15, 2019
@methane methane deleted the bpo-27860 branch April 15, 2019 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants