feat: allow for instance specific cache on Provider
#3454
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
CodSpeed Performance ReportMerging #3454 will improve performances by 85.74%Comparing Summary
Benchmarks breakdown
|
maschad
changed the title
Provider
maschad
requested review from
Dhaiwat10,
danielbate,
nedsalk and
petertonysmith94
as code owners
There was a problem hiding this comment.
For me, the ResourceCache is difficult to understand and not entirely extensible. Could we not implement an interface for each caching strategy and have the ResourceCache just a facade over these?
interface CacheStrategy<T> {
all(): Map<string, T>;
ttl(): number;
get(key: string): T;
set(key: string, value: T): void;
clear(): void;
}
class InstanceCache implements CacheStrategy {}
class GlobalCache implements CacheStrategy {}There was a problem hiding this comment.
What specific extensibility needs did you have in mind?
I can appreciate the suggestion for a cleaner abstraction but the core difference between global and instance strategies is really just about where we store the Map - all the implementation logic of the methods(TTL handling, resource management, etc.) remains the same, so I'm not sure it's necessary here.
There was a problem hiding this comment.
It could be something we want to use for the node and chain information. The above approach would also mean we don't check the strategy every time we get the cache.
getActiveCache(): Map<string, CachedResource> {
return this.strategy === 'global' ? this.globalCache.getCache() : this.instanceCache;
}We could just do:
getActiveCache(): Map<string, CachedResource> {
return this.strategy.all();
}There was a problem hiding this comment.
Thanks for the clarification, but I am not sure I see the immediate use case(s) for node / chain info since at the moment this cache is only used for storing transactions.
Also I don't think that the removing the simple boolean check on the strategy field justifies duplicating all the caching logic across two separate classes.
I definitely do think there is validity in storing recently queried node and chain info though :) so that's a good insight, perhaps we should flesh that out in a separate issue? I think it would be more fruitful in exploring if we need a refactor at that point.
There was a problem hiding this comment.
@maschad, I believe what @petertonysmith94 is suggesting is that by having a simple, non-opinionated cache interface to extend from, we can reuse it for any future caching needs.
This would include the Provider cache for chain and node info. Is this correct, @petertonysmith94?
There was a problem hiding this comment.
@Torres-ssf, that is what I was trying to suggest.
There was a problem hiding this comment.
I definitely do think there is validity in storing recently queried node and chain info though :) so that's a good insight, perhaps we should flesh that out in a separate issue?
We have this already:
|
Coverage Report:
Changed Files:
|
| // Only set TTL if not already set or if new TTL is smaller | ||
| if (this.ttl === 0 || ttl < this.ttl) { | ||
| this.ttl = ttl; | ||
| } |
There was a problem hiding this comment.
Why is the TTL only updated if the provided value is lower than the previously set one?
There was a problem hiding this comment.
This is a preventative measure by taking a just conservative approach for a shared cache service. By keeping the smallest TTL it respects stricter freshness requirements.
Here's an example scenario with a global caching strategy:
- User instantiates Provider 1 and Provider 2 with TTL = 60
- User updates Provider 1 to use TTL = 300
If we allow the larger TTL to override the smaller one, then Provider 2 might may have stale data (up to 300 seconds old) which would violate Provider 2's original requirement for 60 second freshness. In this case Provider 2 may reject a 299 second old UTXO for instance, when that wasn't necessarily desired when a user set a 60 second TTL.
I think by taking this approach we give the user less chance of erring.
There was a problem hiding this comment.
@maschad This reasoning sounds counterintuitive and exposes the [negative] nuances of going down this opinionated caching rabbit hole. The interplay between instance and singleton cannot happen like this; it is confusing and would require an in-depth DX reevaluation.
@Torres-ssf @nedsalk I know you guys discussed this in the past, and although I can't remember exactly the context of where it came from, I must repeat my [YAGNI-inspired] statement from the last sync: I don't see a strong use case to justify such a feature right now.
| private readonly ttl: number; | ||
| private readonly strategy: CacheStrategy; | ||
| private instanceCache: Map<string, CachedResource> = new Map(); | ||
| private globalCache: GlobalCache; |
There was a problem hiding this comment.
Why does ResourceCache store an instance of GlobalCache, even though it might not be used when the instance strategy is selected?
| reset() { | ||
| this.clear(); | ||
| this.globalCache.reset(); | ||
| } |
There was a problem hiding this comment.
Will the GlobalCache be used exclusively by the ResourceCache class? I’m asking to ensure that resetting it won’t inadvertently clear data we still depend on.
There was a problem hiding this comment.
Yes, this seems wrong.
If one instantiates two providers, one local and one global, and then I call local.reset(), it will call the global.reset() and inadvertently clear the global cache.
Additionally, if one instantiates the local provider from a place/function that goes out of scope before the global, the latter will also be reset. This will happen because you chose to use Symbol.dispose and call reset from inside it.
fuels-ts/packages/account/src/providers/resource-cache.ts
Lines 123 to 125 in 350dfd0
This case is even worse since users will never manually call .reset() and will still have their cache wiped out.
There was a problem hiding this comment.
When I had designed this the original intention was for consumers to use exclusively a global cache strategy or an instance specific cache strategy, not a combination of both, but you're right in that this isn't programmatically enforced so the above scenario could happen.
There was a problem hiding this comment.
Hey @maschad, good job! 👍
I've left some questions here to better understand your motivations.
|
Thanks for the detailed reviews @FuelLabs/sdk-ts 👍🏾 Looking at the discussions, whilst there may be some improvements here, overall I agree with @arboleya that allowing for this caching strategy may cause more confusion for consumers than benefits offered. I'm going to close this PR as there's no immediate demand for this feature. We can revisit this if it makes sense when there's a stronger use case. |
Release notes
In this release, we:
resourceCacheStrategyparameter on theProviderclass to allow for instance-specific cacheSummary
If one is using multiple Providers and would like to have different TTLs for each of them, one may want to use an instance-specific cache strategy, which means that each
Providerinstance will have its own cache.This introduces that parameter to allow for an alternative to a global cache shared by all provider instances.
Checklist