Skip to content

Introduce support for caching.#55

Merged
fr33r merged 19 commits into
masterfrom
freer/introduce-identity-map
Oct 3, 2022
Merged

Introduce support for caching.#55
fr33r merged 19 commits into
masterfrom
freer/introduce-identity-map

Conversation

@fr33r
Copy link
Copy Markdown
Contributor

@fr33r fr33r commented Nov 15, 2021

Description

Takes on #24!

Rationale

This feature unlocks the ability to prevent loading the same object (entity) from the applications data store. At the same call sites as #Register, consumers of this package can now check whether the entity / entities being retrieved have already been retrieved and not been acted upon (also referred to as "dirty").

For smaller applications, this ability may seem unnecessary, but as a codebase becomes larger and more complex, there typically arises a need to ensure particular objects are not loaded multiple times unnecessarily.

Callouts

Careful consideration was made into the approach for accomplish this. When assessing the pros and cons of whether or not to enforce an interface in the method signatures, it made more sense to instead leverage type assertions on provided entities instead. The following are the tradeoffs worth noting:

  • The proposed approach of type assertions allows multiple interfaces for determining the entities identifier to be supported, which solves the problem of consumers having pre-existing clashes with their codebases, and additionally enables us to introduce more interfaces without breaking the API.
  • Using type assertions along with unexported interfaces enables consumers to reap the benefits of caching if desired, while allowing them to continue using familiar APIs if they don't care for caching.
  • Caching is typically viewed as an optimization, not a deal breaker - especially in terms of APIs. Imposing a compile time constraint on consumer code such that it can be used for caching-related operations is heavy handed for an optimization focused feature.
  • Consumers can be made aware via logging of instances where entities they are tracking via work units that don't support caching.
  • We need to ensure that consumers don't get left behind on previous major versions (such as V3) for features that we fundamentally feel are options (caching). Not all users will care about caching, and imposing code changes to their entities could prove daunting for large codebases.

work.UnitCache AKA unit.Cache

My initial swing at this had the Cached method returning map[work.TypeName][]interface{}. However, there were some challenges with this approach:

  • The burden of traversing the cache was completely on the consumer. It would look something like the following:
func findEntity(unit work.Unit, id interface{}) (result Foo) {
  cached := unit.Cached()
  if entities, ok := cached[work.TypeNameOf(result)]; ok {
    for _, entity := range entities {
      if result, ok = entity.(Foo); ok && f.ID() == id {
        return
      }
    }
  }
  // ... retrieve from DB ...
}
  • It misses the opportunity to additionally store entities by ID, which then in turn makes cache retrieval O(1) instead of O(N), where N is the number of cached entities of a particular type.
  • Returning a dedicated type (work.UnitCache) provides additional flexibility, as it enables us to expand operations / simplifications for interacting with the cache in a backwards compatible way.

Suggested Version

v4.0.0-beta.4

Example Usage

(psuedocode)

func findEntity(unit work.Unit, id interface{}) (result Foo) {
  cached := unit.Cached()
  if entity, ok := cached.Load(work.TypeNameOf(result), id); ok {
    if result, ok = entity.(Foo); ok {
      return
    }
  }
  // ... retrieve from DB ...
}

@fr33r fr33r marked this pull request as ready for review September 16, 2022 04:48
@fr33r fr33r self-assigned this Sep 16, 2022
@fr33r fr33r mentioned this pull request Sep 16, 2022
@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 20, 2022

Codecov Report

Base: 96.45% // Head: 96.57% // Increases project coverage by +0.12% 🎉

Coverage data is based on head (539c884) compared to base (f79a34e).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #55      +/-   ##
==========================================
+ Coverage   96.45%   96.57%   +0.12%     
==========================================
  Files          13       14       +1     
  Lines        1296     1345      +49     
==========================================
+ Hits         1250     1299      +49     
  Misses         34       34              
  Partials       12       12              
Flag Coverage Δ
v3 98.71% <ø> (ø)
v4 94.72% <100.00%> (+0.38%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
v4/unit_options.go 99.41% <ø> (ø)
v4/unit.go 100.00% <100.00%> (ø)
v4/unit_cache.go 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@fr33r fr33r marked this pull request as draft September 21, 2022 17:50
@fr33r fr33r marked this pull request as ready for review September 21, 2022 22:53
@fr33r fr33r marked this pull request as draft September 22, 2022 17:43
@fr33r fr33r marked this pull request as ready for review October 3, 2022 02:59
@fr33r fr33r merged commit 339bf8e into master Oct 3, 2022
@fr33r fr33r deleted the freer/introduce-identity-map branch October 3, 2022 03:08
Comment thread v4/unit_cache.go
Comment thread v4/unit.go

// Cached provides the entities that have been previously registered
// and have not been acted on via Add, Alter, or Remove.
Cached() *UnitCache
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could an interface be used here?
That gives flexible replacing sync.Map cache on Redis or an another provider in a distributed application.

Copy link
Copy Markdown
Contributor Author

@fr33r fr33r Oct 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a great question and point. it's something i pondered on for a bit when designing this.

after careful consideration, i actually felt it was possible to support accepting arbitrary cache providers even though we return a struct. additionally, i wanted to keep the door open to being able to expand the functionality of UnitCache without it having to be a breaking change.

as this PR was written, UnitCache is essentially a wrapper around sync.Map, but really what it should be is a wrapper around an interface that consumers adhere to when they pass in their arbitrary cache clients.

for now i just went with the sync.Map route, but your comment here has helped confirm that i should go ahead and bite off the work to allow consumers to pass in various cache clients as options, and simply use sync.Map as the default. i will go ahead and slot in that work.

as an example for where i'm heading:

// input interface to handle arbitrary clients...
type UnitCacheClient interface {
  Put(key, value interface{}
  Load(key interface{}) (interface{}, bool)
}

// options...
var (
  ...

  // consumers would write adaptors that adhere to UnitCacheClient.
  // we could even provide some out of the box to make things easier.
  CacheClient = func(c UnitCacheClient) UnitOption {
    return func(o *UnitOptions) {
      o.CacheClient = c
    }
  }

  ...
)

// default cache client
var UnitCacheClientDefault = &mapCacheClient{}

type UnitCache struct {
  client UnitCacheClient
}

and then internally (within work.UnitCache) we would instead wrap work.UnitCacheClient instead of sync.Map.

Comment thread v4/unit_cache.go
Comment thread v4/unit_cache.go
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.

2 participants