Please use text/* MIME type. The .c file was so but the .patch wasn't,
and that gets in the way of reviewing it. More below...
Roderich Schupp wrote on Wed, Aug 07, 2013 at 08:27:43 +0200:
> This patch reduces the cumulative time for svn_repos_authz_check_access
> (when called repeteadly in the same connection) for more than 50%
> (e.g. running the attached test program on a large body of paths).
>
Can this break anything?
For example, when a client uses a long-lived keep-alive connection and
the authz file changes during the life of that connection, will your
change cause the changes to be picked up later than they would before
your patch?
If I'm not mistaken, your patch doesn't affect whether the authz file is
re-read or not, it just caches the answer to the question "What access
does <this> svn_authz_t.cfg give $USER to $PATH". Right?
> The patch needs to #include libsvn_subr/config_impl.h
> in order to gain access to svn_config_t.pool:
> the cache (apr_hash_t itself, keys and values) must be allocated from
> the same pool as svn_config_t so that they have the same lifespan.
Sorry, you can't do that. libsvn_repos may not rely on implementation
details of libsvn_subr. However, you create the svn_config_t structure,
so you know its lifetime; just pass the same pool around (probably in
the baton).
> @@ -754,6 +820,9 @@
> {
> struct authz_validate_baton baton = { 0 };
>
> + authz->cached_user = NULL; /* FIXME does apr_palloc zero the alloced stuff? */
> + authz->cache = NULL; /* FIXME does apr_palloc zero the alloced stuff? */
apr_pcalloc does, apr_palloc doesn't.
BTW, that means you need to set CACHE and CACHED_USER in
svn_repos__authz_read() when it allocats an svn_authz_t.
Cheers,
Daniel
Received on 2013-08-07 11:03:00 CEST