Inconsistencies in FSFS cache-* options

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Inconsistencies in FSFS cache-* options

Julian Foad-5
Stefan2 et al.:

Nodeprops caching is new in 1.10. This caused me to have a look at the
code for enabling it, and I noticed the following in FSFS (and the same
in FSX).

In subversion/libsvn_fs_fs/caching.c@1787000, around line 100:
 >   /* don't cache text deltas by default.

Wrong: the default was changed to TRUE in r1517479, and this new default
is contrary to the reasoning given in this comment.

 >    * Once we reconstructed the fulltexts from the deltas,
 >    * these deltas are rarely re-used. Therefore, only tools
 >    * like svnadmin will activate this to speed up operations
 >    * dump and verify.
 >    */
 >   *cache_txdeltas
 >     = svn_hash__get_bool(fs->config,
 >                          SVN_FS_CONFIG_FSFS_CACHE_DELTAS,
 >                          TRUE);
[...]
 >   /* by default, cache nodeprops: this will match pre-1.10
 >    * behavior where node properties caching was controlled
 >    * by SVN_FS_CONFIG_FSFS_CACHE_FULLTEXTS configuration option.

The default of TRUE doesn't match that behaviour, does it? To do so,
should the last argument (below) be "*cache_txdeltas" instead of "TRUE"?

 >    */
 >   *cache_nodeprops
 >     = svn_hash__get_bool(fs->config,
 >                          SVN_FS_CONFIG_FSFS_CACHE_NODEPROPS,
 >                          TRUE);

Also, the handling of these flags in mod_dav_svn (around line 990) is odd:

> svn_boolean_t
> dav_svn__get_txdelta_cache_flag(request_rec *r)
> {
>   dir_conf_t *conf;
>
>   conf = ap_get_module_config(r->per_dir_config, &dav_svn_module);
>
>   /* txdelta caching is enabled by default. */
>   return get_conf_flag(conf->txdelta_cache, TRUE);
> }
>
>
> svn_boolean_t
> dav_svn__get_fulltext_cache_flag(request_rec *r)
> {
>   dir_conf_t *conf;
>
>   conf = ap_get_module_config(r->per_dir_config, &dav_svn_module);
>   return conf->fulltext_cache == CONF_FLAG_ON;

This construction ("==ON") means the default here is OFF, which is the
opposite of the default that FSFS would apply if we didn't pass it any
value for this flag.

> }
>
>
> svn_boolean_t
> dav_svn__get_revprop_cache_flag(request_rec *r)
> {
>   dir_conf_t *conf;
>
>   conf = ap_get_module_config(r->per_dir_config, &dav_svn_module);
>   return conf->revprop_cache == CONF_FLAG_ON;
> }
>
> svn_boolean_t
> dav_svn__get_nodeprop_cache_flag(request_rec *r)
> {
>   dir_conf_t *conf;
>
>   conf = ap_get_module_config(r->per_dir_config, &dav_svn_module);
>   /* node properties caching is enabled by default. */
>   return get_conf_flag(conf->nodeprop_cache, FALSE);

The code here uses a default value of "FALSE", which is different from
the fsfs.conf default value (see above), and doesn't match the comment here.

> }

- Julian

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Inconsistencies in FSFS cache-* options

Stefan Fuhrmann
On 16.03.2017 15:08, Julian Foad wrote:

> Stefan2 et al.:
>
> Nodeprops caching is new in 1.10. This caused me to have a look at the
> code for enabling it, and I noticed the following in FSFS (and the
> same in FSX).
>
> In subversion/libsvn_fs_fs/caching.c@1787000, around line 100:
> >   /* don't cache text deltas by default.
>
> Wrong: the default was changed to TRUE in r1517479, and this new
> default is contrary to the reasoning given in this comment.
>
> >    * Once we reconstructed the fulltexts from the deltas,
> >    * these deltas are rarely re-used. Therefore, only tools
> >    * like svnadmin will activate this to speed up operations
> >    * dump and verify.

Yup, that comment is outdated. Fixed in r1791793.

> >    */
> >   *cache_txdeltas
> >     = svn_hash__get_bool(fs->config,
> >                          SVN_FS_CONFIG_FSFS_CACHE_DELTAS,
> >                          TRUE);
> [...]
> >   /* by default, cache nodeprops: this will match pre-1.10
> >    * behavior where node properties caching was controlled
> >    * by SVN_FS_CONFIG_FSFS_CACHE_FULLTEXTS configuration option.
>
> The default of TRUE doesn't match that behaviour, does it? To do so,
> should the last argument (below) be "*cache_txdeltas" instead of "TRUE"?

In r1791793, I tried to make Ivan's intention more clear.

>
> >    */
> >   *cache_nodeprops
> >     = svn_hash__get_bool(fs->config,
> >                          SVN_FS_CONFIG_FSFS_CACHE_NODEPROPS,
> >                          TRUE);
>
> Also, the handling of these flags in mod_dav_svn (around line 990) is
> odd:
>
>> svn_boolean_t
>> dav_svn__get_txdelta_cache_flag(request_rec *r)
>> {
>>   dir_conf_t *conf;
>>
>>   conf = ap_get_module_config(r->per_dir_config, &dav_svn_module);
>>
>>   /* txdelta caching is enabled by default. */
>>   return get_conf_flag(conf->txdelta_cache, TRUE);
>> }
>>
>>
>> svn_boolean_t
>> dav_svn__get_fulltext_cache_flag(request_rec *r)
>> {
>>   dir_conf_t *conf;
>>
>>   conf = ap_get_module_config(r->per_dir_config, &dav_svn_module);
>>   return conf->fulltext_cache == CONF_FLAG_ON;
>
> This construction ("==ON") means the default here is OFF, which is the
> opposite of the default that FSFS would apply if we didn't pass it any
> value for this flag.
>
>> }
>>
>>
>> svn_boolean_t
>> dav_svn__get_revprop_cache_flag(request_rec *r)
>> {
>>   dir_conf_t *conf;
>>
>>   conf = ap_get_module_config(r->per_dir_config, &dav_svn_module);
>>   return conf->revprop_cache == CONF_FLAG_ON;
>> }
>>
>> svn_boolean_t
>> dav_svn__get_nodeprop_cache_flag(request_rec *r)
>> {
>>   dir_conf_t *conf;
>>
>>   conf = ap_get_module_config(r->per_dir_config, &dav_svn_module);
>>   /* node properties caching is enabled by default. */
>>   return get_conf_flag(conf->nodeprop_cache, FALSE);
>
> The code here uses a default value of "FALSE", which is different from
> the fsfs.conf default value (see above), and doesn't match the comment
> here.

Yes, the whole section is quite inconsistent. r1791794 should fix this.

Thanks for the review!

-- Stefan^2.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Inconsistencies in FSFS cache-* options

Julian Foad-5
Stefan Fuhrmann wrote:
> On 16.03.2017 15:08, Julian Foad wrote:
> > Nodeprops caching is new in 1.10. This caused me to have a look at the
> > code for enabling it, and I noticed the following in FSFS (and the
> > same in FSX).
[...]
> Yup, that comment is outdated. Fixed in r1791793.
>
[...]
> In r1791793, I tried to make Ivan's intention more clear.
>
[...]
> Yes, the whole section is quite inconsistent. r1791794 should fix this.
>
> Thanks for the review!

Thanks. That looks better.

- Julian

Loading...