Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upbpo-39028: Performance enhancement in keyword extraction #17576
Conversation
seberg
changed the title
This comment has been minimized.
This comment has been minimized.
|
Ah, I missed that this code is used for EDIT: Both can make sense I guess, but for the clinic it is clearly interned I think? So another option is to have two versions here... |
| if (kwname == key) { | ||
| return kwstack[i]; | ||
| } | ||
| } | ||
| /* ptr == ptr should normally find a match in since keyword keys |
This comment has been minimized.
This comment has been minimized.
|
This change LGTM, but please move the comment before the pointers comparison or before the first loop. Although this is almost dead code. It is used in just few sites generated by Argument Clinic and soon will not be used at all. |
This comment has been minimized.
This comment has been minimized.
|
Ah, sorry, the use in |
seberg
force-pushed the
seberg:kwarg-extract-performance
branch
from
ba65512
to
17561c2
This comment has been minimized.
This comment has been minimized.
|
Yeah, although it only will have remotely noticeable effects for >3 kwargs probably, which seems pretty rare in python. Anyway moved and tweaked the comment. |
seberg
changed the title
| } | ||
| /* This function assumes the strings should be interned, so that this | ||
| should only be reached on error, and the loop below will never | ||
| find a match */ |
This comment has been minimized.
This comment has been minimized.
methane
Dec 13, 2019
Member
If we keep the second loop, why we need to have both of find_keyword_interned and find_keyword?
This comment has been minimized.
This comment has been minimized.
seberg
Dec 13, 2019
Author
Contributor
Oh man, sorry, I do not think we do. I was trying around with it a bit, and apparently forgot to commit before pushing or something :/. fixed.
All keywords should first be checked for pointer identity. Only after that failed for all keywords (unlikely) should unicode equality be used. The original code would call unicode equality on any non-matching keyword argument. Meaning calling it often e.g. when a function has many kwargs but only the last one is provided.
seberg
force-pushed the
seberg:kwarg-extract-performance
branch
from
17561c2
to
52562b9
This comment has been minimized.
This comment has been minimized.
|
LGTM. Would you create an NEWS entry? |
methane
merged commit 75bb07e
into
python:master
seberg commentedDec 12, 2019
•
edited by bedevere-bot
All keywords should first be checked for pointer identity. Only
after that failed for all keywords (unlikely) should unicode
equality be used.
The original code would call unicode equality on any non-matching
keyword argument. Meaning calling it often e.g. when a function
has many kwargs but only the last one is provided.
Unless I am missing something big, this seems like it was the original intention, and simple typo. It may make sense to backport?
I have to admit, I have not actually recompiled and timed this, I may do that tomorrow (but I have never had the need to compile python, so need to look up how).
https://bugs.python.org/issue39028