-
-
Notifications
You must be signed in to change notification settings - Fork 75
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
Micro-optimize ValueInitializer #331
Conversation
Some other interesting observations while refactoring this code:
|
Very nice! Thank you.
I tried to remove I will run some tests after work to check if your change works as expected. |
Thanks. I merged #330 but you do not have to rebase. I changed the target branch of this pull request (PR) to So next time, you will simply create a PR targeting another topic branch rather than |
You are right. Mixing different I will open an issue for it. I guess we can solve it by adding the |
This has the following changes/optimizations: - Avoids a needless incref/decref on `waiters`. - Avoids unconditionally cloning the waiter key by moving it to the `WaiterGuard`. - Use an `Option` in the `WaiterGuard` instead of a manual bool. - Move the own `waiter` allocation out of the loop. - Un-indent the loop by using an let-else. - This also clarifies that no looping happens when our waiter was inserted.
e16da48
to
edcc240
Compare
I added an explicit test to make sure all the futures in the public API are While going through all the public API, I noticed that maybe this function is not supposed to be public? |
Thanks!
Ah. You are right. Can you do me a favor? Can you please change it to private in this PR? |
Done. While this is in theory a breaking change, I don’t think anyone will really care :-) |
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.
Thanks. It looks good to me.
I am running mokabench with different command line arguments (on the commit edcc240). It has been 1.5 hours since started and I have not seen any issues.
This has the following changes/optimizations:
waiters
.WaiterGuard
.Option
in theWaiterGuard
instead of a manual bool.waiter
allocation out of the loop.Based on top of #330, I will rebase once that lands.