gh-111662: Update socket module to use AC for optimizing performance #111661
Conversation
|
All commit authors signed the Contributor License Agreement. |
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
There was a problem hiding this comment.
Use AC instead of writing manual parsing.
@serhiy-storchaka
Do you think that it's worth changing this code?
If yes, I prefer to update socketmodules to use AC as much as possible for all methods.
| @@ -6396,11 +6396,19 @@ Convert a 32-bit integer from network to host byte order."); | |||
|
|
|||
|
|
|||
| static PyObject * | |||
| socket_htons(PyObject *self, PyObject *args) | |||
There was a problem hiding this comment.
/*[clinic input]
_socket.socket.htons
x: int
/
Convert a 16-bit unsigned integer from host to network byte order.
[clinic start generated code]*/
Use AC instead of writing manual parsing.
I got a similar performance gain in my local environment.
AS-IS: 5000000 loops, best of 5: 60.9 nsec per loop
TO-BE: 10000000 loops, best of 5: 23.3 nsec per loop
There was a problem hiding this comment.
@wrongnull
You should take a look at the https://devguide.python.org/development-tools/clinic/ if you're not familiar with Argument Clinic :)
There was a problem hiding this comment.
I'm pretty sure a similar approach can be applied to all other functions with only one argument. If yes, I'll be happy to make such a commit in this branch.
There was a problem hiding this comment.
@corona10 seems ready. clinic argument was also applied to other functions where it was beneficial
There was a problem hiding this comment.
I'm not sure if ValueError is more preferable if negative value is passed to ntohl and htonl functions
There was a problem hiding this comment.
I'm not sure if ValueError is more preferable if negative value is passed to ntohl and htonl functions
Please don't change the exception type; it will occur in user regression.
Can I know where the type of exception will be changed if AC is applied?
There was a problem hiding this comment.
I'm not sure if ValueError is more preferable if negative value is passed to ntohl and htonl functions
Please don't change the exception type; it will occur in user regression. Can I know where the type of exception will be changed if AC is applied?
Exception type will change to ValueError from OverflowError in htonl and ntohl functions after applying usigned_long argument converter.
There was a problem hiding this comment.
Exception type will change to ValueError from OverflowError in htonl and ntohl functions after applying usigned_long argument converter.
In that case, I would like to recommend not to touch it.
There was a problem hiding this comment.
Exception type will change to ValueError from OverflowError in htonl and ntohl functions after applying usigned_long argument converter.
In that case, I would like to recommend not to touch it.
OK, I reverted this changes
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
I have made the requested changes; please review again |
|
@Eclips4 Thanks a lot |
Currently
socket.htonsuses unnecessaryMETH_VARAGRScalling convention meanwhile it only accepts one argument. This patch will speed this function up by changing calling convention toMETH_OI ran this test before commit
and after