-
Notifications
You must be signed in to change notification settings - Fork 346
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
improve random string performance #344
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Take a look at this:
Just calling the unique_string once calls |
Whoaaa, nice find! I've reviewed the PR code and it looks good. The only tests failing are the 1.0.0 compatibility ones which seems to be caused by some changes to github actions - definitely not a blocker. I'll merge it now. Thanks a lot 🙇 |
Thank you very much! Can you publish a new version with this? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm am running a worker which is trying to move over a fair bit of data from one location to a second one. This consists of a large number of small files. Tesla is somewhere in the dependencies, and I discovered that more than 50% of my cpu time is right now being spent generating random numbers for multipart. I created faster version to do more-or-less the same, and as for this script:
The current version takes 872331ms to finish, while new version I'm submiting takes 1819ms, so is ~479 times faster.
If it is an issue that new version generates random strings from a smaller number of characters, I can adjust, but it looks good enough, if I might say so.
Please note that this is not an academic exercise, I am running into a serious performance bottleneck with this specific method.
EDIT:
I don't quite understand why this test is failing, I'm happy to collaborate on it. All tests pass locally for me.