-
Notifications
You must be signed in to change notification settings - Fork 653
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
Use Store Cache and merge optimistic cache with Memory cache #5651
Conversation
✅ Deploy Preview for apollo-android-docs canceled.
|
...rc/jsMain/kotlin/com/apollographql/apollo3/cache/normalized/api/internal/ConcurrentMap.js.kt
Outdated
Show resolved
Hide resolved
...sMain/kotlin/com/apollographql/apollo3/cache/normalized/api/internal/ConcurrentMap.wasmJs.kt
Outdated
Show resolved
Hide resolved
result = result && remove(CacheKey(cacheReference.key), true) | ||
result = result || remove(CacheKey(cacheReference.key), true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to say this looks like it needs to be backported to non-incubating but sounds like the whole semantics of the return value need to be refined here, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah the &&
didn't make much sense to me but if so the doc should also be updated to be precise:
- * @return `true` if a record with such key was successfully removed, `false` otherwise
+ * @return `true` if at least one record was successfully removed, `false` otherwise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes 👍 Do we have a list of such API tweaks we need to go through?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet - I could open a dedicated ticket
var size = SIZE_OF_RECORD_OVERHEAD + record.key.utf8Size().toInt() | ||
var size = SIZE_OF_RECORD_OVERHEAD + record.key.length | ||
for ((key, value) in record.fields) { | ||
size += key.utf8Size().toInt() + weighField(value) | ||
size += key.length + weighField(value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reasoning for changing the weighter implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes sorry this is a totally unrelated to this PR! I just noticed this and thought in memory strings are stored either in utf-16 or some sort of compact form that's iso-latin-1 if possible (apparently since JDK9 - I can't tell if this applies to Android). I think it's never utf-8. But length*2
may be a better value if we think it's probably utf-16.
And now the fun question: what is it on apple/js/wasm? 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
O(length) is probably the only cross-platform answer... So if we want that to be correct I'm guessing we need per-platform weighter.
My hunch is that the long term solution is to memory cache btree tables more than Record
this way the same computation applies to both in-memory and persistent.
...cubating/src/commonMain/kotlin/com/apollographql/apollo3/cache/normalized/api/MemoryCache.kt
Show resolved
Hide resolved
val dump = apolloStore.dump().filterKeys { | ||
// Ignore optimistic cache for comparison | ||
it != OptimisticNormalizedCache::class && it != OptimisticNormalizedCacheWrapper::class |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason those might be diferent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MemoryCache
uses OptimisticNormalizedCache::class
as a key in its dump()
fun. Maybe not the most intuitive thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha 👍 Yea, not the best API. Add it to the list of APIs to bikeshed?
...ommonMain/kotlin/com/apollographql/apollo3/cache/normalized/api/internal/store/LocalCache.kt
Show resolved
Hide resolved
…Cache and OptimisticNormalizedCache are now interfaces. Also remove chaining.
…n/kotlin/com/apollographql/apollo3/cache/normalized/api/internal/ConcurrentMap.wasmJs.kt Co-authored-by: Martin Bonnin <martin@mbonnin.net>
…tlin/com/apollographql/apollo3/cache/normalized/api/internal/ConcurrentMap.js.kt Co-authored-by: Martin Bonnin <martin@mbonnin.net>
Merging this for now. I've opened #5689 to circle back if we can use Store as a dependency instead of including it. |
Replace our in-house LRUCache that required guarding with a lock with the
Cache
from the Store project (which is a KMP port of Guava's Cache).Benchmarks after this change looked promising, but we still used a lock upstream inside
OptimisticCache
which reduced these gains.To improve on that, this PR also merges the optimistic cache with the memory cache, and uses a thread-safe map (backed by
ConcurrentHashMap
on the JVM and a regular map behind a lock on other platforms) to store theRecordJournal
s.A few changes to get there:
NormalizedCache
is now an interface, and chaining is removed (MemoryCache
still supports chaining)OptimisticNormalizedCache
is now an interface, implemented byMemoryCache
andOptimisticNormalizedCacheWrapper
(new name for whatOptimisticCache
was)The Store cache was copied to the project, but I'll see if we can depend on it instead - I had to make one addition: the
getAllPresent()
fun, also the wasm target is missing - so opening in draft for now.