This introduces a global cache. It's a welcome change that makes custom caches - a major source of bugs and complexity - unnecessary, but IMO it requires an interface rework.
The key here is that a cache hit is so much cheaper than a cache miss that the user must be constantly aware of the cache contents to get good performance. The current interface is an attempt to make working with cache transparent, which makes it very easy to write code that performs poorly.
get() checks the cache, while find*() methods do not and are always expensive to call. The only place where the user might learn that is documentation/docstrings, and the cost for using the wrong method is high.
get() calls can be expensive too, and it's difficult to predict that now. Sometimes that can be deduced from the call: e.g. a get(Entity, {relations: {someOneToManyField: true}} is always a cache miss, and therefore expensive. But most cache misses are due to just not knowing what's in the cache right now.
The store never warns the user about cache misses. It's also very easy to write a batch processor where every get() is a cache miss. This will perform as bad as a subgraph, and do so silently.
Recommendations:
Visibly separate the caching and non-caching access methods, e.g.
...
cachedGet()
directFind()
directFindBy()
...
Bring back deferred entities from the old @belopash/typeorm-store. Calling a get() from a deferred entity is a guaranteed cache hit (with properly structured code and with the exception of the first call):
const deferredEntity = ctx.store.defer(Entity, id)
queue.push(() => {
deferredEntity.get() // guaranteed hit when the queue executes
}
get()/defer() should refuse to work when the cache miss is guaranteed (e.g. when a one-to-many relation is requested). Other situations when a cache miss can be predicted early should be treated in the same way.
Store should count cache misses per entity and complain loudly when this number looks like it grows linearly with the batch size. It's kinda hard to detect unfortunately, but something like >10 cache misses in a 100+ blocks batch on any given entity is a red flag.
Other notes:
flush() and reset() methods are named in a counterintuitive fashion: flush() in a caching module is not a cache flush, but a DB-related operation. reset() does what 99% of users will expect flush() to do.
The new store behavior settings are hard to interpret. Would be nice to simplify them somewhat.
This introduces a global cache. It's a welcome change that makes custom caches - a major source of bugs and complexity - unnecessary, but IMO it requires an interface rework.
The key here is that a cache hit is so much cheaper than a cache miss that the user must be constantly aware of the cache contents to get good performance. The current interface is an attempt to make working with cache transparent, which makes it very easy to write code that performs poorly.
Here are some examples:
The updated store exposes the following methods:
get()checks the cache, whilefind*()methods do not and are always expensive to call. The only place where the user might learn that is documentation/docstrings, and the cost for using the wrong method is high.get()calls can be expensive too, and it's difficult to predict that now. Sometimes that can be deduced from the call: e.g. aget(Entity, {relations: {someOneToManyField: true}}is always a cache miss, and therefore expensive. But most cache misses are due to just not knowing what's in the cache right now.The store never warns the user about cache misses. It's also very easy to write a batch processor where every
get()is a cache miss. This will perform as bad as a subgraph, and do so silently.Recommendations:
@belopash/typeorm-store. Calling aget()from a deferred entity is a guaranteed cache hit (with properly structured code and with the exception of the first call):get()/defer()should refuse to work when the cache miss is guaranteed (e.g. when a one-to-many relation is requested). Other situations when a cache miss can be predicted early should be treated in the same way.Other notes:
flush()andreset()methods are named in a counterintuitive fashion:flush()in a caching module is not a cache flush, but a DB-related operation.reset()does what 99% of users will expectflush()to do.The new store behavior settings are hard to interpret. Would be nice to simplify them somewhat.