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

Porting argclinic socket.__init__ from Cinder. #92210

Closed
zitterbewegung opened this issue May 3, 2022 · 3 comments
Closed

Porting argclinic socket.__init__ from Cinder. #92210

zitterbewegung opened this issue May 3, 2022 · 3 comments
Labels
type-feature A feature request or enhancement

Comments

@zitterbewegung
Copy link
Contributor

zitterbewegung commented May 3, 2022

Enhancement of socket.init to use argument clinic

In cinder this enhancement that is relatively easy involves adding argument clinic to socket.init . The primary goal will make this much easier to maintain.

@zitterbewegung zitterbewegung added the type-feature A feature request or enhancement label May 3, 2022
@MojoVampire
Copy link
Contributor

Coverage of argument clinic is not a goal. As for performance, the overhead of a slightly slower path to calling socket's initializer is trivial relative to the cost of actually opening a socket; optimizing away even 10% of the cost would surprise me, and while that would be worth it if it were truly a super-hot code path, in practice, I doubt even high performance web servers would be spending an appreciable fraction of their time on just creating sockets.

There's nothing wrong with improving the module as a whole to use argument clinic (it's nice to maintain less argument parsing code, while getting speedups for free when argument clinic improves), but the savings available from switching from using tp_new/tp_init to using a tp_vectorcall is mostly just avoiding the creation and parsing of a couple dicts; worth it for built-ins that get exercised a lot and are only limited by the CPU, not so much for stuff that gets used less and is typically I/O limited.

@zitterbewegung
Copy link
Contributor Author

Thanks @MojoVampire I have revised my issue from your comment.

@zitterbewegung zitterbewegung changed the title Porting use of argclinic socket.__init__ from Cinder. Porting argclinic socket.__init__ from Cinder. May 3, 2022
JelleZijlstra added a commit to zitterbewegung/cpython that referenced this issue May 4, 2022
JelleZijlstra added a commit that referenced this issue May 4, 2022
Co-authored-by: Łukasz Langa <lukasz@langa.pl>
Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@protonmail.com>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@JelleZijlstra
Copy link
Member

Thanks for your patch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

3 participants