LRU cache for the entity manager #4
Conversation
| @@ -183,7 +183,7 @@ where | |||
| ) | |||
There was a problem hiding this comment.
adapter.produce may return many records, right? That means that after above while ... update_entity it's not guaranteed that the entities cache contains the entity_id. Shouldn't produce be changed so that only loads the requested entity_id, nothing more?
Also, if I'm not reading things completely wrong the current implementation of FileLogTopicAdapter.produce just returns an empty stream.
Adding some test that validates that the state is recovered also after cache eviction might be good.
There was a problem hiding this comment.
adapter.producemay return many records, right?
Not anymore. I changed that contract when the initial produce was introduced in the first PR. It is now allowed only to produce those for the entity id.
There was a problem hiding this comment.
Also, if I'm not reading things completely wrong the current implementation of
FileLogTopicAdapter.producejust returns an empty stream.
Ah yeah, I now need to fix that given the LRU cache.
There was a problem hiding this comment.
Adding some test that validates that the state is recovered also after cache eviction might be good.
I will do so.
There was a problem hiding this comment.
I've provided 3 more commits starting at d4edc4d. There's pretty good test coverage now, and a few optimisations.
There was a problem hiding this comment.
...and produce now produces events per entity id.
The entity manager now uses a simple and evolved LRU cache given that we're in control of the concurrency aspects.
Implement produce for the commit log adapter and provide some test coverage. Also, use the NonZeroUsize type to express capacity, inline with what the cache requires.
Eliminate cloning by using references for the entity id. Should result in a massive speed up.
| /// are no events sourced, or if it’s a new entity with a previously unused EntityId. | ||
| /// Any required side effects should be performed once recovery has completed by | ||
| /// overriding this method. | ||
| async fn on_recovery_completed(&self, _context: &Context, _state: &Self::State) {} |
| if let Ok(mut records) = records { | ||
| Ok(Box::pin(stream!({ | ||
| while let Some(record) = records.next().await { | ||
| if &record.entity_id == entity_id { |
There was a problem hiding this comment.
An optimization could be if we could read the record entity_id without deserializing the payload. Then it would be more efficient to scan for a single entity_id.
There was a problem hiding this comment.
Ah yeah - that's a good idea. I'll do that as a separate PR.
The entity manager now uses a simple and evolved LRU cache given that we're in control of the concurrency aspects (as opposed to using a cache supporting concurrency).
I also upped the working set (capacity) to 10. We of course still permit the developer to override that, and I've not forgotten about the potential for a config struct as we move forward.