Remove needless traits along with async-trait usage
#445
Conversation
I originally just found a redundant `Arc::clone` within `GetOrInsert::insert`. But then I discovered that `GetOrInsert` only has a single implementor and is thus unnecessary indirection. The similar thing was true for `GetOrRemoveEntry` and `InnerSync`, so I removed those as well. One reason they might have existed was using a trait for code encapsulation, as removing them resulted in a bit more `pub(crate)` visibility, and a bunch more trait bounds all over the place. However, in particular `#[async_trait]` usage was allocating `Box<dyn Future>` and using dynamic dispatch at runtime. Now that overhead is avoided and the compiler should be able to produce better code.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #445 +/- ##
==========================================
- Coverage 95.04% 94.88% -0.17%
==========================================
Files 44 44
Lines 20979 20938 -41
==========================================
- Hits 19939 19866 -73
- Misses 1040 1072 +32 |
There was a problem hiding this comment.
Thank you very much for working on this change! It looks good to me.
For just in case you are still curious why those traits existed, here is a bit of the history:
GetOrRemoveEntry and InnerSync traits were needed by moka prior to v0.12 to meet the trait and lifetime bounds that ScheduledThreadPool (from scheduled-thead-pool crate) required. I do not remember the very details, but it was hard to give a closure having an Arc<base_cache::Inner> to ScheduledThreadPool as Inner had many internal data structures including ones who are !Send. So I created these traits to hide (encapsulate) the internal data structures.
When I worked for moka v0.12 and removed ScheduledThreadPool, I tried to minimize the changes, so I did not touch the traits and their implementations.
I added GetOrInsert trait when I worked on moka v0.12. Some of the ValueInitializer methods used to take closures to get or insert an cache entry. When I had to create an async version of ValueInitializer for v0.12, I simply translated the closures to trait methods as Rust did not support async closures. (Later, I added the remove method to the trait when I added entry().and_compute() method.)
But, GetOrInsert trait was not absolutely necessary and now I think I should have tried what you did; to pass a reference to base_cache::Inner.
I originally just found a redundant
Arc::clonewithinGetOrInsert::insert.But then I discovered that
GetOrInsertonly has a single implementor and is thus unnecessary indirection.The similar thing was true for
GetOrRemoveEntryandInnerSync, so I removed those as well. One reason they might have existed was using a trait for code encapsulation, as removing them resulted in a bit morepub(crate)visibility, and a bunch more trait bounds all over the place.However, in particular
#[async_trait]usage was allocatingBox<dyn Future>and using dynamic dispatch at runtime. Now that overhead is avoided and the compiler should be able to produce better code.