On 07/31/2014 03:29 AM, Amos Jeffries wrote: > A garbage collection TTL "cleanup_interval" is configurable and removes > cache entries which have been stale for at least 1 hr.
While some old code still uses periodic cleanup, I think we should avoid adding more code like that. Periodic cleanup leads to cache overflows, memory usage surprises, and hot idle. Please remove old unneeded cache entries when adding a new cache entry instead of checking the cache state every hour. > + // only clear tokens out of cache after 1 hour stale > + // could make this configurable > + time_t stalenessLimit = current_time.tv_sec - 60*60; Cache size should be(**) regulated in terms of memory usage (bytes), not entry age (seconds). I believe estimating memory footprint of a cache entry would be fairly straightforward in this case. Would you be willing to convert the code to use size limits? > + /* > + * For big caches we could consider stepping through > + * 100/200 entries at a time. Lets see how it flies > + * first. > + */ > + Auth::Bearer::TokenCache *cache = &Auth::Bearer::Token::Cache; > + for (Auth::Bearer::TokenCache::iterator itr = cache->begin(); itr != > cache->end();) { I do not think we should do linear search like that. It is going to be too slow for large caches and adding arbitrary "maximum number of iterations" limits just adds more tuning headaches. Why not use an LRU cache instead? IIRC, we have even added LruMap class for that recently. > + For Basic and Bearer the default is "Squid proxy-caching web server". Please add a comma after Bearer. Thank you, Alex. (**) I hope we do not need to debate that rule of thumb, but I am ready to justify it if needed.