Re: [PATCH] speed up svn_repos_authz_check_access

On Thu, Aug 8, 2013 at 11:04 PM, Ben Reser <ben_at_reser.org> wrote:
> On Thu, Aug 8, 2013 at 5:41 AM, Ivan Zhakov <ivan_at_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