Skip to content
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

Merged
merged 5 commits into from
Oct 3, 2023
Merged

Conversation

Swatinem
Copy link
Contributor

@Swatinem Swatinem commented Oct 2, 2023

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.

Based on top of #330, I will rebase once that lands.

@Swatinem
Copy link
Contributor Author

Swatinem commented Oct 2, 2023

Some other interesting observations while refactoring this code:

  • The ignore_if closure is never used/executed if we get a fresh value from an existing waiter.
  • Now that I understand the TypeId usage, it is clear to me now that it is used to facilitate the try_ and optionally_ family of accessors.
    However that also means that mixing different kinds of accessors means that they are concurrently executing their respective init closure. That also means that you might get inconsistent results.

@tatsuya6502 tatsuya6502 changed the base branch from main to fix-get-with-in-future-cache October 3, 2023 01:53
@tatsuya6502 tatsuya6502 changed the base branch from fix-get-with-in-future-cache to main October 3, 2023 01:53
@tatsuya6502
Copy link
Member

Very nice! Thank you.

The ignore_if closure is never used/executed if we get a fresh value from an existing waiter.

I tried to remove Arc<Mutex<..>> from ignore_if before, but gave up as the compiler complained the resulting Future of try_init_or_read was not Send and Sync. I do not remember details but maybe I had &mut ignore_if instead of mut ignore_if because it was in the loop? I am glad you found the solution!

I will run some tests after work to check if your change works as expected.

@tatsuya6502
Copy link
Member

Based on top of #330, I will rebase once that lands.

Thanks. I merged #330 but you do not have to rebase.

I changed the target branch of this pull request (PR) to fix-get-with-in-future-cache and it eliminated the extra commit histories and diffs. Then I changed the target back to main, but my colleague said I did not have to do it. He said: when #330 was merged into main, the target branch of this PR should have automatically been changed to main.

So next time, you will simply create a PR targeting another topic branch rather than main. Then GitHub will take care of the rest; it will automatically change the target branch of the PR to main when the target branch is merged in to main.

@tatsuya6502
Copy link
Member

  • Now that I understand the TypeId usage, it is clear to me now that it is used to facilitate the try_ and optionally_ family of accessors.
    However that also means that mixing different kinds of accessors means that they are concurrently executing their respective init closure. That also means that you might get inconsistent results.

You are right. Mixing different init closures will lead unexpected result.

I will open an issue for it. I guess we can solve it by adding the TypeId of init closure/future to the waiter key? The compiler will create an opaque type for init closure/future, and if you have two different init codes, I think they will have different TypeIds.

@tatsuya6502 tatsuya6502 self-assigned this Oct 3, 2023
@tatsuya6502 tatsuya6502 added the enhancement New feature or request label Oct 3, 2023
@tatsuya6502 tatsuya6502 added this to the v0.12.1 milestone Oct 3, 2023
@tatsuya6502 tatsuya6502 self-requested a review October 3, 2023 05:57
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.
@Swatinem
Copy link
Contributor Author

Swatinem commented Oct 3, 2023

I tried to remove Arc<Mutex<..>> from ignore_if before, but gave up as the compiler complained the resulting Future of try_init_or_read was not Send and Sync. I do not remember details but maybe I had &mut ignore_if instead of mut ignore_if because it was in the loop? I am glad you found the solution!

I added an explicit test to make sure all the futures in the public API are Send at least.
However they are not Sync, neither on master.

While going through all the public API, I noticed that maybe this function is not supposed to be public?
https://docs.rs/moka/latest/moka/future/struct.Cache.html#method.invalidate_with_hash

@tatsuya6502
Copy link
Member

I added an explicit test to make sure all the futures in the public API are Send at least. However they are not Sync, neither on master.

Thanks!

While going through all the public API, I noticed that maybe this function is not supposed to be public? https://docs.rs/moka/latest/moka/future/struct.Cache.html#method.invalidate_with_hash

Ah. You are right. Can you do me a favor? Can you please change it to private in this PR?

@Swatinem
Copy link
Contributor Author

Swatinem commented Oct 3, 2023

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 :-)

Copy link
Member

@tatsuya6502 tatsuya6502 left a 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.

@tatsuya6502 tatsuya6502 added this pull request to the merge queue Oct 3, 2023
Merged via the queue into moka-rs:main with commit 616cb6c Oct 3, 2023
@Swatinem Swatinem deleted the opt-waiterguard branch October 3, 2023 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants