Open douglas-raillard-arm opened 5 months ago
We definitely need some memory control here, so I think that would be great.
Before we begin, we should have answers to the following questions:
AsyncInMemoryStorage
or into the LFUCache
class)I think there's two aspects to this:
InMemoryStorage
(because memory is usually more limited than disk space), but that could also apply for other storages. After all, disk is not inifinite, either.In fact, even the storage doesn't have complete information about how much space a cached response will really take to store because the cache "backend" (LFUCache
, SQLite, file system) will probably take a bit more space than just the number of bytes of the serialized response. E.g., a file system cache will always take up a multiple of the block size plus the necessary inodes. SQLite needs to integrate it into its index. The dict in the LFUCache
needs to store pointers to the stored responses and an entry in the self.freq_count
dict, etc. So, in a perfect implementation, the storage could ask its backend how much more space will be taken up if a response were stored. Implementing this would probably get pretty complex and maybe brittle.
Since it's only requested here for the InMemoryStorage
and that's probably where it's most relevant, we could implement the following naive version:
InMemoryStorage
two new parameters: max_cached_response_size
and max_cache_size
, which are passed to the LFUCache
exactly like the capacity
is already.LFUCache.put
if the value's size is greater than max_cached_response_size
. (This will require assuming the value's type. Currently, it's a generic type V
, but we need to be sure that it's a primitive type like str
or bytes
, if we want to use sys.getsizeof
.)max_cache_size
. If yes, evict the least frequently used one, like it's happening right now when exceeding capacity
.
capacity
and max_cached_response_size
together effectively also define a max_cache_size
.What do you think? Should I try including a max_cached_response_size
?
Apart from that, maybe we should advise people using Redis that they should set maxmemory
and maxmemory-policy
?
I think it would be reasonable to have something like that:
track the size of data being cached. Trying to account for the actual storage size is doomed as it's impossible to predict. Even if you start bean-counting things like number of inodes, it will be completely thrown off by e.g. compression at fs level. The only sane alternative I can think of would be a back pressure mechanism where the storage reports the free space left. The generic code can take a "measurement" before and after storing each entry, so it knows exactly how big is each. The downside is that it's impossible to predict an entry size before storing it, and polling for available space might be expensive depending on the storage implementation.
Have a hard limit and a headroom threshold. When soft limit (<hard limit> - <headroom>
) is reached, you start evicting. The headroom used to cache the next request. If the next request turns out to be even larger, you can abort caching it mid-way. If you only have a single limit, you will end up having to evict older entries at the same time as processing the current request when it pushes the cache over the limit, or just abort caching the current request until you evicted older entries. That will either cause delays in handling requests and possibly mess with timeouts, or fail to cache some requests "randomly" from user point of view.
Having the storage report the space left might work quite well with an implementation that tries to preserve some headroom, and will adapt to changing conditions in shared storage situation (e.g. a filesystem).
InMemoryStorage
allows controlling the number of cached request but not the size of the cached data. A single GET request with a large file (a few GB) seems to be consuming unreasonable amounts of memory, which cannot be controlled with the number of cached requests.