[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

[PATCH v2] Saving a few cycles, part 3/3

From: Stefan Fuhrmann <stefanfuhrmann_at_alice-dsl.de>
Date: Sat, 01 May 2010 19:02:53 +0200

Hi devs,

I reworked the patch set according to the feedback I got
here on this list. This is what changed in this last part:

* add rationales to the commentary
* bring svndiff.c changes closer to the original code
* use svn_ctype_* functions in checksum parser

-- Stefan^2.

[[[
Various local optimizations. These opportunities became
visible after significantly optimizing other parts of svn. The
total savings for a 'svn export' is almost 3 percent on top
of the previous.

The largest savings can be attributed to the svndiff.c
changes (~1.5%) and the checksum parser (~1%).

* subversion/include/svn_delta.h
  (enum svn_delta_action): ensure that these definitions
  will always have these values, even if new definitions
  were added

* subversion/libsvn_delta/compose_delta.c
  (search_offset_index): use size_t to index memory.
  This is mainly a consistency fix but may actually result
  in slightly higher performance due to fewer conversions.
  (copy_source_ops): dito; optimize a common case

* subversion/libsvn_delta/svndiff.c
  (decode_file_offset, decode_size): use slightly more
  efficient formulations
  (decode_instruction): directly map action codes -
  made possible by defining the enum values

* subversion/libsvn_subr/checksum.c
  (svn_checksum_parse_hex): eliminate calls to locale-
  aware CRT functions. At least with MS compilers,
  these are very expensive.

* subversion/libsvn_subr/stream.c
  (stream_readline): numbytes is invariant until EOF

patch by stefanfuhrmann < at > alice-dsl.de
]]]

Index: subversion/include/svn_delta.h
===================================================================
--- subversion/include/svn_delta.h (revision 939976)
+++ subversion/include/svn_delta.h (working copy)
@@ -88,6 +88,10 @@
  * When svn_txdelta_next_window() returns zero, we are done building
  * the target string.
  *
+ * Please note that the values assigned in these enumeration must
+ * never be changed. They must match the instruction encoding value.
+ * You are, however, free to add new, non-overlapping definitions.
+ *
  * @defgroup svn_delta_txt_delta Text deltas
  * @{
  */
@@ -100,7 +104,7 @@
      * It must be the case that 0 <= @a offset < @a offset +
      * @a length <= size of source view.
      */
- svn_txdelta_source,
+ svn_txdelta_source = 0,
 
     /** Append the @a length bytes at @a offset in the target view, to the
      * target.
@@ -119,7 +123,7 @@
      * useful in encoding long runs of consecutive characters, long runs
      * of CR/LF pairs, etc.
      */
- svn_txdelta_target,
+ svn_txdelta_target = 1,
 
     /** Append the @a length bytes at @a offset in the window's @a new string
      * to the target.
@@ -129,7 +133,7 @@
      * order with no overlap at the moment; svn_txdelta_to_svndiff()
      * depends on this.
      */
- svn_txdelta_new
+ svn_txdelta_new = 2
 };
 
 /** A single text delta instruction. */
Index: subversion/libsvn_delta/compose_delta.c
===================================================================
--- subversion/libsvn_delta/compose_delta.c (revision 939976)
+++ subversion/libsvn_delta/compose_delta.c (working copy)
@@ -165,10 +165,10 @@
    as hint because most lookups come as a sequence of decreasing values
    for OFFSET and they concentrate on the lower end of the array. */
 
-static int
-search_offset_index(const offset_index_t *ndx, apr_size_t offset, int hint)
+static apr_size_t
+search_offset_index(const offset_index_t *ndx, apr_size_t offset, apr_size_t hint)
 {
- int lo, hi, op;
+ apr_size_t lo, hi, op;
 
   assert(offset < ndx->offs[ndx->length]);
 
@@ -635,13 +635,13 @@
 static void
 copy_source_ops(apr_size_t offset, apr_size_t limit,
                 apr_size_t target_offset,
- int hint,
+ apr_size_t hint,
                 svn_txdelta__ops_baton_t *build_baton,
                 const svn_txdelta_window_t *window,
                 const offset_index_t *ndx,
                 apr_pool_t *pool)
 {
- int op_ndx = search_offset_index(ndx, offset, hint);
+ apr_size_t op_ndx = search_offset_index(ndx, offset, hint);
   for (;; ++op_ndx)
     {
       const svn_txdelta_op_t *const op = &window->ops[op_ndx];
@@ -694,7 +694,15 @@
                  The idea here is to transpose the pattern, then generate
                  another overlapping copy. */
               const apr_size_t ptn_length = off[0] - op->offset;
- const apr_size_t ptn_overlap = fix_offset % ptn_length;
+
+ /* As it turns out, ptn_length is often less than 3.
+ We can use a more efficient modulo is that frequent case.
+ Please note that this optimization is only valid due to
+ the special case actually being the major case. */
+ const apr_size_t ptn_overlap = ptn_length > 2
+ ? fix_offset % ptn_length
+ : fix_offset & (ptn_length-1);
+
               apr_size_t fix_off = fix_offset;
               apr_size_t tgt_off = target_offset;
               assert(ptn_length > ptn_overlap);
Index: subversion/libsvn_delta/svndiff.c
===================================================================
--- subversion/libsvn_delta/svndiff.c (revision 939976)
+++ subversion/libsvn_delta/svndiff.c (working copy)
@@ -359,16 +359,27 @@
                    const unsigned char *p,
                    const unsigned char *end)
 {
+ svn_filesize_t temp = 0;
+
   if (p + MAX_ENCODED_INT_LEN < end)
     end = p + MAX_ENCODED_INT_LEN;
   /* Decode bytes until we're done. */
- *val = 0;
   while (p < end)
     {
- *val = (*val << 7) | (*p & 0x7f);
- if (((*p++ >> 7) & 0x1) == 0)
+ /* Don't use svn_filesize_t here, because this might be 64 bits
+ * on 32 bit targets. Optimizing compilers may or may not be
+ * able to reduce that to the effective code below. */
+ unsigned int c = *p++;
+
+ temp = (temp << 7) | (c & 0x7f);
+ if (c < 0x80)
+ {
+ *val = temp;
         return p;
+ }
     }
+
+ *val = temp;
   return NULL;
 }
 
@@ -379,16 +390,24 @@
             const unsigned char *p,
             const unsigned char *end)
 {
+ apr_size_t temp = 0;
+
   if (p + MAX_ENCODED_INT_LEN < end)
     end = p + MAX_ENCODED_INT_LEN;
   /* Decode bytes until we're done. */
- *val = 0;
   while (p < end)
     {
- *val = (*val << 7) | (*p & 0x7f);
- if (((*p++ >> 7) & 0x1) == 0)
+ apr_size_t c = *p++;
+
+ temp = (temp << 7) | (c & 0x7f);
+ if (c < 0x80)
+ {
+ *val = temp;
         return p;
+ }
     }
+
+ *val = temp;
   return NULL;
 }
 
@@ -453,27 +472,33 @@
                    const unsigned char *p,
                    const unsigned char *end)
 {
+ apr_size_t c;
+ apr_size_t action;
+
   if (p == end)
     return NULL;
 
+ /* We need this more than once */
+ c = *p++;
+
   /* Decode the instruction selector. */
- switch ((*p >> 6) & 0x3)
- {
- case 0x0: op->action_code = svn_txdelta_source; break;
- case 0x1: op->action_code = svn_txdelta_target; break;
- case 0x2: op->action_code = svn_txdelta_new; break;
- case 0x3: return NULL;
- }
+ action = (c >> 6) & 0x3;
+ if (action >= 0x3)
+ return NULL;
 
+ /* This relies on enum svn_delta_action values to match and
+ never to be redefined. */
+ op->action_code = (enum svn_delta_action)(action);
+
   /* Decode the length and offset. */
- op->length = *p++ & 0x3f;
+ op->length = c & 0x3f;
   if (op->length == 0)
     {
       p = decode_size(&op->length, p, end);
       if (p == NULL)
         return NULL;
     }
- if (op->action_code != svn_txdelta_new)
+ if (action != svn_txdelta_new)
     {
       p = decode_size(&op->offset, p, end);
       if (p == NULL)
Index: subversion/libsvn_subr/checksum.c
===================================================================
--- subversion/libsvn_subr/checksum.c (revision 939976)
+++ subversion/libsvn_subr/checksum.c (working copy)
@@ -29,6 +29,7 @@
 
 #include "svn_checksum.h"
 #include "svn_error.h"
+#include "svn_ctype.h"
 
 #include "sha1.h"
 #include "md5.h"
@@ -206,12 +207,15 @@
 
   for (i = 0; i < len; i++)
     {
- if ((! isxdigit(hex[i * 2])) || (! isxdigit(hex[i * 2 + 1])))
+ if ((! svn_ctype_isxdigit(hex[i * 2])) ||
+ (! svn_ctype_isxdigit(hex[i * 2 + 1])))
         return svn_error_create(SVN_ERR_BAD_CHECKSUM_PARSE, NULL, NULL);
 
       ((unsigned char *)(*checksum)->digest)[i] =
- (( isalpha(hex[i*2]) ? hex[i*2] - 'a' + 10 : hex[i*2] - '0') << 4) |
- ( isalpha(hex[i*2+1]) ? hex[i*2+1] - 'a' + 10 : hex[i*2+1] - '0');
+ (( svn_ctype_isalpha(hex[i*2]) ? hex[i*2] - 'a' + 10
+ : hex[i*2] - '0') << 4) |
+ ( svn_ctype_isalpha(hex[i*2+1]) ? hex[i*2+1] - 'a' + 10
+ : hex[i*2+1] - '0');
       is_zeros |= (*checksum)->digest[i];
     }
 
Index: subversion/libsvn_subr/stream.c
===================================================================
--- subversion/libsvn_subr/stream.c (revision 937673)
+++ subversion/libsvn_subr/stream.c (working copy)
@@ -355,9 +355,9 @@
 
       /* Read into STR up to and including the next EOL sequence. */
       match = eol_str;
+ numbytes = 1;
       while (*match)
         {
- numbytes = 1;
           SVN_ERR(svn_stream_read(stream, &c, &numbytes));
           if (numbytes != 1)
             {
 
Received on 2010-05-01 19:03:35 CEST

This is an archived mail posted to the Subversion Dev mailing list.