-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Remove allocation in vtag diff #591
Conversation
Awesome, thanks @tafia, I'll run some benchmarks and see what the improvements are! |
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.
Unfortunately, there wasn't any movement on the benchmark numbers but this is still a great change! I also would have expected this to improve performance though, not sure why :/ I hope to add some automated benchmarking soon which will make it easier to fight this battle :)
After making the changes, can you please rebase to remove the fmt changes you picked up in src/html
? Thanks!
I've applied the suggestions. I am a bit surprised that there is no impact, sorry for having you lose your time! |
Note that I haven't put the
I can put it there if needed. |
No worries at all! Happy for your contribution and would gladly accept more PRs like this one. Sorry for the slow review, looks great!! |
bors r+ |
591: Remove allocation in vtag diff r=jstarry a=tafia **Disclaimer**: I couldn't test it on my system yet. This PR attempts to remove all allocations (`String`, `Vec` and `HashSet`) used in various vtag diff functions. I believe this should have an impact both in terms of performance and memory. Co-authored-by: Johann Tuffe <tafia973@gmail.com>
Build succeeded
|
Trust me I am much slower nowadays! |
Disclaimer: I couldn't test it on my system yet.
This PR attempts to remove all allocations (
String
,Vec
andHashSet
) used in various vtag diff functions.I believe this should have an impact both in terms of performance and memory.