On Sat, Mar 03, 2018 at 01:06:49PM +0000, Philip Martin wrote:
> Stefan Sperling <stsp_at_elego.de> writes:
>
> > I may have found a way to store an svn_repos_t object on the connection.
> > Does it help? (authz_tests pass)
>
> No significant improvement.
>
> I am timing
>
> time svn log http://localhost:8888/repos/asf/subversion \
> --username pm --password mp > /dev/null
>
> and I realised my earlier timings had the apache LogLevel set to Debug
> which means apache spends a significant time logging. Lowering the
> LogLevel to Crit improves the speed. Timings for 7 runs after starting
> apache:
>
> trunk patched reverted
> 1.11 1.11 1.11 1.9
>
> 5.2s 5.1s 3.1s 7.9s
> 3.4s 3.4s 1.3s 4.5s
> 3.7s 3.8s 1.7s 4.5s
> 3.9s 3.9s 1.8s 4.5s
> 4.0s 4.0s 1.9s 4.5s
> 3.9s 3.9s 1.8s 4.5s
> 3.8s 3.8s 1.8s 4.5s
>
> Now that apache is logging less the performance hit is even clearer.
> 1.11 could be 2 to 3 times faster than 1.9 but the authz effect means it
> is only about 10% faster.
>
> Note an odd effect in the above numbers. The second run for 1.11 is
> always the fastest. The first run in each case is the slowest, the
> Subversion cache is cold. The second run is faster, the Subversion
> cache is hot. In 1.9 subsequent runs are comparable to the second, but
> in 1.11 the subsequent runs are a bit slower than the second. I don't
> know what causes this effect.
I think I see why my previous patch didn't affect performance.
The cache lookup happens in authz_read() with svn_object_pool__lookup().
The authz config cache is allocated in an object pool which uses the
result_pool passed to svn_repos_authz_read3(). An item is evicted
from an object pool when the pool the item was allocated in gets
cleaned up (see add_object_ref() in libsvn_subr/object_pool.c).
My commit r1825736 reduced the lifetime of the result pool to the
lifetime of the request, which apparently means the cached authz
config is removed from the object pool during request pool cleanup,
and doesn't survive throughout the connection.
So it looks like we'll have to pass the connection pool as result
pool instead. And this should now be safe since only one allocation
should occur if we pass an svn_repos_t as well?
We will still allocate one svn_authz_t object per request on the
connection. But that struct is small, it contains only a few pointers.
This problem could be addressed with an API change to svn_repos_authz_read3()
which adds a third pool argument for use by the object pool cache.
It seems the intention of r1778923 was to use this object pool cache
but the config cache lookup didn't work because a NULL pointer was passed
for the svn_repos_t argument. I don't fully understand why. Did we get a
new "authz_id' in authz_read() on every request?
There might be another bug to fix here: It looks like get_repos_config()
in libsvn_repos/config_file.c is missing a cache lookup:
/* Fetch checksum and see whether we already have a matching config */
SVN_ERR(svn_fs_file_checksum(checksum, svn_checksum_md5, root, fs_path,
TRUE, access->pool));
Given the comment, I would expect a cache lookup based on the checksum
here, but the code below just does this:
*stream = representation_stream(root, fs_path, access->pool);
return SVN_NO_ERROR;
}
Regardless, could you try one more test run with this diff against
trunk to see if it has any impact already?
Index: subversion/mod_authz_svn/mod_authz_svn.c
===================================================================
--- subversion/mod_authz_svn/mod_authz_svn.c (revision 1825762)
+++ subversion/mod_authz_svn/mod_authz_svn.c (working copy)
@@ -399,7 +399,6 @@ resolve_repos_relative_url(const char **path, cons
*/
static svn_authz_t *
get_access_conf(request_rec *r, authz_svn_config_rec *conf,
- apr_pool_t *result_pool,
apr_pool_t *scratch_pool)
{
const char *access_file;
@@ -409,6 +408,7 @@ get_access_conf(request_rec *r, authz_svn_config_r
svn_authz_t *access_conf = NULL;
svn_error_t *svn_err = SVN_NO_ERROR;
dav_error *dav_err;
+ svn_repos_t *repos;
dav_err = dav_svn_get_repos_path2(r, conf->base_path, &repos_path, scratch_pool);
if (dav_err)
@@ -465,10 +465,25 @@ get_access_conf(request_rec *r, authz_svn_config_r
"Path to groups file is %s", groups_file);
}
+ /* Store an svn_repos_t on this connection to allow authz config caching. */
+ repos = ap_get_module_config(r->connection->conn_config, &authz_svn_module);
+ if (repos == NULL)
+ {
+ svn_err = svn_repos_open3(&repos, repos_path, NULL, r->connection->pool,
+ scratch_pool);
+ if (svn_err)
+ {
+ log_svn_error(APLOG_MARK, r, "Failed to open repository:",
+ svn_err, scratch_pool);
+ return NULL;
+ }
+ ap_set_module_config(r->connection->conn_config, &authz_svn_module, repos);
+ }
+
svn_err = svn_repos_authz_read3(&access_conf,
access_file, groups_file,
- TRUE, NULL,
- result_pool, scratch_pool);
+ TRUE, repos,
+ r->connection->pool, scratch_pool);
if (svn_err)
{
@@ -688,7 +703,7 @@ req_check_access(request_rec *r,
}
/* Retrieve/cache authorization file */
- access_conf = get_access_conf(r,conf, r->pool, r->pool);
+ access_conf = get_access_conf(r,conf, r->pool);
if (access_conf == NULL)
return DECLINED;
@@ -805,7 +820,7 @@ subreq_bypass2(request_rec *r,
}
/* Retrieve authorization file */
- access_conf = get_access_conf(r, conf, scratch_pool, scratch_pool);
+ access_conf = get_access_conf(r, conf, scratch_pool);
if (access_conf == NULL)
return HTTP_FORBIDDEN;
Received on 2018-03-03 15:10:58 CET