On Thu, Aug 8, 2013 at 11:04 PM, Ben Reser <b...@reser.org> wrote: > On Thu, Aug 8, 2013 at 5:41 AM, Ivan Zhakov <i...@visualsvn.com> wrote: >> I don't see problem here: in worst scenario cache size would same as >> authorization file. So even for large authorization files memory usage >> will be limited. > > Yes I realize it's not entirely unbounded. > > It's my opinion that caches should always have bounds outside of just > the source data. > > It is much better to limit the cache and then run slower in these rare > circumstances than it is to simply fail entirely due to out of memory > conditions. > As far I understand cache will be very inefficient if authorization file will be bigger than configured limit. I real world cases cache will hold data for every configured path if user run svn log on repository root or something.
>> Other approaches are: >> 1. use svn_cache__t object to store cached values > > I think this is the route you have to go if you want to set limits. > I'm not sure how reasonable it is to keep only doing per-connection > caches with this though. Caching beyond the connection means you have > to start dealing with cache invalidation from changes to the conf > file. > I didn't suggest caching beyond connection. It's unrelated improvement. And I agree that cache invalidation is big issue in this case. >> 2. Factor out configuration file parser and store authorization >> settings in our own hash table with interesting cached values. > > I tend to think there's a lot bigger savings from this because the > config parser is certainly not storing things in a way that is optimal > for an authz system. It should not be hard to implement. We can just change signature of svn_config__parse() function to svn_error_t *svn_config__parse(svn_config__set_option_t set_option_cb, void *baton, svn_stream_t *stream, apr_pool_t *result_pool, apr_pool_t *scratch_pool); typedef svn_error_t *(*svn_config__set_option_t)( void *baton, const char *name, const char *value, apr_pool_t *pool); It would be best solution, but require more changes though. -- Ivan Zhakov CTO | VisualSVN | http://www.visualsvn.com
