On Wed, Dec 15, 2010 at 10:58 AM, Stefan Fuhrmann <eq...@web.de> wrote: > On 15.12.2010 02:30, Stefan Fuhrmann wrote: >> >> On 14.12.2010 23:35, Johan Corveleyn wrote: >> >>> Thoughts? >> >> Two things you might try: >> * introduce a local variable for afile[i] >> * replace the for() loop with two nested ones, keeping >> calls to other functions out of the hot spot: >> >> for (i=0; i < file_len;) > > That should read: > for (i=0; i < file_len; i++) >> >> { >> /* hot spot: */ >> for(; i < file_len; i++) >> { >> curFile = afile[i]; >> if (curFile->chunk==-1) >> curFile->chunk = 0; >> else if (curFile->curp != curFile->endp -1) >> curFile->curp++; >> else >> break; >> } >> >> if (i < file_len) >> { >> /* the complex, rarely used stuff goes here */ >> } >> }
Ok, I tried this, but it didn't really help. It's still only about as fast as before. While the macro version is about 10% faster (I made a cleaner macro version, only macro'ing the hotspot stuff, with a function call to the complex, rarely used stuff). For the inline version, I tried several variations (always with APR_INLINE in the function signature, and with local variable for file[i]): with or without the nested for loop, with or without the complex stuff factored out to a separate function, ... it made no difference. Maybe I'm still doing something wrong, keeping the compiler from inlining it (but I'm not really a compiler expert, maybe I have to add some compiler options or something, ...). OTOH, I now have a macro implementation which is quite readable (and which uses the do ... while(0) construct - thanks Peter), and which does the trick. So maybe I should simply stick with that option ... FYI, in attachment some patches for the different variations (to be applied against HEAD of diff-optimizations-bytes branch). And the two main versions here below in full for easy review/discussion/context. At the bottom, some of my measurements. As always, I welcome any feedback. 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++; \ else \ SVN_ERR(increment_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); /* Unless this was the last chunk, we need to read a new chunk. */ if (file->chunk == last_chunk) { file->curp++; /* curp == endp signals end of file */ } else { 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; } ]]] APR_INLINE version (with nested for loop, and separate function) [[[ /* 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. */ static APR_INLINE svn_error_t * increment_pointers(struct file_info *file[], int file_len, apr_pool_t *pool) { struct file_info *curFile; int i; for (i = 0; i < file_len; i++) { /* hot spot: */ for(; i < file_len; i++) { curFile = file[i]; if (curFile->chunk == -1) /* indicates before beginning of file */ curFile->chunk = 0; /* point to beginning of file again */ else if (curFile->curp != curFile->endp - 1) curFile->curp++; else break; } if (i < file_len) { SVN_ERR(increment_chunk(curFile, pool)); } } return SVN_NO_ERROR; } 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); /* Unless this was the last chunk, we need to read a new chunk. */ if (file->chunk == last_chunk) { file->curp++; /* curp == endp signals end of file */ } else { 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; } ]]] Finally, some measurements (with the help of some instrumentation code in blame.c, see patch in attachment). This is timing of "svn blame -x-b" of a ~1,5 Mb file (61000 lines) with 2272 revisions (each time average of 2nd and 3rd run): Total blame time (s) diff time (s) Normal function (original version): 118 80 Inline function (w/nested for) : 118 79 Macro version : 115 72 Ok, it's nothing huge, but it makes me feel a little bit better :-). Cheers, -- Johan
Index: subversion/libsvn_diff/diff_file.c =================================================================== --- subversion/libsvn_diff/diff_file.c (revision 1050737) +++ subversion/libsvn_diff/diff_file.c (working copy) @@ -252,76 +252,87 @@ datasource_open(void *baton, svn_diff_datasource_e * 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. */ -static svn_error_t * -increment_pointers(struct file_info *file[], int file_len, apr_pool_t *pool) -{ - int i; +#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++; \ + else \ + SVN_ERR(increment_chunk(all_files[i], pool)); \ + } \ + } while (0) - for (i = 0; i < file_len; i++) - if (file[i]->chunk == -1) /* indicates before beginning of file */ - { - file[i]->chunk = 0; /* point to beginning of file again */ - } - else if (file[i]->curp == file[i]->endp - 1) - { - apr_off_t last_chunk = offset_to_chunk(file[i]->size); - if (file[i]->chunk == last_chunk) - { - file[i]->curp++; /* curp == endp signals end of file */ - } - else - { - apr_off_t length; - file[i]->chunk++; - length = file[i]->chunk == last_chunk ? - offset_in_chunk(file[i]->size) : CHUNK_SIZE; - SVN_ERR(read_chunk(file[i]->file, file[i]->path, file[i]->buffer, - length, chunk_to_offset(file[i]->chunk), - pool)); - file[i]->endp = file[i]->buffer + length; - file[i]->curp = file[i]->buffer; - } - } - else - { - file[i]->curp++; - } - return SVN_NO_ERROR; -} - /* 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 * -decrement_pointers(struct file_info *file[], int file_len, apr_pool_t *pool) +increment_chunk(struct file_info *file, apr_pool_t *pool) { - int i; + apr_off_t length; + apr_off_t last_chunk = offset_to_chunk(file->size); - for (i = 0; i < file_len; i++) - if (file[i]->curp == file[i]->buffer) - { - if (file[i]->chunk == 0) - file[i]->chunk--; /* chunk == -1 signals beginning of file */ - else - { - file[i]->chunk--; - SVN_ERR(read_chunk(file[i]->file, file[i]->path, file[i]->buffer, - CHUNK_SIZE, chunk_to_offset(file[i]->chunk), - pool)); - file[i]->endp = file[i]->buffer + CHUNK_SIZE; - file[i]->curp = file[i]->endp - 1; - } - } - else - { - file[i]->curp--; - } + /* Unless this was the last chunk, we need to read a new chunk. */ + if (file->chunk == last_chunk) + { + file->curp++; /* curp == endp signals end of file */ + } + else + { + 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) +{ + /* Unless this was the very first chunk, we need to read a new chunk. */ + if (file->chunk == 0) + file->chunk--; /* chunk == -1 signals beginning of file */ + else + { + 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; } + /* Check whether one of the FILEs has its pointers 'before' the beginning of * the file (this can happen while scanning backwards). This is the case if * one of them has chunk == -1. */
Index: subversion/libsvn_diff/diff_file.c =================================================================== --- subversion/libsvn_diff/diff_file.c (revision 1050737) +++ subversion/libsvn_diff/diff_file.c (working copy) @@ -247,45 +247,74 @@ datasource_open(void *baton, svn_diff_datasource_e } +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); + + /* Unless this was the last chunk, we need to read a new chunk. */ + if (file->chunk == last_chunk) + { + file->curp++; /* curp == endp signals end of file */ + } + else + { + 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) +{ + /* Unless this was the very first chunk, we need to read a new chunk. */ + if (file->chunk == 0) + file->chunk--; /* chunk == -1 signals beginning of file */ + else + { + 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; +} + + /* 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. */ -static svn_error_t * +static APR_INLINE svn_error_t * increment_pointers(struct file_info *file[], int file_len, apr_pool_t *pool) { + struct file_info *curFile; int i; for (i = 0; i < file_len; i++) - if (file[i]->chunk == -1) /* indicates before beginning of file */ - { - file[i]->chunk = 0; /* point to beginning of file again */ - } - else if (file[i]->curp == file[i]->endp - 1) - { - apr_off_t last_chunk = offset_to_chunk(file[i]->size); - if (file[i]->chunk == last_chunk) - { - file[i]->curp++; /* curp == endp signals end of file */ - } - else - { - apr_off_t length; - file[i]->chunk++; - length = file[i]->chunk == last_chunk ? - offset_in_chunk(file[i]->size) : CHUNK_SIZE; - SVN_ERR(read_chunk(file[i]->file, file[i]->path, file[i]->buffer, - length, chunk_to_offset(file[i]->chunk), - pool)); - file[i]->endp = file[i]->buffer + length; - file[i]->curp = file[i]->buffer; - } - } + { + curFile = file[i]; + if (curFile->chunk == -1) /* indicates before beginning of file */ + curFile->chunk = 0; /* point to beginning of file again */ + else if (curFile->curp != curFile->endp - 1) + curFile->curp++; else - { - file[i]->curp++; - } + SVN_ERR(increment_chunk(curFile, pool)); + } return SVN_NO_ERROR; } @@ -294,30 +323,20 @@ increment_pointers(struct file_info *file[], int f * 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. */ -static svn_error_t * +static APR_INLINE svn_error_t * decrement_pointers(struct file_info *file[], int file_len, apr_pool_t *pool) { + struct file_info *curFile; int i; for (i = 0; i < file_len; i++) - if (file[i]->curp == file[i]->buffer) - { - if (file[i]->chunk == 0) - file[i]->chunk--; /* chunk == -1 signals beginning of file */ - else - { - file[i]->chunk--; - SVN_ERR(read_chunk(file[i]->file, file[i]->path, file[i]->buffer, - CHUNK_SIZE, chunk_to_offset(file[i]->chunk), - pool)); - file[i]->endp = file[i]->buffer + CHUNK_SIZE; - file[i]->curp = file[i]->endp - 1; - } - } + { + curFile = file[i]; + if (curFile->curp > curFile->buffer) + curFile->curp--; else - { - file[i]->curp--; - } + SVN_ERR(decrement_chunk(curFile, pool)); + } return SVN_NO_ERROR; }
Index: subversion/libsvn_diff/diff_file.c =================================================================== --- subversion/libsvn_diff/diff_file.c (revision 1050737) +++ subversion/libsvn_diff/diff_file.c (working copy) @@ -247,46 +247,84 @@ datasource_open(void *baton, svn_diff_datasource_e } +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); + + /* Unless this was the last chunk, we need to read a new chunk. */ + if (file->chunk == last_chunk) + { + file->curp++; /* curp == endp signals end of file */ + } + else + { + 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) +{ + /* Unless this was the very first chunk, we need to read a new chunk. */ + if (file->chunk == 0) + file->chunk--; /* chunk == -1 signals beginning of file */ + else + { + 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; +} + + /* 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. */ -static svn_error_t * +static APR_INLINE svn_error_t * increment_pointers(struct file_info *file[], int file_len, apr_pool_t *pool) { + struct file_info *curFile; int i; for (i = 0; i < file_len; i++) - if (file[i]->chunk == -1) /* indicates before beginning of file */ - { - file[i]->chunk = 0; /* point to beginning of file again */ - } - else if (file[i]->curp == file[i]->endp - 1) - { - apr_off_t last_chunk = offset_to_chunk(file[i]->size); - if (file[i]->chunk == last_chunk) - { - file[i]->curp++; /* curp == endp signals end of file */ - } - else - { - apr_off_t length; - file[i]->chunk++; - length = file[i]->chunk == last_chunk ? - offset_in_chunk(file[i]->size) : CHUNK_SIZE; - SVN_ERR(read_chunk(file[i]->file, file[i]->path, file[i]->buffer, - length, chunk_to_offset(file[i]->chunk), - pool)); - file[i]->endp = file[i]->buffer + length; - file[i]->curp = file[i]->buffer; - } - } - else - { - file[i]->curp++; - } + { + /* hot spot: */ + for(; i < file_len; i++) + { + curFile = file[i]; + if (curFile->chunk == -1) /* indicates before beginning of file */ + curFile->chunk = 0; /* point to beginning of file again */ + else if (curFile->curp != curFile->endp - 1) + curFile->curp++; + else + break; + } + if (i < file_len) + { + SVN_ERR(increment_chunk(curFile, pool)); + } + } + return SVN_NO_ERROR; } @@ -294,31 +332,30 @@ increment_pointers(struct file_info *file[], int f * 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. */ -static svn_error_t * +static APR_INLINE svn_error_t * decrement_pointers(struct file_info *file[], int file_len, apr_pool_t *pool) { + struct file_info *curFile; int i; for (i = 0; i < file_len; i++) - if (file[i]->curp == file[i]->buffer) - { - if (file[i]->chunk == 0) - file[i]->chunk--; /* chunk == -1 signals beginning of file */ - else - { - file[i]->chunk--; - SVN_ERR(read_chunk(file[i]->file, file[i]->path, file[i]->buffer, - CHUNK_SIZE, chunk_to_offset(file[i]->chunk), - pool)); - file[i]->endp = file[i]->buffer + CHUNK_SIZE; - file[i]->curp = file[i]->endp - 1; - } - } - else - { - file[i]->curp--; - } + { + /* hot spot: */ + for(; i < file_len; i++) + { + curFile = file[i]; + if (curFile->curp > curFile->buffer) + curFile->curp--; + else + break; + } + if (i < file_len) + { + SVN_ERR(decrement_chunk(curFile, pool)); + } + } + return SVN_NO_ERROR; }
Index: subversion/libsvn_client/blame.c =================================================================== --- subversion/libsvn_client/blame.c (revision 1041582) +++ subversion/libsvn_client/blame.c (working copy) @@ -22,6 +22,7 @@ */ #include <apr_pools.h> +#include <apr_time.h> #include "client.h" @@ -42,6 +43,12 @@ #include <assert.h> +static long diff_time = 0L; +static long blame_process_time = 0L; +static long delta_process_time = 0L; +static long file_rev_time = 0L; +static long window_time = 0L; + /* The metadata associated with a particular revision. */ struct rev { @@ -260,7 +267,9 @@ output_diff_modified(void *baton, struct diff_baton *db = baton; if (original_length) - SVN_ERR(blame_delete_range(db->chain, modified_start, original_length)); + { + SVN_ERR(blame_delete_range(db->chain, modified_start, original_length)); + } if (modified_length) SVN_ERR(blame_insert_range(db->chain, db->rev, modified_start, @@ -285,10 +294,16 @@ add_file_blame(const char *last_file, const svn_diff_file_options_t *diff_options, apr_pool_t *pool) { + apr_time_t start_time; + long result_time; + if (!last_file) { SVN_ERR_ASSERT(chain->blame == NULL); + start_time = apr_time_now(); chain->blame = blame_create(chain, rev, 0); + result_time = apr_time_now() - start_time; + blame_process_time += result_time; } else { @@ -299,9 +314,15 @@ add_file_blame(const char *last_file, diff_baton.rev = rev; /* We have a previous file. Get the diff and adjust blame info. */ + start_time = apr_time_now(); SVN_ERR(svn_diff_file_diff_2(&diff, last_file, cur_file, diff_options, pool)); + result_time = apr_time_now() - start_time; + diff_time += result_time; + start_time = apr_time_now(); SVN_ERR(svn_diff_output(diff, &diff_baton, &output_fns)); + result_time = apr_time_now() - start_time; + blame_process_time += result_time; } return SVN_NO_ERROR; @@ -313,13 +334,23 @@ window_handler(svn_txdelta_window_t *window, void struct delta_baton *dbaton = baton; struct file_rev_baton *frb = dbaton->file_rev_baton; struct blame_chain *chain; + apr_time_t wrapped_start_time, start_time; + long wrapped_result_time, result_time; + start_time = apr_time_now(); /* Call the wrapped handler first. */ + wrapped_start_time = apr_time_now(); SVN_ERR(dbaton->wrapped_handler(window, dbaton->wrapped_baton)); + wrapped_result_time = apr_time_now() - wrapped_start_time; + delta_process_time += wrapped_result_time; /* We patiently wait for the NULL window marking the end. */ if (window) + { + result_time = apr_time_now() - start_time; + window_time += result_time; return SVN_NO_ERROR; + } /* Close the files used for the delta. It is important to do this early, since otherwise, they will be deleted @@ -375,6 +406,8 @@ window_handler(svn_txdelta_window_t *window, void frb->currpool = tmp_pool; } + result_time = apr_time_now() - start_time; + window_time += result_time; return SVN_NO_ERROR; } @@ -415,7 +448,10 @@ file_rev_handler(void *baton, const char *path, sv svn_stream_t *cur_stream; struct delta_baton *delta_baton; apr_pool_t *filepool; + apr_time_t start_time; + long result_time; + start_time = apr_time_now(); /* Clear the current pool. */ svn_pool_clear(frb->currpool); @@ -446,7 +482,11 @@ file_rev_handler(void *baton, const char *path, sv since the tempfile will be removed by the pool and we need the tempfile from the last revision with content changes. */ if (!content_delta_handler) + { + result_time = apr_time_now() - start_time; + file_rev_time += result_time; return SVN_NO_ERROR; + } frb->merged_revision = merged_revision; @@ -511,6 +551,8 @@ file_rev_handler(void *baton, const char *path, sv if (frb->include_merged_revisions) frb->rev->path = apr_pstrdup(frb->mainpool, path); + result_time = apr_time_now() - start_time; + file_rev_time += result_time; return SVN_NO_ERROR; } @@ -600,6 +642,8 @@ svn_client_blame5(const char *target, svn_stream_t *last_stream; svn_stream_t *stream; const char *target_abspath_or_url; + apr_time_t starttime; + long result_time; if (start->kind == svn_opt_revision_unspecified || end->kind == svn_opt_revision_unspecified) @@ -666,10 +710,18 @@ svn_client_blame5(const char *target, We need to ensure that we get one revision before the start_rev, if available so that we can know what was actually changed in the start revision. */ + starttime = apr_time_now(); SVN_ERR(svn_ra_get_file_revs2(ra_session, "", start_revnum - (start_revnum > 0 ? 1 : 0), end_revnum, include_merged_revisions, file_rev_handler, &frb, pool)); + result_time = apr_time_now() - starttime; + fprintf(stderr, "### blame took %d usec (%d s)\n", result_time, result_time/1000000L); + fprintf(stderr, "### file_rev_handler: %d (%d s) - window_handler: %d (%d s)\n", + file_rev_time, file_rev_time/1000000L, window_time, window_time/1000000L); + fprintf(stderr, "### wrapped_handler: %d (%d s) - diff: %d (%d s) - blame_process: %d (%d s)\n\n", + delta_process_time, delta_process_time/1000000L, diff_time, diff_time/1000000L, + blame_process_time, blame_process_time/1000000L); if (end->kind == svn_opt_revision_working) {