-
Notifications
You must be signed in to change notification settings - Fork 6
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
Optimize code paths #23
Conversation
Converting the alphabet to a list is very costly at scale. Getting the length of the alphabet repeatedly is a little costly. Comparing `result == 0` vs `not result` is measurably costly. These have all been eliminated. Python's timeit module suggest a performance improvement of ~300%.
Previous behavior required checking the entire list even if the first number is invalid.
By filtering the blocklist once during instantiation, a significant amount of computation can be eliminated when the same instance is reused over and over. This additionally updates the hypothesis testing; generated IDs are now confirmed to be blockable.
Hi @kurtmckee, Thank you for the recent changes and this PR as well 💪 Questions:
|
I think that the average user isn't going to notice this.
This only pays off at scale, or for large bulk operations, but there's no route to be speedy prior to this change. If you're open to a transformation in how the blocklist is stored in the project, then it's possible to skip filtering of the default blocklist at instantiation for all installations everywhere, similar to the Ruby PR (at least as I'm reading it). That's an important metric, too, but I'd need the go-ahead to make that change, since there's an administrative burden when updating the global blocklist. |
Fair enough: using it once is minor disadvantage, but at scale it's big advantage. I agree.
I am - to how it's stored in this library. The spec's blocklist is an unordered list, and individual implementations are free to optimize the order or chunk it as needed. Perhaps it's worth creating a small script here to transform it, so that the next time it changes on the spec level it will be easy to update? Other than that, I like the optimizations and will be happy to merge. Thank you for adjusting the tests as well! Edit: Maybe a script is an overkill? Some LLM can probably adjust the list as needed. |
@4kimov We were thinking along the same lines! I wrote a script to update the I think this will reduce the administrative burden of maintaining the blocklist and keeping instantiation fast for the default blocklist. Here's the new performance results, which reflects using the default blocklist during instantiation:
Thanks for pointing out the Ruby PR's take on performance improvements! That was insightful. |
4419b95
to
7c4ef18
Compare
@kurtmckee Thank you for all the work and numerous PRs. Very cool optimizations indeed! 💪 And @Pevtrick thanks for the quick merges when I wasn't around! |
You're welcome! This has been a lot of fun! |
That's because I forgot about it :) I've pushed it now. |
This PR significantly increases the speed of encoding by optimizing blocklist checking, which were very expensive. It also adds a simple performance testing script.
The performance improvements are made possible by pre-filtering the blocklist into groups:
This pre-filtering allows blocklist checks to eliminate almost all looping in Python.
Here is the output of the performance testing for the
main
branch before this PR, and for this PR branch:As you can see, IDs can be encoded ~85% faster. Although it's not reflected in these performance tests, if a non-default alphabet or blocklist is used, instantiation will require more up-front computation to filter the blocklist, but the encoding will still be faster.
Decode times are not affected by these changes.