You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2013/05/23 01:22:22 UTC
svn commit: r1485499 - /subversion/trunk/subversion/libsvn_ra_svn/marshal.c
Author: stefan2
Date: Wed May 22 23:22:22 2013
New Revision: 1485499
URL: http://svn.apache.org/r1485499
Log:
Reduce ra_svn protocol parsing overhead.
* subversion/libsvn_ra_svn/marshal.c
(vparse_tuple): test for the most frequent item types first
Modified:
subversion/trunk/subversion/libsvn_ra_svn/marshal.c
Modified: subversion/trunk/subversion/libsvn_ra_svn/marshal.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_svn/marshal.c?rev=1485499&r1=1485498&r2=1485499&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_svn/marshal.c (original)
+++ subversion/trunk/subversion/libsvn_ra_svn/marshal.c Wed May 22 23:22:22 2013
@@ -1202,14 +1202,15 @@ static svn_error_t *vparse_tuple(const a
if (**fmt == '?')
(*fmt)++;
elt = &APR_ARRAY_IDX(items, count, svn_ra_svn_item_t);
- if (**fmt == 'n' && elt->kind == SVN_RA_SVN_NUMBER)
- *va_arg(*ap, apr_uint64_t *) = elt->u.number;
- else if (**fmt == 'r' && elt->kind == SVN_RA_SVN_NUMBER)
- *va_arg(*ap, svn_revnum_t *) = (svn_revnum_t) elt->u.number;
- else if (**fmt == 's' && elt->kind == SVN_RA_SVN_STRING)
- *va_arg(*ap, svn_string_t **) = elt->u.string;
+ if (**fmt == '(' && elt->kind == SVN_RA_SVN_LIST)
+ {
+ (*fmt)++;
+ SVN_ERR(vparse_tuple(elt->u.list, pool, fmt, ap));
+ }
else if (**fmt == 'c' && elt->kind == SVN_RA_SVN_STRING)
*va_arg(*ap, const char **) = elt->u.string->data;
+ else if (**fmt == 's' && elt->kind == SVN_RA_SVN_STRING)
+ *va_arg(*ap, svn_string_t **) = elt->u.string;
else if (**fmt == 'w' && elt->kind == SVN_RA_SVN_WORD)
*va_arg(*ap, const char **) = elt->u.word;
else if (**fmt == 'b' && elt->kind == SVN_RA_SVN_WORD)
@@ -1221,6 +1222,10 @@ static svn_error_t *vparse_tuple(const a
else
break;
}
+ else if (**fmt == 'n' && elt->kind == SVN_RA_SVN_NUMBER)
+ *va_arg(*ap, apr_uint64_t *) = elt->u.number;
+ else if (**fmt == 'r' && elt->kind == SVN_RA_SVN_NUMBER)
+ *va_arg(*ap, svn_revnum_t *) = (svn_revnum_t) elt->u.number;
else if (**fmt == 'B' && elt->kind == SVN_RA_SVN_WORD)
{
if (strcmp(elt->u.word, "true") == 0)
@@ -1241,11 +1246,6 @@ static svn_error_t *vparse_tuple(const a
}
else if (**fmt == 'l' && elt->kind == SVN_RA_SVN_LIST)
*va_arg(*ap, apr_array_header_t **) = elt->u.list;
- else if (**fmt == '(' && elt->kind == SVN_RA_SVN_LIST)
- {
- (*fmt)++;
- SVN_ERR(vparse_tuple(elt->u.list, pool, fmt, ap));
- }
else if (**fmt == ')')
return SVN_NO_ERROR;
else
Re: svn commit: r1485499 - /subversion/trunk/subversion/libsvn_ra_svn/marshal.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Thu, May 23, 2013 at 2:01 AM, Blair Zajac <bl...@orcaware.com> wrote:
> On 05/22/2013 04:22 PM, stefan2@apache.org wrote:
>
>> Author: stefan2
>> Date: Wed May 22 23:22:22 2013
>> New Revision: 1485499
>>
>> URL: http://svn.apache.org/r1485499
>> Log:
>> Reduce ra_svn protocol parsing overhead.
>>
>> * subversion/libsvn_ra_svn/**marshal.c
>> (vparse_tuple): test for the most frequent item types first
>>
>> Modified:
>> subversion/trunk/subversion/**libsvn_ra_svn/marshal.c
>>
>> Modified: subversion/trunk/subversion/**libsvn_ra_svn/marshal.c
>> URL: http://svn.apache.org/viewvc/**subversion/trunk/subversion/**
>> libsvn_ra_svn/marshal.c?rev=**1485499&r1=1485498&r2=1485499&**view=diff<http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_svn/marshal.c?rev=1485499&r1=1485498&r2=1485499&view=diff>
>> ==============================**==============================**
>> ==================
>> --- subversion/trunk/subversion/**libsvn_ra_svn/marshal.c (original)
>> +++ subversion/trunk/subversion/**libsvn_ra_svn/marshal.c Wed May 22
>> 23:22:22 2013
>> @@ -1202,14 +1202,15 @@ static svn_error_t *vparse_tuple(const a
>> if (**fmt == '?')
>> (*fmt)++;
>> elt = &APR_ARRAY_IDX(items, count, svn_ra_svn_item_t);
>> - if (**fmt == 'n' && elt->kind == SVN_RA_SVN_NUMBER)
>> - *va_arg(*ap, apr_uint64_t *) = elt->u.number;
>> - else if (**fmt == 'r' && elt->kind == SVN_RA_SVN_NUMBER)
>> - *va_arg(*ap, svn_revnum_t *) = (svn_revnum_t) elt->u.number;
>> - else if (**fmt == 's' && elt->kind == SVN_RA_SVN_STRING)
>> - *va_arg(*ap, svn_string_t **) = elt->u.string;
>> + if (**fmt == '(' && elt->kind == SVN_RA_SVN_LIST)
>> + {
>> + (*fmt)++;
>> + SVN_ERR(vparse_tuple(elt->u.**list, pool, fmt, ap));
>> + }
>>
>
> Out of curiosity, how much cost is there in the double dereferencing of
> fmt?
Don't know but since there are no intermittent writes,
**fmt can be read once and then be taken from the
same register over and over again. The performance
relevant part are the branch misprediction rates. We
see about 1 .. 1.5 mispredicted jumps / iteration.
> Could you save it in a local variable and do the if/else on that instead?
The compiler already does that.
> Also, would a switch block be more efficient?
>
Possibly. *If* the compiler generated a jump table
for it, we might reduce the misprediction rate by
50% or so. But my strategy is rather to replace
the usage of vparse_tuple by a set of functions
that extract the data explicitly - similar to what
the command functions already do.
-- Stefan^2.
--
*Join one of our free daily demo sessions on* *Scaling Subversion for the
Enterprise <http://www.wandisco.com/training/webinars>*
*
*
Re: svn commit: r1485499 - /subversion/trunk/subversion/libsvn_ra_svn/marshal.c
Posted by Blair Zajac <bl...@orcaware.com>.
On 05/22/2013 04:22 PM, stefan2@apache.org wrote:
> Author: stefan2
> Date: Wed May 22 23:22:22 2013
> New Revision: 1485499
>
> URL: http://svn.apache.org/r1485499
> Log:
> Reduce ra_svn protocol parsing overhead.
>
> * subversion/libsvn_ra_svn/marshal.c
> (vparse_tuple): test for the most frequent item types first
>
> Modified:
> subversion/trunk/subversion/libsvn_ra_svn/marshal.c
>
> Modified: subversion/trunk/subversion/libsvn_ra_svn/marshal.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_svn/marshal.c?rev=1485499&r1=1485498&r2=1485499&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_ra_svn/marshal.c (original)
> +++ subversion/trunk/subversion/libsvn_ra_svn/marshal.c Wed May 22 23:22:22 2013
> @@ -1202,14 +1202,15 @@ static svn_error_t *vparse_tuple(const a
> if (**fmt == '?')
> (*fmt)++;
> elt = &APR_ARRAY_IDX(items, count, svn_ra_svn_item_t);
> - if (**fmt == 'n' && elt->kind == SVN_RA_SVN_NUMBER)
> - *va_arg(*ap, apr_uint64_t *) = elt->u.number;
> - else if (**fmt == 'r' && elt->kind == SVN_RA_SVN_NUMBER)
> - *va_arg(*ap, svn_revnum_t *) = (svn_revnum_t) elt->u.number;
> - else if (**fmt == 's' && elt->kind == SVN_RA_SVN_STRING)
> - *va_arg(*ap, svn_string_t **) = elt->u.string;
> + if (**fmt == '(' && elt->kind == SVN_RA_SVN_LIST)
> + {
> + (*fmt)++;
> + SVN_ERR(vparse_tuple(elt->u.list, pool, fmt, ap));
> + }
Out of curiosity, how much cost is there in the double dereferencing of
fmt? Could you save it in a local variable and do the if/else on that
instead? Also, would a switch block be more efficient?
Blair