You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by hw...@apache.org on 2010/09/15 08:52:07 UTC
svn commit: r997203 - in /subversion/trunk: ./ subversion/include/
subversion/libsvn_client/ subversion/libsvn_diff/ subversion/libsvn_fs_fs/
subversion/libsvn_ra_svn/ subversion/libsvn_repos/ subversion/libsvn_subr/
subversion/libsvn_wc/ subversion/sv...
Author: hwright
Date: Wed Sep 15 06:52:06 2010
New Revision: 997203
URL: http://svn.apache.org/viewvc?rev=997203&view=rev
Log:
Merge r985037, r985046, r995507 and r995603 from the performance branch.
These changes introduce the svn_stringbuf_appendbyte() function, which has
significantly less overhead than svn_stringbuf_appendbytes(), and can be
used in a number of places within our codebase.
r995507 is the removal of an extra function on the branch. This function
conflicted with previous revisions in the merge, and is itself no longer on
trunk.
Modified:
subversion/trunk/ (props changed)
subversion/trunk/subversion/include/svn_string.h
subversion/trunk/subversion/libsvn_client/deprecated.c
subversion/trunk/subversion/libsvn_diff/diff_file.c
subversion/trunk/subversion/libsvn_diff/parse-diff.c
subversion/trunk/subversion/libsvn_fs_fs/lock.c
subversion/trunk/subversion/libsvn_ra_svn/marshal.c
subversion/trunk/subversion/libsvn_repos/dump.c
subversion/trunk/subversion/libsvn_subr/config_file.c
subversion/trunk/subversion/libsvn_subr/prompt.c
subversion/trunk/subversion/libsvn_subr/quoprint.c
subversion/trunk/subversion/libsvn_subr/skel.c
subversion/trunk/subversion/libsvn_subr/stream.c
subversion/trunk/subversion/libsvn_subr/subst.c
subversion/trunk/subversion/libsvn_subr/svn_string.c
subversion/trunk/subversion/libsvn_wc/old-and-busted.c
subversion/trunk/subversion/libsvn_wc/props.c
subversion/trunk/subversion/svn/util.c
subversion/trunk/subversion/svndumpfilter/main.c
subversion/trunk/subversion/tests/libsvn_subr/skel-test.c
subversion/trunk/subversion/tests/libsvn_subr/stream-test.c
Propchange: subversion/trunk/
------------------------------------------------------------------------------
--- svn:mergeinfo (original)
+++ svn:mergeinfo Wed Sep 15 06:52:06 2010
@@ -22,7 +22,7 @@
/subversion/branches/log-g-performance:870941-871032
/subversion/branches/merge-skips-obstructions:874525-874615
/subversion/branches/nfc-nfd-aware-client:870276,870376
-/subversion/branches/performance:983764,983766,984927,985014,985669,987893
+/subversion/branches/performance:983764,983766,984927,985014,985037,985046,985669,987893,995507,995603
/subversion/branches/ra_serf-digest-authn:875693-876404
/subversion/branches/reintegrate-improvements:873853-874164
/subversion/branches/subtree-mergeinfo:876734-878766
Modified: subversion/trunk/subversion/include/svn_string.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_string.h?rev=997203&r1=997202&r2=997203&view=diff
==============================================================================
--- subversion/trunk/subversion/include/svn_string.h (original)
+++ subversion/trunk/subversion/include/svn_string.h Wed Sep 15 06:52:06 2010
@@ -253,6 +253,15 @@ svn_stringbuf_chop(svn_stringbuf_t *str,
void
svn_stringbuf_fillchar(svn_stringbuf_t *str, unsigned char c);
+/** Append a single character @a byte onto @a targetstr.
+ *
+ * reallocs if necessary. @a targetstr is affected, nothing else is.
+ * @since New in 1.7.
+ */
+void
+svn_stringbuf_appendbyte(svn_stringbuf_t *targetstr,
+ char byte);
+
/** Append an array of bytes onto @a targetstr.
*
* reallocs if necessary. @a targetstr is affected, nothing else is.
Modified: subversion/trunk/subversion/libsvn_client/deprecated.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/deprecated.c?rev=997203&r1=997202&r2=997203&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_client/deprecated.c (original)
+++ subversion/trunk/subversion/libsvn_client/deprecated.c Wed Sep 15 06:52:06 2010
@@ -317,7 +317,7 @@ wrapped_receiver(void *baton,
struct wrapped_receiver_baton_s *b = baton;
svn_stringbuf_t *expanded_line = svn_stringbuf_create(line, pool);
- svn_stringbuf_appendbytes(expanded_line, "\r", 1);
+ svn_stringbuf_appendbyte(expanded_line, '\r');
return b->orig_receiver(b->orig_baton, line_no, revision, author,
date, expanded_line->data, pool);
Modified: subversion/trunk/subversion/libsvn_diff/diff_file.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_diff/diff_file.c?rev=997203&r1=997202&r2=997203&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_diff/diff_file.c (original)
+++ subversion/trunk/subversion/libsvn_diff/diff_file.c Wed Sep 15 06:52:06 2010
@@ -860,7 +860,7 @@ output_unified_line(svn_diff__file_outpu
{
if (type != svn_diff__file_output_unified_skip)
{
- svn_stringbuf_appendbytes(baton->hunk, curp, 1);
+ svn_stringbuf_appendbyte(baton->hunk, *curp);
}
/* We don't append the LF to extra_context, since it would
* just be stripped anyway. */
Modified: subversion/trunk/subversion/libsvn_diff/parse-diff.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_diff/parse-diff.c?rev=997203&r1=997202&r2=997203&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_diff/parse-diff.c (original)
+++ subversion/trunk/subversion/libsvn_diff/parse-diff.c Wed Sep 15 06:52:06 2010
@@ -205,7 +205,7 @@ parse_hunk_header(const char *header, sv
p++;
while (*p && *p != ' ')
{
- svn_stringbuf_appendbytes(range, p, 1);
+ svn_stringbuf_appendbyte(range, *p);
p++;
}
if (*p != ' ')
@@ -226,7 +226,7 @@ parse_hunk_header(const char *header, sv
p++;
while (*p && *p != ' ')
{
- svn_stringbuf_appendbytes(range, p, 1);
+ svn_stringbuf_appendbyte(range, *p);
p++;
}
if (*p != ' ')
@@ -362,7 +362,7 @@ hunk_readline(svn_stream_t *stream,
else
match = eol_str;
- svn_stringbuf_appendbytes(str, &c, 1);
+ svn_stringbuf_appendbyte(str, c);
}
svn_stringbuf_chop(str, match - eol_str);
Modified: subversion/trunk/subversion/libsvn_fs_fs/lock.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/lock.c?rev=997203&r1=997202&r2=997203&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/lock.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/lock.c Wed Sep 15 06:52:06 2010
@@ -192,7 +192,7 @@ write_digest_file(apr_hash_t *children,
svn_stringbuf_appendbytes(children_list,
svn__apr_hash_index_key(hi),
svn__apr_hash_index_klen(hi));
- svn_stringbuf_appendbytes(children_list, "\n", 1);
+ svn_stringbuf_appendbyte(children_list, '\n');
}
hash_store(hash, CHILDREN_KEY, sizeof(CHILDREN_KEY)-1,
children_list->data, children_list->len, pool);
Modified: subversion/trunk/subversion/libsvn_ra_svn/marshal.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_svn/marshal.c?rev=997203&r1=997202&r2=997203&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_svn/marshal.c (original)
+++ subversion/trunk/subversion/libsvn_ra_svn/marshal.c Wed Sep 15 06:52:06 2010
@@ -631,7 +631,7 @@ static svn_error_t *read_item(svn_ra_svn
SVN_ERR(readbuf_getchar(conn, pool, &c));
if (!svn_ctype_isalnum(c) && c != '-')
break;
- svn_stringbuf_appendbytes(str, &c, 1);
+ svn_stringbuf_appendbyte(str, c);
}
item->kind = SVN_RA_SVN_WORD;
item->u.word = str->data;
Modified: subversion/trunk/subversion/libsvn_repos/dump.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/dump.c?rev=997203&r1=997202&r2=997203&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_repos/dump.c (original)
+++ subversion/trunk/subversion/libsvn_repos/dump.c Wed Sep 15 06:52:06 2010
@@ -42,6 +42,7 @@
/*----------------------------------------------------------------------*/
+
/* Compute the delta between OLDROOT/OLDPATH and NEWROOT/NEWPATH and
store it into a new temporary file *TEMPFILE. OLDROOT may be NULL,
in which case the delta will be computed against an empty file, as
Modified: subversion/trunk/subversion/libsvn_subr/config_file.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/config_file.c?rev=997203&r1=997202&r2=997203&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/config_file.c (original)
+++ subversion/trunk/subversion/libsvn_subr/config_file.c Wed Sep 15 06:52:06 2010
@@ -163,7 +163,7 @@ parse_value(int *pch, parse_context_t *c
/* last ch seen was ':' or '=' in parse_option. */
{
const char char_from_int = ch;
- svn_stringbuf_appendbytes(ctx->value, &char_from_int, 1);
+ svn_stringbuf_appendbyte(ctx->value, char_from_int);
SVN_ERR(parser_getc(ctx, &ch));
}
/* Leading and trailing whitespace is ignored. */
@@ -214,13 +214,12 @@ parse_value(int *pch, parse_context_t *c
else
{
/* This is a continuation line. Read it. */
- svn_stringbuf_appendbytes(ctx->value, " ", 1);
+ svn_stringbuf_appendbyte(ctx->value, ' ');
while (ch != EOF && ch != '\n')
{
const char char_from_int = ch;
- svn_stringbuf_appendbytes(ctx->value,
- &char_from_int, 1);
+ svn_stringbuf_appendbyte(ctx->value, char_from_int);
SVN_ERR(parser_getc(ctx, &ch));
}
/* Trailing whitespace is ignored. */
@@ -247,7 +246,7 @@ parse_option(int *pch, parse_context_t *
while (ch != EOF && ch != ':' && ch != '=' && ch != '\n')
{
const char char_from_int = ch;
- svn_stringbuf_appendbytes(ctx->option, &char_from_int, 1);
+ svn_stringbuf_appendbyte(ctx->option, char_from_int);
SVN_ERR(parser_getc(ctx, &ch));
}
@@ -290,7 +289,7 @@ parse_section_name(int *pch, parse_conte
while (ch != EOF && ch != ']' && ch != '\n')
{
const char char_from_int = ch;
- svn_stringbuf_appendbytes(ctx->section, &char_from_int, 1);
+ svn_stringbuf_appendbyte(ctx->section, char_from_int);
SVN_ERR(parser_getc(ctx, &ch));
}
Modified: subversion/trunk/subversion/libsvn_subr/prompt.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/prompt.c?rev=997203&r1=997202&r2=997203&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/prompt.c (original)
+++ subversion/trunk/subversion/libsvn_subr/prompt.c Wed Sep 15 06:52:06 2010
@@ -147,7 +147,7 @@ prompt(const char **result,
SVN_ERR_MALFUNCTION();
}
- svn_stringbuf_appendbytes(strbuf, &c, 1);
+ svn_stringbuf_appendbyte(strbuf, c);
}
}
else
Modified: subversion/trunk/subversion/libsvn_subr/quoprint.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/quoprint.c?rev=997203&r1=997202&r2=997203&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/quoprint.c (original)
+++ subversion/trunk/subversion/libsvn_subr/quoprint.c Wed Sep 15 06:52:06 2010
@@ -92,7 +92,7 @@ encode_bytes(svn_stringbuf_t *str, const
/* Encode this character. */
if (ENCODE_AS_LITERAL(*p))
{
- svn_stringbuf_appendbytes(str, p, 1);
+ svn_stringbuf_appendbyte(str, *p);
(*linelen)++;
}
else
@@ -218,7 +218,7 @@ decode_bytes(svn_stringbuf_t *str, const
{
/* Literal character; append it if it's valid as such. */
if (VALID_LITERAL(*inbuf))
- svn_stringbuf_appendbytes(str, inbuf, 1);
+ svn_stringbuf_appendbyte(str, *inbuf);
*inbuflen = 0;
}
else if (*inbuf == '=' && *inbuflen == 2 && inbuf[1] == '\n')
@@ -234,7 +234,7 @@ decode_bytes(svn_stringbuf_t *str, const
if (find1 != NULL && find2 != NULL)
{
c = ((find1 - hextab) << 4) | (find2 - hextab);
- svn_stringbuf_appendbytes(str, &c, 1);
+ svn_stringbuf_appendbyte(str, c);
}
*inbuflen = 0;
}
Modified: subversion/trunk/subversion/libsvn_subr/skel.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/skel.c?rev=997203&r1=997202&r2=997203&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/skel.c (original)
+++ subversion/trunk/subversion/libsvn_subr/skel.c Wed Sep 15 06:52:06 2010
@@ -535,7 +535,7 @@ unparse(const svn_skel_t *skel, svn_stri
}
/* Emit a closing parenthesis. */
- svn_stringbuf_appendbytes(str, ")", 1);
+ svn_stringbuf_appendbyte(str, ')');
}
return str;
Modified: subversion/trunk/subversion/libsvn_subr/stream.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/stream.c?rev=997203&r1=997202&r2=997203&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/stream.c (original)
+++ subversion/trunk/subversion/libsvn_subr/stream.c Wed Sep 15 06:52:06 2010
@@ -296,7 +296,7 @@ stream_readline(svn_stringbuf_t **string
else
match = eol_str;
- svn_stringbuf_appendbytes(str, &c, 1);
+ svn_stringbuf_appendbyte(str, c);
}
*eof = FALSE;
Modified: subversion/trunk/subversion/libsvn_subr/subst.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/subst.c?rev=997203&r1=997202&r2=997203&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/subst.c (original)
+++ subversion/trunk/subversion/libsvn_subr/subst.c Wed Sep 15 06:52:06 2010
@@ -211,10 +211,10 @@ keyword_printf(const char *fmt,
svn_stringbuf_appendcstr(value, url);
break;
case '%': /* '%%' => a literal % */
- svn_stringbuf_appendbytes(value, cur, 1);
+ svn_stringbuf_appendbyte(value, *cur);
break;
case '\0': /* '%' as the last character of the string. */
- svn_stringbuf_appendbytes(value, cur, 1);
+ svn_stringbuf_appendbyte(value, *cur);
/* Now go back one character, since this was just a one character
* sequence, whereas all others are two characters, and we do not
* want to skip the null terminator entirely and carry on
Modified: subversion/trunk/subversion/libsvn_subr/svn_string.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/svn_string.c?rev=997203&r1=997202&r2=997203&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/svn_string.c (original)
+++ subversion/trunk/subversion/libsvn_subr/svn_string.c Wed Sep 15 06:52:06 2010
@@ -389,6 +389,34 @@ svn_stringbuf_ensure(svn_stringbuf_t *st
void
+svn_stringbuf_appendbyte(svn_stringbuf_t *str, char byte)
+{
+ /* In most cases, there will be pre-allocated memory left
+ * to just write the new byte at the end of the used section
+ * and terminate the string properly.
+ */
+ apr_size_t old_len = str->len;
+ if (str->blocksize > old_len + 1)
+ {
+ char *dest = str->data;
+
+ dest[old_len] = byte;
+ dest[old_len+1] = '\0';
+
+ str->len = old_len+1;
+ }
+ else
+ {
+ /* we need to re-allocate the string buffer
+ * -> let the more generic implementation take care of that part
+ */
+ char b = byte;
+ svn_stringbuf_appendbytes(str, &b, 1);
+ }
+}
+
+
+void
svn_stringbuf_appendbytes(svn_stringbuf_t *str, const char *bytes,
apr_size_t count)
{
Modified: subversion/trunk/subversion/libsvn_wc/old-and-busted.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/old-and-busted.c?rev=997203&r1=997202&r2=997203&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_wc/old-and-busted.c (original)
+++ subversion/trunk/subversion/libsvn_wc/old-and-busted.c Wed Sep 15 06:52:06 2010
@@ -164,7 +164,7 @@ read_str(const char **result,
svn_stringbuf_appendbytes(s, start, *buf - start);
(*buf)++;
SVN_ERR(read_escaped(&c, buf, end));
- svn_stringbuf_appendbytes(s, &c, 1);
+ svn_stringbuf_appendbyte(s, c);
start = *buf;
}
else
Modified: subversion/trunk/subversion/libsvn_wc/props.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/props.c?rev=997203&r1=997202&r2=997203&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_wc/props.c (original)
+++ subversion/trunk/subversion/libsvn_wc/props.c Wed Sep 15 06:52:06 2010
@@ -2268,7 +2268,7 @@ svn_wc_canonicalize_svn_prop(const svn_s
if (propval->data[propval->len - 1] != '\n')
{
new_value = svn_stringbuf_create_from_string(propval, pool);
- svn_stringbuf_appendbytes(new_value, "\n", 1);
+ svn_stringbuf_appendbyte(new_value, '\n');
}
/* Make sure this is a valid externals property. Do not
Modified: subversion/trunk/subversion/svn/util.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/svn/util.c?rev=997203&r1=997202&r2=997203&view=diff
==============================================================================
--- subversion/trunk/subversion/svn/util.c (original)
+++ subversion/trunk/subversion/svn/util.c Wed Sep 15 06:52:06 2010
@@ -764,9 +764,9 @@ svn_cl__get_log_message(const char **log
&& item->state_flags & SVN_CLIENT_COMMIT_ITEM_LOCK_TOKEN)
unlock = 'U';
- svn_stringbuf_appendbytes(tmp_message, &text_mod, 1);
- svn_stringbuf_appendbytes(tmp_message, &prop_mod, 1);
- svn_stringbuf_appendbytes(tmp_message, &unlock, 1);
+ svn_stringbuf_appendbyte(tmp_message, text_mod);
+ svn_stringbuf_appendbyte(tmp_message, prop_mod);
+ svn_stringbuf_appendbyte(tmp_message, unlock);
if (item->state_flags & SVN_CLIENT_COMMIT_ITEM_IS_COPY)
/* History included via copy/move. */
svn_stringbuf_appendcstr(tmp_message, "+ ");
Modified: subversion/trunk/subversion/svndumpfilter/main.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/svndumpfilter/main.c?rev=997203&r1=997202&r2=997203&view=diff
==============================================================================
--- subversion/trunk/subversion/svndumpfilter/main.c (original)
+++ subversion/trunk/subversion/svndumpfilter/main.c Wed Sep 15 06:52:06 2010
@@ -91,20 +91,20 @@ write_prop_to_stringbuf(svn_stringbuf_t
bytes_used = apr_snprintf(buf, sizeof(buf), "%d", namelen);
svn_stringbuf_appendbytes(*strbuf, buf, bytes_used);
- svn_stringbuf_appendbytes(*strbuf, "\n", 1);
+ svn_stringbuf_appendbyte(*strbuf, '\n');
svn_stringbuf_appendbytes(*strbuf, name, namelen);
- svn_stringbuf_appendbytes(*strbuf, "\n", 1);
+ svn_stringbuf_appendbyte(*strbuf, '\n');
/* Output value length, then value. */
svn_stringbuf_appendbytes(*strbuf, "V ", 2);
bytes_used = apr_snprintf(buf, sizeof(buf), "%" APR_SIZE_T_FMT, value->len);
svn_stringbuf_appendbytes(*strbuf, buf, bytes_used);
- svn_stringbuf_appendbytes(*strbuf, "\n", 1);
+ svn_stringbuf_appendbyte(*strbuf, '\n');
svn_stringbuf_appendbytes(*strbuf, value->data, value->len);
- svn_stringbuf_appendbytes(*strbuf, "\n", 1);
+ svn_stringbuf_appendbyte(*strbuf, '\n');
}
@@ -365,19 +365,19 @@ output_revision(struct revision_baton_t
bytes_used = apr_snprintf(buf, sizeof(buf), ": %" APR_SIZE_T_FMT,
props->len);
svn_stringbuf_appendbytes(rb->header, buf, bytes_used);
- svn_stringbuf_appendbytes(rb->header, "\n", 1);
+ svn_stringbuf_appendbyte(rb->header, '\n');
}
svn_stringbuf_appendcstr(rb->header, SVN_REPOS_DUMPFILE_CONTENT_LENGTH);
bytes_used = apr_snprintf(buf, sizeof(buf), ": %" APR_SIZE_T_FMT, props->len);
svn_stringbuf_appendbytes(rb->header, buf, bytes_used);
- svn_stringbuf_appendbytes(rb->header, "\n", 1);
+ svn_stringbuf_appendbyte(rb->header, '\n');
/* put an end to headers */
- svn_stringbuf_appendbytes(rb->header, "\n", 1);
+ svn_stringbuf_appendbyte(rb->header, '\n');
/* put an end to revision */
- svn_stringbuf_appendbytes(props, "\n", 1);
+ svn_stringbuf_appendbyte(props, '\n');
/* write out the revision */
/* Revision is written out in the following cases:
@@ -633,7 +633,7 @@ output_node(struct node_baton_t *nb)
bytes_used = apr_snprintf(buf, sizeof(buf), ": %" APR_SIZE_T_FMT,
nb->props->len);
svn_stringbuf_appendbytes(nb->header, buf, bytes_used);
- svn_stringbuf_appendbytes(nb->header, "\n", 1);
+ svn_stringbuf_appendbyte(nb->header, '\n');
}
if (nb->has_text)
{
@@ -642,16 +642,16 @@ output_node(struct node_baton_t *nb)
bytes_used = apr_snprintf(buf, sizeof(buf), ": %" SVN_FILESIZE_T_FMT,
nb->tcl);
svn_stringbuf_appendbytes(nb->header, buf, bytes_used);
- svn_stringbuf_appendbytes(nb->header, "\n", 1);
+ svn_stringbuf_appendbyte(nb->header, '\n');
}
svn_stringbuf_appendcstr(nb->header, SVN_REPOS_DUMPFILE_CONTENT_LENGTH);
bytes_used = apr_snprintf(buf, sizeof(buf), ": %" SVN_FILESIZE_T_FMT,
(svn_filesize_t) (nb->props->len + nb->tcl));
svn_stringbuf_appendbytes(nb->header, buf, bytes_used);
- svn_stringbuf_appendbytes(nb->header, "\n", 1);
+ svn_stringbuf_appendbyte(nb->header, '\n');
/* put an end to headers */
- svn_stringbuf_appendbytes(nb->header, "\n", 1);
+ svn_stringbuf_appendbyte(nb->header, '\n');
/* 3. output all the stuff */
Modified: subversion/trunk/subversion/tests/libsvn_subr/skel-test.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_subr/skel-test.c?rev=997203&r1=997202&r2=997203&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/libsvn_subr/skel-test.c (original)
+++ subversion/trunk/subversion/tests/libsvn_subr/skel-test.c Wed Sep 15 06:52:06 2010
@@ -184,9 +184,9 @@ put_implicit_length_byte(svn_stringbuf_t
&& ! skel_is_space(term)
&& ! skel_is_paren(term))
abort();
- svn_stringbuf_appendbytes(str, &byte, 1);
+ svn_stringbuf_appendbyte(str, byte);
if (term != '\0')
- svn_stringbuf_appendbytes(str, &term, 1);
+ svn_stringbuf_appendbyte(str, term);
}
@@ -239,7 +239,7 @@ put_implicit_length_all_chars(svn_string
svn_stringbuf_appendbytes(str, name, len);
if (term != '\0')
- svn_stringbuf_appendbytes(str, &term, 1);
+ svn_stringbuf_appendbyte(str, term);
}
@@ -461,7 +461,7 @@ put_list_start(svn_stringbuf_t *str, cha
svn_stringbuf_appendcstr(str, "(");
for (i = 0; i < len; i++)
- svn_stringbuf_appendbytes(str, &space, 1);
+ svn_stringbuf_appendbyte(str, space);
}
@@ -476,7 +476,7 @@ put_list_end(svn_stringbuf_t *str, char
abort();
for (i = 0; i < len; i++)
- svn_stringbuf_appendbytes(str, &space, 1);
+ svn_stringbuf_appendbyte(str, space);
svn_stringbuf_appendcstr(str, ")");
}
Modified: subversion/trunk/subversion/tests/libsvn_subr/stream-test.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_subr/stream-test.c?rev=997203&r1=997202&r2=997203&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/libsvn_subr/stream-test.c (original)
+++ subversion/trunk/subversion/tests/libsvn_subr/stream-test.c Wed Sep 15 06:52:06 2010
@@ -128,7 +128,7 @@ generate_test_bytes(int num_bytes, apr_p
for (total = 0, repeat = repeat_iter = 1, c = 0; total < num_bytes; total++)
{
- svn_stringbuf_appendbytes(buffer, &c, 1);
+ svn_stringbuf_appendbyte(buffer, c);
repeat_iter--;
if (repeat_iter == 0)
Re: svn commit: r997203 - in /subversion/trunk: ./
subversion/include/ subversion/libsvn_client/ subversion/libsvn_diff/
subversion/libsvn_fs_fs/ subversion/libsvn_ra_svn/
subversion/libsvn_repos/ subversion/libsvn_subr/ subversion/libsvn_wc/
subversion/sv...
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Sep 15, 2010 at 06:52:07AM -0000, hwright@apache.org wrote:
> ==============================================================================
> --- subversion/trunk/subversion/include/svn_string.h (original)
> +++ subversion/trunk/subversion/include/svn_string.h Wed Sep 15 06:52:06 2010
> @@ -253,6 +253,15 @@ svn_stringbuf_chop(svn_stringbuf_t *str,
> void
> svn_stringbuf_fillchar(svn_stringbuf_t *str, unsigned char c);
>
> +/** Append a single character @a byte onto @a targetstr.
> + *
> + * reallocs if necessary. @a targetstr is affected, nothing else is.
> + * @since New in 1.7.
> + */
> +void
> +svn_stringbuf_appendbyte(svn_stringbuf_t *targetstr,
> + char byte);
> +
The docstring should list advantages svn_stringbuf_appendbyte(buf, c)
has over svn_stringbuf_appendbytes(buf, &c, 1). We need to understand
where the performance benefits really come from. IIRC the benefit was
dependent on the optimizer in the compiler to some extent, is this correct?
Stefan
Re: Performance optimization - svn_stringbuf_appendbyte()
Posted by Philip Martin <ph...@wandisco.com>.
Julian Foad <ju...@wandisco.com> writes:
> This tells me that the hand-optimization is actually harmful and the
> compiler does a 10% better job by itself.
>
> Have I made a mistake?
>
> What are the results for your system?
>
> (I'm using GCC 4.4.1 on an Intel Centrino laptop CPU.)
On my system (Intel Core 2 Duo, GCC 4.3.2, 64bit), the difference is
smaller, about 5%.
--
Philip
Re: Performance optimization - svn_stringbuf_appendbyte()
Posted by Stefan Fuhrmann <st...@alice-dsl.de>.
On 11.10.2010 17:07, Julian Foad wrote:
> On Sun, 2010-10-10, Stefan Fuhrmann wrote:
>> On 07.10.2010 16:07, Julian Foad wrote:
>>>> New Revision: 997203
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=997203&view=rev
>>>> Log:
>>>> Merge r985037, r985046, r995507 and r995603 from the performance branch.
>>>>
>>>> These changes introduce the svn_stringbuf_appendbyte() function, which has
>>>> significantly less overhead than svn_stringbuf_appendbytes(), and can be
>>>> used in a number of places within our codebase.
>>> Here are the results (see full patch attached):
>>>
>>> Times: appendbytes appendbyte0 appendbyte (in ms)
>>> run: 89 31 34
>>> run: 88 30 35
>>> run: 88 31 34
>>> run: 88 30 34
>>> run: 88 31 34
>>> min: 88 30 34
>>>
>>> This tells me that the hand-optimization is actually harmful and the
>>> compiler does a 10% better job by itself.
>>>
>>> Have I made a mistake?
>> My guess is that you might have made two mistakes actually.
> Heh, OK.
>
>> First, the number of operations was relatively low - everything
>> in the low ms range could be due to secondary effects (and
>> still be repeatable).
> I can't think of any reason why. I ran the whole benchmark lots of
> times. Occasionally some of the times were a big chunk longer due to
> other system activity. Normally they were stable. I also ran it
> several times with 10 million ops instead of 1 million, and the results
> were exactly 10x longer with the same degree of variability.
>
OK. It is just that sometimes, process startup or caching
artifacts (especially running tests in a particular order)
result in producible effects of that magnitude. But obviously
that was not the case here.
>> The most important problem would be compiler optimizations
>> or lack thereof. Since the numbers are very large (50..100
>> ticks per byte, depending on your CPU clock), I would assume
>> that you used a normal debug build for testing. In that case,
> You're right. I'll try again, with "--disable-debug -O2".
Gotcha! The optimizer's impact is massive on that kind
of code.
>> the actual number of C statements has a large impact on
>> the execution time. See my results below for details.
>>> What are the results for your system?
>>>
>>> (I'm using GCC 4.4.1 on an Intel Centrino laptop CPU.)
>>>
>> Test code used (10^10 calls, newer re-allocate):
>>
>> int i;
>> unsigned char c;
>>
>> svn_stringbuf_t* s = svn_stringbuf_create_ensure (255, pool);
> OK so you're avoiding any calls to the "re-alloc". My tests included
> re-allocs, but were long enough strings that this wasn't much overhead.
> Nevertheless I'll avoid re-allocs so we can compare results fairly.
>
>> for (i = 0; i< 50000000; ++i)
>> {
>> s->len = 0;
>> for (c = 0; c< 200; ++c)
>> svn_stringbuf_appendbyte (s, c);
>> }
>>
>>
>> XEON 55xx / Core i7, hyper-threading on, 3GHz peak
>> 64 bits Linux, GCC 4.3.3; ./configure --disable-debug
>>
>> (1) 10,7s; IPC = 2.1
>> (2) 8,11s; IPC = 1.4
>> (3) 2,64s; IPC = 2.4
>> (4) 2,43s; IPC = 2.3
Grml. After normalizing the numbers to 10^9 iterations,
I forgot to adjust the example code. Sorry!
>> (1) use appendbytes gives 9 instructions in main, 59 in appbytes
>> (2) handle count==1 directly in in appendbytes; 9 inst. in main, 26 in
>> appbytes
>> (3) appendbyte0 (same compiler output as the the non-handtuned code);
>> 13 inst. in appbyte, 6 in main
>> (4) trunk@HEAD appendbyte; 11 inst. in appbyte, 6 in main
>>
>> Core2 2.4GHz, Win32, VS2010
>> (not using valgrind to count instructions here; also not testing (2))
>>
>> (1) 17,0s release, 20,2s debug
>> (3) 10,6s release, 12,2s debug
>> (4) 9,7s release, 13,0s debug
> With a test harness more similar to yours, and built with
> "--disable-debug -O2", here are my relative numbers (but only 1 million
> outer loops, compared to your 50 million):
>
> $ svn-c-test subr string 24
> Times for 1000000 x 200 bytes, in seconds
> (1)appendbytes (3)appendbyte0 (4)trunk@HEAD (5)macro
> run: 7.03 2.06 1.37 0.53
> run: 6.62 1.76 1.55 0.53
> run: 6.53 1.67 1.44 0.53
> run: 6.54 1.60 1.44 0.53
> run: 6.52 1.84 1.37 0.53
> min: 6.52 1.60 1.37 0.53
>
> This agrees with what you found: the hand-tuned code gives a 10%
> improvement.
>
> This also shows another variant, writing the function as a macro,
> reported in the column "(5)macro". This gives a further two-and-a-half
> times speed-up in this test.
>
The macro variant (basically inlining) would take 2.65s for 10^9
total iterations. That is more or less what I got on Core i7 for the
non-inlined variant. My theory:
* function call & return are cheap jumps
* the code is small enough for the loop stream decoder to kick in
* you platform ABI uses stack frames and probably passes parameters
on the stack (instead of registers).
The latter is also the case for 32 bit Windows and gives similar
results (9.7s vs. your 6.8s).
> [...]
>> * your numbers seem very large in comparison
>> -> probably something else going on
> Can you use my test code or show me yours? Mine has the harness in
> "string-test.c" and the code under test in libsvn_subr - see attachment
> - and the default build on my platform loads libsvn_subr as a shared
> library. Maybe yours is completely different.
I think our test results are consistent now. For simple
tests like that, I usually misuse svn.exe or so and put
the code into main().
> Other observations:
>
> - Some callers that use very tight loops are in fact simply copying a
> string, up as far as some particular end marker. They may benefit from
> being re-written to first determine how much they want to copy and then
> call svn_stringbuf_appendbytyes().
I will browse through the code and see where it can
be reworked easily.
> - The stringbuf type initially allocates a 1-byte space, and doubles
> this every time it re-allocates. Could we not could allocate at least a
> few bytes (4? 8?) as a minimum, without actually consuming more memory
> in practice? That would avoid two or three re-allocs in some usage
> patterns.
True. Two points from my side:
* APR pools allocate memory in 8 byte chunks.
-> allocating just one byte does not save any memory
over allocating 8
-> will post a patch for that
* I added a svn_stringbuf_create_empty() function that
creates a truly empty string (zero capacity) in r985697
and used it r985673. That is usefuly if the actual required
capacity will be known only later - but before data is being
added to the string. Alas, these empty strings seem to
break some assumptions and cannot be used freely
in all suitable places. I tried that and got errors.
-- Stefan^2.
Re: Performance optimization - svn_stringbuf_appendbyte()
Posted by Julian Foad <ju...@wandisco.com>.
On Sun, 2010-10-10, Stefan Fuhrmann wrote:
> On 07.10.2010 16:07, Julian Foad wrote:
> >> New Revision: 997203
> >>
> >> URL: http://svn.apache.org/viewvc?rev=997203&view=rev
> >> Log:
> >> Merge r985037, r985046, r995507 and r995603 from the performance branch.
> >>
> >> These changes introduce the svn_stringbuf_appendbyte() function, which has
> >> significantly less overhead than svn_stringbuf_appendbytes(), and can be
> >> used in a number of places within our codebase.
> > Hi Stefan2.
> >
> > I have been wondering about the actual benefit of such tightly
> > hand-optimized code. Today I got around to giving it a quick spin.
> >
> > In my first test, it made no difference whatsoever, because the
> > optimized code is never executed. The recent merge r1002898 of r1001413
> > from the performance branch introduced a bug:
> >
> > - if (str->blocksize> old_len + 1)
> > + if (str->blocksize< old_len + 1)
> WTF how did that happen?
> Well, that's what reviews are for ;)
> > which totally disables the optimized code path.
> >
> > Fixed in r1005437.
> Thanks.
> > That solved, a loop doing a million simple appendbyte calls runs more
> > than twice as fast as calling appendbytes(..., 1). That's fantastic.
> >
> > But what about that hand-optimization? I wrote a simple version of the
> > function, and called it 'appendbyte0':
> >
> > svn_stringbuf_appendbyte0(svn_stringbuf_t *str, char byte)
> > {
> > if (str->blocksize> str->len + 1)
> > {
> > str->data[str->len++] = byte;
> > str->data[str->len] = '\0';
> > }
> > else
> > svn_stringbuf_appendbytes(str,&byte, 1);
> > }
> >
> > Here are the results (see full patch attached):
> >
> > Times: appendbytes appendbyte0 appendbyte (in ms)
> > run: 89 31 34
> > run: 88 30 35
> > run: 88 31 34
> > run: 88 30 34
> > run: 88 31 34
> > min: 88 30 34
> >
> > This tells me that the hand-optimization is actually harmful and the
> > compiler does a 10% better job by itself.
> >
> > Have I made a mistake?
> My guess is that you might have made two mistakes actually.
Heh, OK.
> First, the number of operations was relatively low - everything
> in the low ms range could be due to secondary effects (and
> still be repeatable).
I can't think of any reason why. I ran the whole benchmark lots of
times. Occasionally some of the times were a big chunk longer due to
other system activity. Normally they were stable. I also ran it
several times with 10 million ops instead of 1 million, and the results
were exactly 10x longer with the same degree of variability.
> The most important problem would be compiler optimizations
> or lack thereof. Since the numbers are very large (50..100
> ticks per byte, depending on your CPU clock), I would assume
> that you used a normal debug build for testing. In that case,
You're right. I'll try again, with "--disable-debug -O2".
> the actual number of C statements has a large impact on
> the execution time. See my results below for details.
> > What are the results for your system?
> >
> > (I'm using GCC 4.4.1 on an Intel Centrino laptop CPU.)
> >
> Test code used (10^10 calls, newer re-allocate):
>
> int i;
> unsigned char c;
>
> svn_stringbuf_t* s = svn_stringbuf_create_ensure (255, pool);
OK so you're avoiding any calls to the "re-alloc". My tests included
re-allocs, but were long enough strings that this wasn't much overhead.
Nevertheless I'll avoid re-allocs so we can compare results fairly.
> for (i = 0; i < 50000000; ++i)
> {
> s->len = 0;
> for (c = 0; c < 200; ++c)
> svn_stringbuf_appendbyte (s, c);
> }
>
>
> XEON 55xx / Core i7, hyper-threading on, 3GHz peak
> 64 bits Linux, GCC 4.3.3; ./configure --disable-debug
>
> (1) 10,7s; IPC = 2.1
> (2) 8,11s; IPC = 1.4
> (3) 2,64s; IPC = 2.4
> (4) 2,43s; IPC = 2.3
> (1) use appendbytes gives 9 instructions in main, 59 in appbytes
> (2) handle count==1 directly in in appendbytes; 9 inst. in main, 26 in
> appbytes
> (3) appendbyte0 (same compiler output as the the non-handtuned code);
> 13 inst. in appbyte, 6 in main
> (4) trunk@HEAD appendbyte; 11 inst. in appbyte, 6 in main
>
> Core2 2.4GHz, Win32, VS2010
> (not using valgrind to count instructions here; also not testing (2))
>
> (1) 17,0s release, 20,2s debug
> (3) 10,6s release, 12,2s debug
> (4) 9,7s release, 13,0s debug
With a test harness more similar to yours, and built with
"--disable-debug -O2", here are my relative numbers (but only 1 million
outer loops, compared to your 50 million):
$ svn-c-test subr string 24
Times for 1000000 x 200 bytes, in seconds
(1)appendbytes (3)appendbyte0 (4)trunk@HEAD (5)macro
run: 7.03 2.06 1.37 0.53
run: 6.62 1.76 1.55 0.53
run: 6.53 1.67 1.44 0.53
run: 6.54 1.60 1.44 0.53
run: 6.52 1.84 1.37 0.53
min: 6.52 1.60 1.37 0.53
This agrees with what you found: the hand-tuned code gives a 10%
improvement.
This also shows another variant, writing the function as a macro,
reported in the column "(5)macro". This gives a further two-and-a-half
times speed-up in this test.
[...]
> * your numbers seem very large in comparison
> -> probably something else going on
Can you use my test code or show me yours? Mine has the harness in
"string-test.c" and the code under test in libsvn_subr - see attachment
- and the default build on my platform loads libsvn_subr as a shared
library. Maybe yours is completely different.
Other observations:
- Some callers that use very tight loops are in fact simply copying a
string, up as far as some particular end marker. They may benefit from
being re-written to first determine how much they want to copy and then
call svn_stringbuf_appendbytyes().
- The stringbuf type initially allocates a 1-byte space, and doubles
this every time it re-allocates. Could we not could allocate at least a
few bytes (4? 8?) as a minimum, without actually consuming more memory
in practice? That would avoid two or three re-allocs in some usage
patterns.
- Julian
Re: Performance optimization - svn_stringbuf_appendbyte()
Posted by Julian Foad <ju...@wandisco.com>.
On Sun, 2010-10-10, Stefan Fuhrmann wrote:
> On 07.10.2010 16:07, Julian Foad wrote:
> >> New Revision: 997203
> >>
> >> URL: http://svn.apache.org/viewvc?rev=997203&view=rev
> >> Log:
> >> Merge r985037, r985046, r995507 and r995603 from the performance branch.
> >>
> >> These changes introduce the svn_stringbuf_appendbyte() function, which has
> >> significantly less overhead than svn_stringbuf_appendbytes(), and can be
> >> used in a number of places within our codebase.
> > Hi Stefan2.
> >
> > I have been wondering about the actual benefit of such tightly
> > hand-optimized code. Today I got around to giving it a quick spin.
> >
> > In my first test, it made no difference whatsoever, because the
> > optimized code is never executed. The recent merge r1002898 of r1001413
> > from the performance branch introduced a bug:
> >
> > - if (str->blocksize> old_len + 1)
> > + if (str->blocksize< old_len + 1)
> WTF how did that happen?
> Well, that's what reviews are for ;)
> > which totally disables the optimized code path.
> >
> > Fixed in r1005437.
> Thanks.
> > That solved, a loop doing a million simple appendbyte calls runs more
> > than twice as fast as calling appendbytes(..., 1). That's fantastic.
> >
> > But what about that hand-optimization? I wrote a simple version of the
> > function, and called it 'appendbyte0':
> >
> > svn_stringbuf_appendbyte0(svn_stringbuf_t *str, char byte)
> > {
> > if (str->blocksize> str->len + 1)
> > {
> > str->data[str->len++] = byte;
> > str->data[str->len] = '\0';
> > }
> > else
> > svn_stringbuf_appendbytes(str,&byte, 1);
> > }
> >
> > Here are the results (see full patch attached):
> >
> > Times: appendbytes appendbyte0 appendbyte (in ms)
> > run: 89 31 34
> > run: 88 30 35
> > run: 88 31 34
> > run: 88 30 34
> > run: 88 31 34
> > min: 88 30 34
> >
> > This tells me that the hand-optimization is actually harmful and the
> > compiler does a 10% better job by itself.
> >
> > Have I made a mistake?
> My guess is that you might have made two mistakes actually.
Heh, OK.
> First, the number of operations was relatively low - everything
> in the low ms range could be due to secondary effects (and
> still be repeatable).
I can't think of any reason why. I ran the whole benchmark lots of
times. Occasionally some of the times were a big chunk longer due to
other system activity. Normally they were stable. I also ran it
several times with 10 million ops instead of 1 million, and the results
were exactly 10x longer with the same degree of variability.
> The most important problem would be compiler optimizations
> or lack thereof. Since the numbers are very large (50..100
> ticks per byte, depending on your CPU clock), I would assume
> that you used a normal debug build for testing. In that case,
You're right. I'll try again, with "--disable-debug -O2".
> the actual number of C statements has a large impact on
> the execution time. See my results below for details.
> > What are the results for your system?
> >
> > (I'm using GCC 4.4.1 on an Intel Centrino laptop CPU.)
> >
> Test code used (10^10 calls, newer re-allocate):
>
> int i;
> unsigned char c;
>
> svn_stringbuf_t* s = svn_stringbuf_create_ensure (255, pool);
OK so you're avoiding any calls to the "re-alloc". My tests included
re-allocs, but were long enough strings that this wasn't much overhead.
Nevertheless I'll avoid re-allocs so we can compare results fairly.
> for (i = 0; i < 50000000; ++i)
> {
> s->len = 0;
> for (c = 0; c < 200; ++c)
> svn_stringbuf_appendbyte (s, c);
> }
>
>
> XEON 55xx / Core i7, hyper-threading on, 3GHz peak
> 64 bits Linux, GCC 4.3.3; ./configure --disable-debug
>
> (1) 10,7s; IPC = 2.1
> (2) 8,11s; IPC = 1.4
> (3) 2,64s; IPC = 2.4
> (4) 2,43s; IPC = 2.3
> (1) use appendbytes gives 9 instructions in main, 59 in appbytes
> (2) handle count==1 directly in in appendbytes; 9 inst. in main, 26 in
> appbytes
> (3) appendbyte0 (same compiler output as the the non-handtuned code);
> 13 inst. in appbyte, 6 in main
> (4) trunk@HEAD appendbyte; 11 inst. in appbyte, 6 in main
>
> Core2 2.4GHz, Win32, VS2010
> (not using valgrind to count instructions here; also not testing (2))
>
> (1) 17,0s release, 20,2s debug
> (3) 10,6s release, 12,2s debug
> (4) 9,7s release, 13,0s debug
With a test harness more similar to yours, and built with
"--disable-debug -O2", here are my relative numbers (but only 1 million
outer loops, compared to your 50 million):
$ svn-c-test subr string 24
Times for 1000000 x 200 bytes, in seconds
(1)appendbytes (3)appendbyte0 (4)trunk@HEAD (5)macro
run: 7.03 2.06 1.37 0.53
run: 6.62 1.76 1.55 0.53
run: 6.53 1.67 1.44 0.53
run: 6.54 1.60 1.44 0.53
run: 6.52 1.84 1.37 0.53
min: 6.52 1.60 1.37 0.53
This agrees with what you found: the hand-tuned code gives a 10%
improvement.
This also shows another variant, writing the function as a macro,
reported in the column "(5)macro". This gives a further two-and-a-half
times speed-up in this test.
[...]
> * your numbers seem very large in comparison
> -> probably something else going on
Can you use my test code or show me yours? Mine has the harness in
"string-test.c" and the code under test in libsvn_subr - see attachment
- and the default build on my platform loads libsvn_subr as a shared
library. Maybe yours is completely different.
Other observations:
- Some callers that use very tight loops are in fact simply copying a
string, up as far as some particular end marker. They may benefit from
being re-written to first determine how much they want to copy and then
call svn_stringbuf_appendbytyes().
- The stringbuf type initially allocates a 1-byte space, and doubles
this every time it re-allocates. Could we not could allocate at least a
few bytes (4? 8?) as a minimum, without actually consuming more memory
in practice? That would avoid two or three re-allocs in some usage
patterns.
- Julian
Re: Performance optimization - svn_stringbuf_appendbyte()
Posted by Stefan Fuhrmann <st...@alice-dsl.de>.
On 07.10.2010 16:07, Julian Foad wrote:
>> New Revision: 997203
>>
>> URL: http://svn.apache.org/viewvc?rev=997203&view=rev
>> Log:
>> Merge r985037, r985046, r995507 and r995603 from the performance branch.
>>
>> These changes introduce the svn_stringbuf_appendbyte() function, which has
>> significantly less overhead than svn_stringbuf_appendbytes(), and can be
>> used in a number of places within our codebase.
> Hi Stefan2.
>
> I have been wondering about the actual benefit of such tightly
> hand-optimized code. Today I got around to giving it a quick spin.
>
> In my first test, it made no difference whatsoever, because the
> optimized code is never executed. The recent merge r1002898 of r1001413
> from the performance branch introduced a bug:
>
> - if (str->blocksize> old_len + 1)
> + if (str->blocksize< old_len + 1)
WTF how did that happen?
Well, that's what reviews are for ;)
> which totally disables the optimized code path.
>
> Fixed in r1005437.
Thanks.
> That solved, a loop doing a million simple appendbyte calls runs more
> than twice as fast as calling appendbytes(..., 1). That's fantastic.
>
> But what about that hand-optimization? I wrote a simple version of the
> function, and called it 'appendbyte0':
>
> svn_stringbuf_appendbyte0(svn_stringbuf_t *str, char byte)
> {
> if (str->blocksize> str->len + 1)
> {
> str->data[str->len++] = byte;
> str->data[str->len] = '\0';
> }
> else
> svn_stringbuf_appendbytes(str,&byte, 1);
> }
>
> Here are the results (see full patch attached):
>
> Times: appendbytes appendbyte0 appendbyte (in ms)
> run: 89 31 34
> run: 88 30 35
> run: 88 31 34
> run: 88 30 34
> run: 88 31 34
> min: 88 30 34
>
> This tells me that the hand-optimization is actually harmful and the
> compiler does a 10% better job by itself.
>
> Have I made a mistake?
My guess is that you might have made two mistakes actually.
First, the number of operations was relatively low - everything
in the low ms range could be due to secondary effects (and
still be repeatable).
The most important problem would be compiler optimizations
or lack thereof. Since the numbers are very large (50..100
ticks per byte, depending on your CPU clock), I would assume
that you used a normal debug build for testing. In that case,
the actual number of C statements has a large impact on
the execution time. See my results below for details.
> What are the results for your system?
>
> (I'm using GCC 4.4.1 on an Intel Centrino laptop CPU.)
>
Test code used (10^10 calls, newer re-allocate):
int i;
unsigned char c;
svn_stringbuf_t* s = svn_stringbuf_create_ensure (255, pool);
for (i = 0; i < 50000000; ++i)
{
s->len = 0;
for (c = 0; c < 200; ++c)
svn_stringbuf_appendbyte (s, c);
}
XEON 55xx / Core i7, hyper-threading on, 3GHz peak
64 bits Linux, GCC 4.3.3; ./configure --disable-debug
(1) 10,7s; IPC = 2.1
(2) 8,11s; IPC = 1.4
(3) 2,64s; IPC = 2.4
(4) 2,43s; IPC = 2.3
(1) use appendbytes gives 9 instructions in main, 59 in appbytes
(2) handle count==1 directly in in appendbytes; 9 inst. in main, 26 in
appbytes
(3) appendbyte0 (same compiler output as the the non-handtuned code);
13 inst. in appbyte, 6 in main
(4) trunk@HEAD appendbyte; 11 inst. in appbyte, 6 in main
Core2 2.4GHz, Win32, VS2010
(not using valgrind to count instructions here; also not testing (2))
(1) 17,0s release, 20,2s debug
(3) 10,6s release, 12,2s debug
(4) 9,7s release, 13,0s debug
That shows the following:
* for debug code, appendbyte0 is faster
(because debug code stores all variables on stack and the
hand-tuned code introduced some additional ones)
* the ABI has significant effect (Win32: all parameters put on stack;
Lin64: all fit into registers)
* shorter API (function signature) speeds up the *caller* significantly
* as long as small functions don't need to r/w to stack they can
be very fast (64 bit gives more registers, parameters passed in
registers eliminate stack frame ops, simple functions don't need
more than the available scratch registers)
* your numbers seem very large in comparison
-> probably something else going on
-- Stefan^2
Re: Performance optimization - svn_stringbuf_appendbyte()
Posted by Julian Foad <ju...@wandisco.com>.
> New Revision: 997203
>
> URL: http://svn.apache.org/viewvc?rev=997203&view=rev
> Log:
> Merge r985037, r985046, r995507 and r995603 from the performance branch.
>
> These changes introduce the svn_stringbuf_appendbyte() function, which has
> significantly less overhead than svn_stringbuf_appendbytes(), and can be
> used in a number of places within our codebase.
Hi Stefan2.
I have been wondering about the actual benefit of such tightly
hand-optimized code. Today I got around to giving it a quick spin.
In my first test, it made no difference whatsoever, because the
optimized code is never executed. The recent merge r1002898 of r1001413
from the performance branch introduced a bug:
- if (str->blocksize > old_len + 1)
+ if (str->blocksize < old_len + 1)
which totally disables the optimized code path.
Fixed in r1005437.
That solved, a loop doing a million simple appendbyte calls runs more
than twice as fast as calling appendbytes(..., 1). That's fantastic.
But what about that hand-optimization? I wrote a simple version of the
function, and called it 'appendbyte0':
svn_stringbuf_appendbyte0(svn_stringbuf_t *str, char byte)
{
if (str->blocksize > str->len + 1)
{
str->data[str->len++] = byte;
str->data[str->len] = '\0';
}
else
svn_stringbuf_appendbytes(str, &byte, 1);
}
Here are the results (see full patch attached):
Times: appendbytes appendbyte0 appendbyte (in ms)
run: 89 31 34
run: 88 30 35
run: 88 31 34
run: 88 30 34
run: 88 31 34
min: 88 30 34
This tells me that the hand-optimization is actually harmful and the
compiler does a 10% better job by itself.
Have I made a mistake?
What are the results for your system?
(I'm using GCC 4.4.1 on an Intel Centrino laptop CPU.)
- Julian