-
-
Notifications
You must be signed in to change notification settings - Fork 32.7k
bpo-43014: Improve performance of tokenize by 20-30% #24311
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
Conversation
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.
Great optimization, though there are 2 concerns of mine;
- For people who are not using
tokenize
module to generate tokens (likedetect_encoding
/open
are the most common functions), they'd have to pay this cost - Also, even though breaking them is somewhat OK, there are wild usages out there that monkeypatches the
PseduoToken
to change the behavior (add new tokens) oftokenize
module.
Maybe there is a solution that would both optimize this, and also don't cause any new regressions for normal users (something like @lru_cache
to _compile
maybe?)
I initially approached this with lru_cache, however the function call alone accounts for 6% of the execution so the performance gains aren't as significant |
Maybe we could set it to a global ( |
from my tests this performs the same as the lru_cache approach (within a few 1s of |
bc2dc35
to
2025476
Compare
Thanks a lot @asottile! |
@@ -95,6 +96,7 @@ def _all_string_prefixes(): | |||
result.add(''.join(u)) | |||
return result | |||
|
|||
@functools.lru_cache |
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 think we should limit explicitly (I am aware there is a default of 128) the max size of the cache in this call so this doesn't blow up for some super intensive cases.
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.
In the re
module we use the same trick and we have 512 as the max size of the cache.
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 commented on the other PR, there's only 5 possible regex strings that go through this call
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.
Roger. Then is fine, although I would have preferred to manually pre-compile those and avoid importing functools
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.
that was my original patch, it increased import time of this module by ~20ms
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.
How much is importing functools
? Also, we can still go that way if we do the compilation lazily, but I don't think is worth it.
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 is already imported via tokenize => re => functools
so 0
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.
Oh, then I retract my concern. This is an absolutely better approach. Thanks for following with me :)
https://bugs.python.org/issue43014