Re: [RFC] diff-optimizations-bytes branch: avoiding function call overhead (?)

On Mon, Dec 20, 2010 at 2:16 PM, Johan Corveleyn <jcorvel_at_gmail.com> wrote:
> On Mon, Dec 20, 2010 at 11:03 AM, Julian Foad <julian.foad_at_wandisco.com> wrote:
>> On Mon, 2010-12-20, Johan Corveleyn wrote:
>>> New macro version (increment only, decrement is similar):
>>> [[[
>>> /* For all files in the FILE array, increment the curp pointer.  If a file
>>>  * points before the beginning of file, let it point at the first byte again.
>>>  * If the end of the current chunk is reached, read the next chunk in the
>>>  * buffer and point curp to the start of the chunk.  If EOF is reached, set
>>>  * curp equal to endp to indicate EOF. */
>>> #define increment_pointers(all_files, files_len, pool)                       \
>>>   do {                                                                       \
>>>     int i;                                                                   \
>>>                                                                              \
>>>     for (i = 0; i < files_len; i++)                                          \
>>>     {                                                                        \
>>>       if (all_files[i]->chunk == -1) /* indicates before beginning of file */\
>>>         all_files[i]->chunk = 0;     /* point to beginning of file again */  \
>>>       else if (all_files[i]->curp != all_files[i]->endp - 1)                 \
>>>         all_files[i]->curp++;                                                \
>>
>> Hi Johan.
>>
>> Here you are having to test for two special cases every time: chunk==-1
>> and curp==endp-1.  I would suggest changing the specification of "before
>> beginning of file" to include the promise that curp==endp-1, so that you
>> don't have to use a separate test here and can instead test for this
>> special case within increment_chunk().
>
> Good idea. I'll try this tonight ...

Ok, this worked pretty well. It's a little bit faster (about 1 second,
which seems right intuitively).

One style question though: should these macros be all upper case
(INCREMENT_POINTERS and DECREMENT_POINTERS)? I guess so.

The code now looks as follows (in attachment a patch relative to
diff-optimizations-bytes branch):

[[[
/* For all files in the FILE array, increment the curp pointer. If a file
 * points before the beginning of file, let it point at the first byte again.
 * If the end of the current chunk is reached, read the next chunk in the
 * buffer and point curp to the start of the chunk. If EOF is reached, set
 * curp equal to endp to indicate EOF. */
#define increment_pointers(all_files, files_len, pool) \
  do { \
    int i; \
                                                                             \
    for (i = 0; i < files_len; i++) \
    { \
      if (all_files[i]->curp < all_files[i]->endp - 1) \
        all_files[i]->curp++; \
      else \
        SVN_ERR(increment_chunk(all_files[i], pool)); \
    } \
  } while (0)

/* For all files in the FILE array, decrement the curp pointer. If the
 * start of a chunk is reached, read the previous chunk in the buffer and
 * point curp to the last byte of the chunk. If the beginning of a FILE is
 * reached, set chunk to -1 to indicate BOF. */
#define decrement_pointers(all_files, files_len, pool) \
  do { \
    int i; \
                                                                             \
    for (i = 0; i < files_len; i++) \
    { \
      if (all_files[i]->curp > all_files[i]->buffer) \
        all_files[i]->curp--; \
      else \
        SVN_ERR(decrement_chunk(all_files[i], pool)); \
    } \
  } while (0)

static svn_error_t *
increment_chunk(struct file_info *file, apr_pool_t *pool)
{
  apr_off_t length;
  apr_off_t last_chunk = offset_to_chunk(file->size);

  if (file->chunk == -1)
    {
      /* We are at BOF (Beginning Of File). Point to first chunk/byte again. */
      file->chunk = 0;
      file->curp = file->buffer;
    }
  else if (file->chunk == last_chunk)
    {
      /* We are at the last chunk. Indicate EOF by setting curp == endp. */
      file->curp = file->endp;
    }
  else
    {
      /* There are still chunks left. Read next chunk and reset pointers. */
      file->chunk++;
      length = file->chunk == last_chunk ?
        offset_in_chunk(file->size) : CHUNK_SIZE;
      SVN_ERR(read_chunk(file->file, file->path, file->buffer,
                         length, chunk_to_offset(file->chunk),
                         pool));
      file->endp = file->buffer + length;
      file->curp = file->buffer;
    }

  return SVN_NO_ERROR;
}

static svn_error_t *
decrement_chunk(struct file_info *file, apr_pool_t *pool)
{
  if (file->chunk == 0)
    {
      /* We are already at the first chunk. Indicate BOF (Beginning Of File)
         by setting chunk = -1 and curp = endp - 1. Both conditions are
         important. They help the increment step to catch the BOF situation
         in an efficient way. */
      file->chunk--;
      file->curp = file->endp - 1;
    }
  else
    {
      /* Read previous chunk and reset pointers. */
      file->chunk--;
      SVN_ERR(read_chunk(file->file, file->path, file->buffer,
                         CHUNK_SIZE, chunk_to_offset(file->chunk),
                         pool));
      file->endp = file->buffer + CHUNK_SIZE;
      file->curp = file->endp - 1;
    }

  return SVN_NO_ERROR;
}
]]]

Cheers,

-- 
Johan