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

typeorm: typeorm-store rework #293

Open
wants to merge 11 commits into
base: beta
Choose a base branch
from
Open

Conversation

belopash
Copy link
Contributor

No description provided.

@belopash belopash force-pushed the feat/typeorm-store-cache branch 2 times, most recently from 0915830 to b44249d Compare May 27, 2024 19:53
@belopash belopash force-pushed the feat/typeorm-store-cache branch from b44249d to 87edb33 Compare May 27, 2024 19:56
@belopash belopash requested review from vanruch and abernatskiy May 28, 2024 15:01
	modified:   src/database.ts
	modified:   src/store.ts
	modified:   src/test/store.test.ts
@abernatskiy
Copy link
Contributor

abernatskiy commented Jun 10, 2024

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:

  1. The updated store exposes the following methods:

    ...
    get(e: EntityType, id: string | options)
    find(e: EntityType, options)
    findBy(e: EntityType, options)
    ...
    

    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.

  2. 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.

  3. 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:

  1. 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.

  2. The new store behavior settings are hard to interpret. Would be nice to simplify them somewhat.

@belopash belopash force-pushed the feat/typeorm-store-cache branch from 55a4dd0 to 77ace58 Compare June 13, 2024 17:43
@belopash belopash changed the base branch from master to beta July 23, 2024 08:44
@belopash belopash force-pushed the beta branch 2 times, most recently from f0b16d8 to 7ee0a60 Compare July 29, 2024 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants