ZDM-522 Add size limit to PS Cache #99
Conversation
alicel
commented
Jan 20, 2023
alicel
commented
- Replaced all three maps in the PSCache struct with the LRU cache provided by Hashicorp.
- Added a configuration variable that sets a default size limit of 5000 for now (the footprint of each element is small so it could be much higher, but applications should not need to create so many prepared statements anyway)
- Added a unit test that verifies that the PS Cache behaves as expected
…o set the max cache size. Fixed tests.
alicel
requested review from
joao-r-reis,
grighetto,
absurdfarce and
weideng1
as code owners
| @@ -437,6 +437,7 @@ func NewTestConfig(originHost string, targetHost string) *config.Config { | |||
|
|
|||
| conf.ProxyMaxClientConnections = 1000 | |||
| conf.ProxyMaxStreamIds = 2048 | |||
| conf.ProxyMaxPreparedStatementCacheSize = 5000 | |||
There was a problem hiding this comment.
do we know what this limit is on the server side? We don't have to match it but it would probably be good to know before we decide which limit to set as the default on the proxy
There was a problem hiding this comment.
The limit is based on size, rather than number of prepared statements: The default calculated value is 1/256th of the heap or 10 MB, whichever is greater.
When I set the default to 5000, my intention was to keep the PS cache memory footprint relatively small, given that the proxy usually runs on instances with limited resources. It is quite unlikely for applications to create a large number of prepared statements if they are using them correctly, so even if a user has multiple applications using the proxy I thought that this should be a reasonable value. On the other hand, the footprint of each statement in the proxy PS cache maps is small, so choosing a default value that is a bit higher should also be fine.
There was a problem hiding this comment.
So for a 16GB heap the limit would be about 60MB, I think it would be good to get an estimate of what size an average prepared statement has on our cache so we can come up with a good value for this limit instead of guessing blindly. 5000 sounds a bit too low but without any data on the size that each statement takes I'm just guessing blindly. Is there server metrics for the prepared cache size? We could use those metrics in a benchmark to get some data that would help us come up with a good limit.
| @@ -64,31 +81,37 @@ func (psc *PreparedStatementCache) StoreIntercepted(preparedResult *message.Prep | |||
| func (psc *PreparedStatementCache) Get(originPreparedId []byte) (PreparedData, bool) { | |||
| psc.lock.RLock() | |||
| defer psc.lock.RUnlock() | |||
There was a problem hiding this comment.
We need to reevaluate these locks. This new cache implementation already does locking so it would be great if we could remove our locks.
proxy/pkg/zdmproxy/pscache.go
Outdated
| } | ||
|
|
||
| func (psc PreparedStatementCache) GetPreparedStatementCacheSize() float64 { | ||
| psc.lock.RLock() | ||
| defer psc.lock.RUnlock() | ||
|
|
||
| return float64(len(psc.cache) + len(psc.interceptedCache)) | ||
| //return float64(len(psc.cache) + len(psc.interceptedCache)) |
| psc.cache[originPrepareIdStr] = NewPreparedData(originPreparedResult, targetPreparedResult, prepareRequestInfo) | ||
| psc.index[targetPrepareIdStr] = originPrepareIdStr | ||
| psc.cache.Add(originPrepareIdStr, NewPreparedData(originPreparedResult, targetPreparedResult, prepareRequestInfo)) | ||
| psc.index.Add(targetPrepareIdStr, originPrepareIdStr) |
There was a problem hiding this comment.
Hmm what happens if the limit is reached and the key that is selected to be evicted on one cache is different from the key that was selected on the other cache?
I'm not sure if bad behavior can happen if these two caches are out of sync or if it's fine...
If we do need both caches to be in sync then a potential alternative would be to provide an eviction callback to the cache that removes that key from the index cache. This way we could revert the index cache to a normal map (with our own locks) and perform the eviction ourselves.
