-
Notifications
You must be signed in to change notification settings - Fork 8
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
Major performance improvement achieved through fixing clippy warnings #35
base: master
Are you sure you want to change the base?
Conversation
…se gloo-utils instead now.
Running clippy in CI to ensure it doesn't break again. Forcing clippy to pass without any warnings. Fixed warnings added by newer rustc.
@jstewmon any chance you can give this one a quick look and perhaps merge it into a new version? This is a very significant performance gain compared to the current stable version, with little to no risk IMO |
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 tested the pull request with some examples and everything is running fine.
So looks good to me! 👍
Unfortunately I haven't the rights to merge this work.
Can you do it @jstewmon or @mplanchard when you have time? 🙏
Thank you ! 🤟
Hi, thank you for your interest in contributing. ❤️ Sorry for the slow response; we don't have much capacity to service this project currently. Thank you for making a few separate commits - I was able pick the I'd be interested in accepting the clippy-related changes if you can parcel them out into a couple of separate PRs, so that I can quickly verify the ones that are automatic fixes and then more carefully review the hand crafted ones. I'd also like to isolate whatever change necessitated the library changes and have a more detailed explanation of why the library change is needed. |
Hi,
I also don't have the capacity for redoing the PR as even more separate
commits. I believe it would be faster for you to run clippy the next time
you do work on the library, than for me to organize it your way. The
ok_or_else was the critical PR, as it sped up the library a hundred fold.
The rest, I did out of curtesy to help keeping the library up to date with
modern Rust. It's ok by me if you leave it unmaintained if it's ok by you.
Your call of course.
Cheers,
Avishay
…On Tue, Jul 2, 2024, 01:58 Jonathan Stewmon ***@***.***> wrote:
Hi, thank you for your interest in contributing. ❤️ Sorry for the slow
response; we don't have much capacity to service this project currently.
Thank you for making a few separate commits - I was able pick the
ok_or_else changes while doing some work in this repo today. 🙌
I'd be interested in accepting the clippy-related changes if you can
parcel them out into a couple of separate PRs, so that I can quickly verify
the ones that are automatic fixes and then more carefully review the hand
crafted ones. I'd also like to isolate whatever change necessitated the
library changes and have a more detailed explanation of why the library
change is needed.
—
Reply to this email directly, view it on GitHub
<#35 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEAXZM5MHL3HCYGIVRYSHKLZKHNJLAVCNFSM6AAAAABGCDYY2WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMBRGMZDANJVGM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Most notable performance problem was the code used
Result::ok_or()
instead ofResult::ok_or_else(||)
, thus causing the creation of the error strings / objects regardless of whether there was an error or not. This reducedParsed::from_value()
times about 50 fold.I then followed to fix all clippy warnings, and to add a
cargo clippy
step to theci.yaml
build section, so that we ensure clippy is happy in the future too.